Re: [PATCH v2] diskdump: Add support for reading dumpfiles compressed by Zstandard

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Philipp,

-----Original Message-----
> 
> Hi Kazu,
> 
> the patch follows the snappy and lzo example and all in all the
> looks good to me. Two small comments below though. Nevertheless
> 
> Reviewed-by: Philipp Rudo <prudo@xxxxxxxxxx>

Thanks for reviewing this!

> 
> 
> On Fri, 17 Sep 2021 07:55:32 +0000
> HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> wrote:
> 
> > -----Original Message-----
> > > @@ -1002,6 +1006,11 @@ is_diskdump(char *file)
> > >  	dd->flags |= SNAPPY_SUPPORTED;
> > >  #endif
> > >
> > > +#ifdef ZSTD
> > > +	if ((dctx = ZSTD_createDCtx()) != NULL)
> > > +		dd->flags |= ZSTD_SUPPORTED;
> >
> > I found that this part allocates dctx superfluously for split dumpfiles,
> > so sending v2.
> >
> > --
> > From: Kazuhito Hagio <k-hagio@xxxxxxxxxxxxx>
> > Subject: [PATCH] diskdump: Add support for reading dumpfiles compressed by
> >  Zstandard
> >
> > Add support for reading dumpfiles compressed by Zstandard (zstd)
> > using makedumpfile.
> >
> > To build crash with zstd support, type "make zstd".
> >
> > Signed-off-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
> > ---
> >  Makefile    |  4 ++++
> >  README      |  4 ++--
> >  configure.c | 20 +++++++++++++++++---
> >  defs.h      |  4 ++++
> >  diskdump.c  | 33 +++++++++++++++++++++++++++++++++
> >  diskdump.h  |  1 +
> >  help.c      |  4 ++--
> >  7 files changed, 63 insertions(+), 7 deletions(-)
> 
> [...]
> 
> > --- a/configure.c
> > +++ b/configure.c
> > @@ -1746,6 +1746,7 @@ add_extra_lib(char *option)
> 
> You should probably add a 'For ZSTD:' section in the comment above this
> function as well.

Agree, will add in v3.

