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

On Thu, Apr 20, 2023 at 8:00 PM <crash-utility-request(a)redhat.com&gt; wrote: > Date: Thu, 20 Apr 2023 18:25:22 +0800 > From: Tao Liu <ltao(a)redhat.com&gt; > To: crash-utility(a)redhat.com > Subject: [Crash-utility] [PATCH v2] Replace lseek/read into pread for > kcore and vmcore reading. > Message-ID: <20230420102521.33698-1-ltao(a)redhat.com&gt; > Content-Type: text/plain; charset="US-ASCII"; x-default=true > > 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. > > Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > --- > > v1 -> v2: add offset check before pread. > > --- > diskdump.c | 31 +++++++++++++++++++++++++------ > netdump.c | 19 ++++++++++++++----- > 2 files changed, 39 insertions(+), 11 deletions(-) > > diff --git a/diskdump.c b/diskdump.c > index cf5f5d9..6ecd08a 100644 > --- a/diskdump.c > +++ b/diskdump.c > @@ -515,16 +515,24 @@ arm_kdump_header_adjust(int header_version) > static int > read_pd(int fd, off_t offset, page_desc_t *pd) > { > - const off_t failed = (off_t)-1; > + int ret; > > if (FLAT_FORMAT()) { > if (!read_flattened_format(fd, offset, pd, sizeof(*pd))) > return READ_ERROR; > } else { > - if (lseek(fd, offset, SEEK_SET) == failed) > + if (offset < 0) { > + if (CRASHDEBUG(8)) { > + fprintf(fp, "read_pd: invalid offset: > %lx\n", offset); > + } > return SEEK_ERROR; > - if (read(fd, pd, sizeof(*pd)) != sizeof(*pd)) > + } > + if ((ret = pread(fd, pd, sizeof(*pd), offset)) != > sizeof(*pd)) { > + if (ret == -1 && CRASHDEBUG(8)) { > + fprintf(fp, "read_pd: pread error: %s\n", > strerror(errno)); > + } > return READ_ERROR; > + } > } > > return 0; > @@ -1125,7 +1133,6 @@ cache_page(physaddr_t paddr) > off_t seek_offset; > page_desc_t pd; > const int block_size = dd->block_size; > - const off_t failed = (off_t)-1; > ulong retlen; > #ifdef ZSTD > static ZSTD_DCtx *dctx = NULL; > @@ -1190,10 +1197,22 @@ cache_page(physaddr_t paddr) > return PAGE_INCOMPLETE; > } > } else { > - if (lseek(dd->dfd, pd.offset, SEEK_SET) == failed) > + if (pd.offset < 0) { > + if (CRASHDEBUG(8)) { > + fprintf(fp, > + "read_diskdump/cache_page: " > + "invalid offset: %lx\n", > pd.offset); > + } > return SEEK_ERROR; > - if (read(dd->dfd, dd->compressed_page, pd.size) != pd.size) > + } > + if ((ret = pread(dd->dfd, dd->compressed_page, pd.size, > pd.offset)) != pd.size) { > + if (ret == -1 && CRASHDEBUG(8)) { > + fprintf(fp, > + "read_diskdump/cache_page: " > + "pread error: %s\n", > strerror(errno)); > + } > return READ_ERROR; > + } > } > > if (pd.flags & DUMP_DH_COMPRESSED_ZLIB) { > diff --git a/netdump.c b/netdump.c > index 01af145..696f5fd 100644 > --- a/netdump.c > +++ b/netdump.c > @@ -4336,7 +4336,7 @@ no_nt_prstatus_exists: > int > read_proc_kcore(int fd, void *bufptr, int cnt, ulong addr, physaddr_t > paddr) > { > - int i; > + int i, ret; > size_t readcnt; > ulong kvaddr; > Elf32_Phdr *lp32; > @@ -4436,11 +4436,20 @@ read_proc_kcore(int fd, void *bufptr, int cnt, > ulong addr, physaddr_t paddr) > if (offset == UNINITIALIZED) > return SEEK_ERROR; > > - if (lseek(fd, offset, SEEK_SET) != offset) > - perror("lseek"); > - > - if (read(fd, bufptr, readcnt) != readcnt) > + if (offset < 0) { > + if (CRASHDEBUG(8)) { > + fprintf(fp, "read_proc_kcore: " > + "invalid offset: %lx\n", offset); > + } > + return SEEK_ERROR; > + } > + if ((ret = pread(fd, bufptr, readcnt, offset)) != readcnt) { > + if (ret == -1 && CRASHDEBUG(8)) { > + fprintf(fp, "read_proc_kcore: " > + "pread error: %s\n", strerror(errno)); > + } > return READ_ERROR; > + } > > return cnt; > } > Thank you for the update, Tao. The v2 is expected. So: Ack