Re: [Crash-utility] [PATCH 5/5] [RFC] Replace lseek/read into pread for kcore and vmcore reading

>>> On 2023/03/25 13:12, Tao Liu wrote: >>>> Previously crash use lseek/read for kcore and vmcore reading, this > involves 2 >>>> syscalls. And we can replace them with pread, only 1 syscall is needed > for >>>> kcore/vmcore reading, and we can have a better performance. Please > note there >>>> are plenty of places in crash using lseek/read, this patch doesn't > modify all >>>> of them, just the most commonly used kcore and diskdump vmcore reading. >>> >>> This is nice as an optimization, but can we distinguish a seek error and >>> read error with errno as currently we can do? >>> >> >> Sorry I didn't understand your question... >> >> The functions which I modified in this patch didn't use a global errno >> to distinguish error types, they used a returned value instead. >> Currently if read error, READ_ERROR will be returned, same as seek >> error returns SEEK_ERROR. Do you want to distinguish pread error by >> introducing a new error number as PREAD_ERROR? > > No, I would like to know which causes an error in pread(), seek or read. > In other words, is there any way to know which is wrong, the specified > offset or file/media? > > Good questions. In fact, the SEEK_ERROR is simply returned to the top-level call, but it seems that there is no corresponding processing for the type of errors at the top-level code. Only saw two cases(please correct me if I missed anything): [1] readmem() ... case SEEK_ERROR: if (PRINT_ERROR_MESSAGE) error(INFO, SEEK_ERRMSG, memtype_string(memtype, 0), addr, type); goto readmem_error; case READ_ERROR: if (PRINT_ERROR_MESSAGE) error(INFO, READ_ERRMSG, memtype_string(memtype, 0), addr, type); if ((pc->flags & DEVMEM) && (kt->flags & PRE_KERNEL_INIT) && !(error_handle & NO_DEVMEM_SWITCH) && devmem_is_restricted() && switch_to_proc_kcore()) { error_handle &= ~QUIET; return(readmem(addr, memtype, bufptr, size, type, error_handle)); } goto readmem_error;.. [2] read_diskdump() ... if ((ret = cache_page(curpaddr)) < 0) { if (CRASHDEBUG(8)) fprintf(fp, "read_diskdump: " "%s: cannot cache page: %llx\n", ret == SEEK_ERROR ? "SEEK_ERROR" : "READ_ERROR", (ulonglong)curpaddr); return ret; } ... Given that, only recognizing the type of errors, it can not do anything. Would it really make sense to distinguish the type of errors(seek/read errors)?

BTW: Could it be helpful to print an actual error when the pread() fails? For example: + ssize_t prdcnt = pread(fd, bufptr, readcnt, offset); + if (prdcnt != readcnt) { + if (prdcnt == -1) + error(WARNING, "/proc/kcore: %s\n", strerror(errno)); return READ_ERROR; + }