> 
> >  {
> >  	int lzo, add_DLZO, add_llzo2;
> >  	int snappy, add_DSNAPPY, add_lsnappy;
> > +	int zstd, add_DZSTD, add_lzstd;
> >  	int valgrind, add_DVALGRIND;
> >  	char *cflags, *ldflags;
> >  	FILE *fp_cflags, *fp_ldflags;
> 
> [...]
> 
> > --- a/diskdump.c
> > +++ b/diskdump.c
> > @@ -96,6 +96,10 @@ static struct diskdump_data **dd_list = NULL;
> >  static int num_dd = 0;
> >  static int num_dumpfiles = 0;
> >
> > +#ifdef ZSTD
> > +static ZSTD_DCtx *dctx = NULL;
> > +#endif
> > +
> >  int dumpfile_is_split(void)
> >  {
> >  	return KDUMP_SPLIT();
> > @@ -1002,6 +1006,13 @@ is_diskdump(char *file)
> >  	dd->flags |= SNAPPY_SUPPORTED;
> >  #endif
> >
> > +#ifdef ZSTD
> > +	if (!dctx)
> > +		dctx = ZSTD_createDCtx();
> > +	if (dctx)
> > +		dd->flags |= ZSTD_SUPPORTED;
> > +#endif
> > +
> 
> would it make sense to only set ZSTD_SUPPORTED here and move the
> creation of dctx to cache_page where it is used? That at least would
> save a couple of bytes when zstd is supported but the dump is not zstd
> compressed.

Makes sense.  This also will make an error distinguishable between
no zstd support or failure of ZSTD_createDCtx().

will post v3.

Thanks,
Kazu

> 
> Thanks
> Philipp
> 
> >  	pc->read_vmcoreinfo = vmcoreinfo_read_string;
> >
> >  	if ((pc->flags2 & GET_LOG) && KDUMP_CMPRS_VALID()) {
> > @@ -1251,6 +1262,24 @@ cache_page(physaddr_t paddr)
> >  			      ret);
> >  			return READ_ERROR;
> >  		}
> > +#endif
> > +	} else if (pd.flags & DUMP_DH_COMPRESSED_ZSTD) {
> > +
> > +		if (!(dd->flags & ZSTD_SUPPORTED)) {
> > +			error(INFO, "%s: uncompess failed: no zstd compression support\n",
> > +				DISKDUMP_VALID() ? "diskdump" : "compressed kdump");
> > +			return READ_ERROR;
> > +		}
> > +#ifdef ZSTD
> > +		retlen = ZSTD_decompressDCtx(dctx,
> > +				dd->page_cache_hdr[i].pg_bufptr, block_size,
> > +				dd->compressed_page, pd.size);
> > +		if (ZSTD_isError(retlen)) {
> > +			error(INFO, "%s: uncompress failed: %d (%s)\n",
> > +				DISKDUMP_VALID() ? "diskdump" : "compressed kdump",
> > +				retlen, ZSTD_getErrorName(retlen));
> > +			return READ_ERROR;
> > +		}
> >  #endif
> >  	} else
> >  		memcpy(dd->page_cache_hdr[i].pg_bufptr,
> > @@ -1806,6 +1835,8 @@ __diskdump_memory_dump(FILE *fp)
> >  		fprintf(fp, "%sLZO_SUPPORTED", others++ ? "|" : "");
> >  	if (dd->flags & SNAPPY_SUPPORTED)
> >  		fprintf(fp, "%sSNAPPY_SUPPORTED", others++ ? "|" : "");
> > +	if (dd->flags & ZSTD_SUPPORTED)
> > +		fprintf(fp, "%sZSTD_SUPPORTED", others++ ? "|" : "");
> >          fprintf(fp, ") %s\n", FLAT_FORMAT() ? "[FLAT]" : "");
> >          fprintf(fp, "               dfd: %d\n", dd->dfd);
> >          fprintf(fp, "               ofp: %lx\n", (ulong)dd->ofp);
> > @@ -1872,6 +1903,8 @@ __diskdump_memory_dump(FILE *fp)
> >  			fprintf(fp, "DUMP_DH_COMPRESSED_LZO");
> >  		if (dh->status & DUMP_DH_COMPRESSED_SNAPPY)
> >  			fprintf(fp, "DUMP_DH_COMPRESSED_SNAPPY");
> > +		if (dh->status & DUMP_DH_COMPRESSED_ZSTD)
> > +			fprintf(fp, "DUMP_DH_COMPRESSED_ZSTD");
> >  		if (dh->status & DUMP_DH_COMPRESSED_INCOMPLETE)
> >  			fprintf(fp, "DUMP_DH_COMPRESSED_INCOMPLETE");
> >  		if (dh->status & DUMP_DH_EXCLUDED_VMEMMAP)
> > diff --git a/diskdump.h b/diskdump.h
> > index 28713407b841..c152c7b86616 100644
> > --- a/diskdump.h
> > +++ b/diskdump.h
> > @@ -86,6 +86,7 @@ struct kdump_sub_header {
> >  #define DUMP_DH_COMPRESSED_SNAPPY  0x4   /* page is compressed with snappy */
> >  #define DUMP_DH_COMPRESSED_INCOMPLETE  0x8   /* dumpfile is incomplete */
> >  #define DUMP_DH_EXCLUDED_VMEMMAP   0x10  /* unused vmemmap pages are excluded */
> > +#define DUMP_DH_COMPRESSED_ZSTD    0x20  /* page is compressed with zstd */
> >
> >  /* descriptor of each page for vmcore */
> >  typedef struct page_desc {
> > diff --git a/help.c b/help.c
> > index 6c262a3ffcbb..f34838d59908 100644
> > --- a/help.c
> > +++ b/help.c
> > @@ -9420,8 +9420,8 @@ README_ENTER_DIRECTORY,
> >  "  Traditionally when vmcores are compressed via the makedumpfile(8) facility",
> >  "  the libz compression library is used, and by default the crash utility",
> >  "  only supports libz.  Recently makedumpfile has been enhanced to optionally",
> > -"  use either the LZO or snappy compression libraries.  To build crash with",
> > -"  either or both of those libraries, type \"make lzo\" or \"make snappy\".",
> > +"  use the LZO, snappy or zstd compression libraries.  To build crash with any",
> > +"  or all of those libraries, type \"make lzo\", \"make snappy\" or \"make zstd\".",
> >  "",
> >  "  crash supports valgrind Memcheck tool on the crash's custom memory allocator.",
> >  "  To build crash with this feature enabled, type \"make valgrind\" and then run",

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux