[Cluster-devel] [PATCH] gfs2_edit: Add compression to savemeta and restoremeta

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

Some comments...

On Thu, 2011-04-14 at 18:08 +0100, Andrew Price wrote:
> savemeta now outputs a gzip-compressed file by default and there is a
> -nocompress option to turn off compression. restoremeta can restore from a
> gzip-compressed or raw metadata file without any extra options.
> 
> The file opening, closing and writing code from savemeta has been abstracted
> away into savemeta{open,close,write} functions to abstract away compressed and
> non-compressed file output.
> 
> Adds a dependency on zlib.
> 
> rhbz#695767
> 
Is there a way to control the compression level? Something like the gzip
-1 to -9 options would be good. It should probably default to -9

> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>  configure.ac          |    1 +
>  gfs2/edit/Makefile.am |    4 +-
>  gfs2/edit/hexedit.c   |   16 +++--
>  gfs2/edit/hexedit.h   |    2 +-
>  gfs2/edit/savemeta.c  |  211 +++++++++++++++++++++++++++++++++++-------------
>  gfs2/man/gfs2_edit.8  |   42 +++++-----
>  6 files changed, 190 insertions(+), 86 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 10bcc95..3fc02d0 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -119,6 +119,7 @@ PKG_CHECK_MODULES([cfg],[libcfg])
>  PKG_CHECK_MODULES([fenced],[libfenced])
>  PKG_CHECK_MODULES([dlmcontrol],[libdlmcontrol])
>  PKG_CHECK_MODULES([quorum],[libquorum])
> +PKG_CHECK_MODULES([zlib],[zlib])
>  
>  # old versions of ncurses don't ship pkg-config files
>  PKG_CHECK_MODULES([ncurses],[ncurses],,
> diff --git a/gfs2/edit/Makefile.am b/gfs2/edit/Makefile.am
> index 9701c91..c4814dc 100644
> --- a/gfs2/edit/Makefile.am
> +++ b/gfs2/edit/Makefile.am
> @@ -10,8 +10,8 @@ gfs2_edit_CPPFLAGS	= -D_FILE_OFFSET_BITS=64 -DHELPER_PROGRAM \
>  			  -I$(top_srcdir)/gfs2/include \
>  			  -I$(top_srcdir)/gfs2/libgfs2
>  
> -gfs2_edit_CFLAGS	= $(ncurses_CFLAGS)
> +gfs2_edit_CFLAGS	= $(ncurses_CFLAGS) $(zlib_CFLAGS)
>  
> -gfs2_edit_LDFLAGS	= $(ncurses_LIBS)
> +gfs2_edit_LDFLAGS	= $(ncurses_LIBS) $(zlib_LIBS)
>  
>  gfs2_edit_LDADD		= $(top_builddir)/gfs2/libgfs2/libgfs2.la
> diff --git a/gfs2/edit/hexedit.c b/gfs2/edit/hexedit.c
> index d8f8841..8f0630b 100644
> --- a/gfs2/edit/hexedit.c
> +++ b/gfs2/edit/hexedit.c
> @@ -41,6 +41,7 @@ struct gfs2_log_header *llh;
>  struct gfs2_log_descriptor *lld;
>  int pgnum;
>  int details = 0;
> +int compressmeta = 1;
>  
>  int display(int identify_only);
>  
> @@ -3294,7 +3295,7 @@ static void dump_journal(const char *journal)
>  /* ------------------------------------------------------------------------ */
>  static void usage(void)
>  {
> -	fprintf(stderr,"\nFormat is: gfs2_edit [-c 1] [-V] [-x] [-h] [identify] [-p structures|blocks][blocktype][blockalloc [val]][blockbits][blockrg][find sb|rg|rb|di|in|lf|jd|lh|ld|ea|ed|lb|13|qc][field <f>[val]] /dev/device\n\n");
> +	fprintf(stderr,"\nFormat is: gfs2_edit [-c 1] [-V] [-x] [-h] [identify] [-nocompress] [-p structures|blocks][blocktype][blockalloc [val]][blockbits][blockrg][find sb|rg|rb|di|in|lf|jd|lh|ld|ea|ed|lb|13|qc][field <f>[val]] /dev/device\n\n");
>  	fprintf(stderr,"If only the device is specified, it enters into hexedit mode.\n");
>  	fprintf(stderr,"identify - prints out only the block type, not the details.\n");
>  	fprintf(stderr,"printsavedmeta - prints out the saved metadata blocks from a savemeta file.\n");
> @@ -3335,6 +3336,7 @@ static void usage(void)
>  	fprintf(stderr,"-p   <b> find sb|rg|rb|di|in|lf|jd|lh|ld|ea|ed|lb|"
>  		"13|qc - find block of given type after block <b>\n");
>  	fprintf(stderr,"     <b> specifies the starting block for search\n");
> +	fprintf(stderr,"-nocompress  don't compress savemeta/savergs output\n");
>  	fprintf(stderr,"-s   specifies a starting block such as root, rindex, quota, inum.\n");
>  	fprintf(stderr,"-x   print in hexmode.\n");
>  	fprintf(stderr,"-h   prints this help.\n\n");
> @@ -3361,8 +3363,8 @@ static void usage(void)
>  	fprintf(stderr,"     gfs2_edit -p quota find di /dev/x/y\n");
>  	fprintf(stderr,"   To set the Resource Group flags for rg #7 to 3.\n");
>  	fprintf(stderr,"     gfs2_edit rgflags 7 3 /dev/sdc2\n");
> -	fprintf(stderr,"   To save off all metadata for /dev/vg/lv:\n");
> -	fprintf(stderr,"     gfs2_edit savemeta /dev/vg/lv /tmp/metasave\n");
> +	fprintf(stderr,"   To save off all metadata for /dev/vg/lv without compression:\n");
> +	fprintf(stderr,"     gfs2_edit -nocompress savemeta /dev/vg/lv /tmp/metasave\n");
>  }/* usage */
>  
>  /* ------------------------------------------------------------------------ */
> @@ -3395,6 +3397,8 @@ static void parameterpass1(int argc, char *argv[], int i)
>  	else if (!strcasecmp(argv[i], "-d") ||
>  		 !strcasecmp(argv[i], "-details"))
>  		details = 1;
> +	else if (!strcasecmp(argv[i], "-nocompress"))
> +		compressmeta = 0;
>  	else if (!strcasecmp(argv[i], "savemeta"))
>  		termlines = 0;
>  	else if (!strcasecmp(argv[i], "savemetaslow"))
> @@ -3559,11 +3563,11 @@ static void process_parameters(int argc, char *argv[], int pass)
>  			}
>  		}
>  		else if (!strcasecmp(argv[i], "savemeta"))
> -			savemeta(argv[i+2], 0);
> +			savemeta(argv[i+2], 0, compressmeta);
>  		else if (!strcasecmp(argv[i], "savemetaslow"))
> -			savemeta(argv[i+2], 1);
> +			savemeta(argv[i+2], 1, compressmeta);
>  		else if (!strcasecmp(argv[i], "savergs"))
> -			savemeta(argv[i+2], 2);
> +			savemeta(argv[i+2], 2, compressmeta);
>  		else if (isdigit(argv[i][0])) { /* decimal addr */
>  			sscanf(argv[i], "%"SCNd64, &temp_blk);
>  			push_block(temp_blk);
> diff --git a/gfs2/edit/hexedit.h b/gfs2/edit/hexedit.h
> index a7a3109..53a6670 100644
> --- a/gfs2/edit/hexedit.h
> +++ b/gfs2/edit/hexedit.h
> @@ -341,7 +341,7 @@ extern void gfs_log_header_in(struct gfs_log_header *head,
>  			      struct gfs2_buffer_head *bh);
>  extern void gfs_log_header_print(struct gfs_log_header *lh);
>  extern void gfs_dinode_in(struct gfs_dinode *di, struct gfs2_buffer_head *bh);
> -extern void savemeta(char *out_fn, int saveoption);
> +extern void savemeta(char *out_fn, int saveoption, int compressmeta);
>  extern void restoremeta(const char *in_fn, const char *out_device,
>  			uint64_t printblocksonly);
>  extern int display(int identify_only);
> diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
> index 4d9f591..b96537e 100644
> --- a/gfs2/edit/savemeta.c
> +++ b/gfs2/edit/savemeta.c
> @@ -18,6 +18,7 @@
>  #include <limits.h>
>  #include <sys/time.h>
>  #include <linux/gfs2_ondisk.h>
> +#include <zlib.h>
>  
>  #include "osi_list.h"
>  #include "gfs2hex.h"
> @@ -34,6 +35,13 @@ struct saved_metablock {
>  	char buf[BUFSIZE];
>  };
>  
> +struct metafd {
> +	int fd;
> +	gzFile gzfd;
> +	const char *filename;
> +	int compressed;
> +};
> +
>  struct saved_metablock *savedata;
>  uint64_t last_fs_block, last_reported_block, blks_saved, total_out, pct;
>  uint64_t journal_blocks[MAX_JOURNALS_SAVED];
> @@ -193,7 +201,98 @@ static void warm_fuzzy_stuff(uint64_t wfsblock, int force)
>  	}
>  }
>  
> -static int save_block(int fd, int out_fd, uint64_t blk)
> +/**
> + * Open a file and prepare it for writing by savemeta()
> + * out_fn: the path to the file, which will be truncated if it exists
> + * compressed: 0 - do not compress the file, 1 - compress the file
> + * Returns a struct metafd containing the opened file descriptor
> + */
> +static struct metafd savemetaopen(char *out_fn, int compressed)
> +{
> +	struct metafd mfd;
> +
> +	if (!out_fn) {
> +		out_fn = strdup(DFT_SAVE_FILE);
why strdup? I can't see where this memory is freed

Otherwise looks good,

Steve.