Date: Tue, 11 Apr 2023 02:01:36 +0000
From: HAGIO KAZUHITO(?????) <k-hagio-ab@nec.com>
To: Tao Liu <ltao@redhat.com>
Cc: "Discussion list for crash utility usage, maintenance and
development" <crash-utility@redhat.com>
Subject: Re: [Crash-utility] [PATCH 5/5] [RFC] Replace lseek/read into
pread for kcore and vmcore reading.
Message-ID: <bfb9b27d-f5b6-9615-3419-24d97c13aeeb@nec.com>
Content-Type: text/plain; charset="utf-8"
On 2023/04/11 10:23, Tao Liu wrote:
> Hi Kazu,
>
> Sorry for the late reply.
No problem :-)
>
> On Fri, Apr 7, 2023 at 10:36?AM HAGIO KAZUHITO(?????)
> <k-hagio-ab@nec.com> wrote:
>>
>> 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;
+ }
Thanks.
Lianbo