[Cluster-devel] [PATCH 0/5] gfs2_edit fixes

[Cluster-devel] [PATCH 0/5] gfs2_edit fixes

[Andrew Price]

This patch set addresses two gfs2_edit issues: one where savemeta fails to save
leaf chain blocks and another where unaligned accesses cause test suite
failures on sparc64.

Andrew Price (5):
  gfs2_edit savemeta: Factor out the bh saving code
  gfs2_edit savemeta: Don't read rgrp blocks twice
  gfs2_edit savemeta: Follow lf_next
  gfs2_edit: Fix unaligned access in restore_init()
  gfs2_edit: Fix unaligned accesses due to saved_metablock size

 gfs2/edit/savemeta.c | 195 +++++++++++++++++++++++++++++----------------------
 1 file changed, 111 insertions(+), 84 deletions(-)

-- 
2.9.3

[Cluster-devel] [PATCH 1/5] gfs2_edit savemeta: Factor out the bh saving code

[Andrew Price]

save_block() accepts a block number, reads it and saves it, which makes
it tricky to save an already-read block without reading it a second
time. Split out the bh saving part of save_block() into save_bh() to
make things more flexible.

Also switch the return value of save_block to only convey error/success
and return the block type through an optional pointer argument instead.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/savemeta.c | 74 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 41 insertions(+), 33 deletions(-)

diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index db9a947..3e8dae0 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -208,14 +208,16 @@ static int get_gfs_struct_info(struct gfs2_buffer_head *lbh, uint64_t owner,
 	struct gfs2_meta_header mh;
 	struct gfs2_inode *inode;
 
-	*block_type = 0;
+	if (block_type != NULL)
+		*block_type = 0;
 	*gstruct_len = sbd.bsize;
 
 	gfs2_meta_header_in(&mh, lbh);
 	if (mh.mh_magic != GFS2_MAGIC)
 		return -1;
 
-	*block_type = mh.mh_type;
+	if (block_type != NULL)
+		*block_type = mh.mh_type;
 
 	switch (mh.mh_type) {
 	case GFS2_METATYPE_SB:   /* 1 (superblock) */
@@ -405,32 +407,19 @@ static int savemetaclose(struct metafd *mfd)
 	return close(mfd->fd);
 }
 
-static int save_block(int fd, struct metafd *mfd, uint64_t blk, uint64_t owner)
+static int save_bh(struct metafd *mfd, struct gfs2_buffer_head *savebh, uint64_t owner, int *blktype)
 {
-	int blktype, blklen;
-	size_t outsz;
-	struct gfs2_buffer_head *savebh;
 	struct saved_metablock *savedata;
-
-	if (gfs2_check_range(&sbd, blk) && blk != sbd.sb_addr) {
-		fprintf(stderr, "\nWarning: bad block pointer '0x%llx' "
-			"ignored in block (block %llu (0x%llx))",
-			(unsigned long long)blk,
-			(unsigned long long)owner, (unsigned long long)owner);
-		return 0;
-	}
-	savebh = bread(&sbd, blk);
+	size_t outsz;
+	int blklen;
 
 	/* If this isn't metadata and isn't a system file, we don't want it.
 	   Note that we're checking "owner" here rather than blk.  That's
 	   because we want to know if the source inode is a system inode
 	   not the block within the inode "blk". They may or may not
 	   be the same thing. */
-	if (get_gfs_struct_info(savebh, owner, &blktype, &blklen) &&
-	    !block_is_systemfile(owner)) {
-		brelse(savebh);
+	if (get_gfs_struct_info(savebh, owner, blktype, &blklen) && !block_is_systemfile(owner))
 		return 0; /* Not metadata, and not system file, so skip it */
-	}
 
 	/* No need to save trailing zeroes */
 	for (; blklen > 0 && savebh->b_data[blklen - 1] == '\0'; blklen--);
@@ -444,7 +433,7 @@ static int save_block(int fd, struct metafd *mfd, uint64_t blk, uint64_t owner)
 		perror("Failed to save block");
 		exit(1);
 	}
-	savedata->blk = cpu_to_be64(blk);
+	savedata->blk = cpu_to_be64(savebh->b_blocknr);
 	savedata->siglen = cpu_to_be16(blklen);
 	memcpy(savedata->buf, savebh->b_data, blklen);
 
@@ -459,8 +448,27 @@ static int save_block(int fd, struct metafd *mfd, uint64_t blk, uint64_t owner)
 
 	blks_saved++;
 	free(savedata);
+	return 0;
+}
+
+static int save_block(int fd, struct metafd *mfd, uint64_t blk, uint64_t owner, int *blktype)
+{
+	struct gfs2_buffer_head *savebh;
+	int err;
+
+	if (gfs2_check_range(&sbd, blk) && blk != sbd.sb_addr) {
+		fprintf(stderr, "\nWarning: bad block pointer '0x%llx' "
+			"ignored in block (block %llu (0x%llx))",
+			(unsigned long long)blk,
+			(unsigned long long)owner, (unsigned long long)owner);
+		return 0;
+	}
+	savebh = bread(&sbd, blk);
+	if (savebh == NULL)
+		return 1;
+	err = save_bh(mfd, savebh, owner, blktype);
 	brelse(savebh);
-	return blktype;
+	return err;
 }
 
 /*
@@ -484,7 +492,7 @@ static void save_ea_block(struct metafd *mfd, struct gfs2_buffer_head *metabh, u
 			b = (uint64_t *)(metabh->b_data);
 			b += charoff + i;
 			blk = be64_to_cpu(*b);
-			save_block(sbd.device_fd, mfd, blk, owner);
+			save_block(sbd.device_fd, mfd, blk, owner, NULL);
 		}
 		if (!ea.ea_rec_len)
 			break;
@@ -514,7 +522,7 @@ static void save_indirect_blocks(struct metafd *mfd, osi_list_t *cur_list,
 		if (indir_block == old_block)
 			continue;
 		old_block = indir_block;
-		blktype = save_block(sbd.device_fd, mfd, indir_block, owner);
+		save_block(sbd.device_fd, mfd, indir_block, owner, &blktype);
 		if (blktype == GFS2_METATYPE_EA) {
 			nbh = bread(&sbd, indir_block);
 			save_ea_block(mfd, nbh, owner);
@@ -625,7 +633,7 @@ static void save_inode_data(struct metafd *mfd, uint64_t iblk)
 			mybh = bread(&sbd, leaf_no);
 			warm_fuzzy_stuff(iblk, FALSE);
 			if (gfs2_check_meta(mybh, GFS2_METATYPE_LF) == 0)
-				save_block(sbd.device_fd, mfd, leaf_no, iblk);
+				save_block(sbd.device_fd, mfd, leaf_no, iblk, NULL);
 			brelse(mybh);
 		}
 	}
@@ -634,7 +642,7 @@ static void save_inode_data(struct metafd *mfd, uint64_t iblk)
 		struct gfs2_buffer_head *lbh;
 
 		lbh = bread(&sbd, inode->i_di.di_eattr);
-		save_block(sbd.device_fd, mfd, inode->i_di.di_eattr, iblk);
+		save_block(sbd.device_fd, mfd, inode->i_di.di_eattr, iblk, NULL);
 		gfs2_meta_header_in(&mh, lbh);
 		if (mh.mh_magic == GFS2_MAGIC &&
 		    mh.mh_type == GFS2_METATYPE_EA)
@@ -644,8 +652,8 @@ static void save_inode_data(struct metafd *mfd, uint64_t iblk)
 			save_indirect_blocks(mfd, cur_list, lbh, iblk, 2, 2);
 		else {
 			if (mh.mh_magic == GFS2_MAGIC) /* if it's metadata */
-				save_block(sbd.device_fd, mfd,
-					   inode->i_di.di_eattr, iblk);
+				save_block(sbd.device_fd, mfd, inode->i_di.di_eattr,
+				           iblk, NULL);
 			fprintf(stderr,
 				"\nWarning: corrupt extended "
 				"attribute at block %llu (0x%llx) "
@@ -722,7 +730,7 @@ static void save_allocated(struct rgrp_tree *rgd, struct metafd *mfd)
 		for (j = 0; j < m; j++) {
 			blk = ibuf[j];
 			warm_fuzzy_stuff(blk, FALSE);
-			blktype = save_block(sbd.device_fd, mfd, blk, blk);
+			save_block(sbd.device_fd, mfd, blk, blk, &blktype);
 			if (blktype == GFS2_METATYPE_DI)
 				save_inode_data(mfd, blk);
 		}
@@ -734,7 +742,7 @@ static void save_allocated(struct rgrp_tree *rgd, struct metafd *mfd)
 		 * If we don't, we may run into metadata allocation issues. */
 		m = lgfs2_bm_scan(rgd, i, ibuf, GFS2_BLKST_UNLINKED);
 		for (j = 0; j < m; j++) {
-			save_block(sbd.device_fd, mfd, blk, blk);
+			save_block(sbd.device_fd, mfd, blk, blk, NULL);
 		}
 	}
 	free(ibuf);
@@ -826,7 +834,7 @@ void savemeta(char *out_fn, int saveoption, int gziplevel)
 		exit(1);
 	}
 	/* Save off the superblock */
-	save_block(sbd.device_fd, &mfd, GFS2_SB_ADDR * GFS2_BASIC_BLOCK / sbd.bsize, 0);
+	save_block(sbd.device_fd, &mfd, GFS2_SB_ADDR * GFS2_BASIC_BLOCK / sbd.bsize, 0, NULL);
 	/* If this is gfs1, save off the rindex because it's not
 	   part of the file system as it is in gfs2. */
 	if (sbd.gfs1) {
@@ -834,7 +842,7 @@ void savemeta(char *out_fn, int saveoption, int gziplevel)
 		int j;
 
 		blk = sbd1->sb_rindex_di.no_addr;
-		save_block(sbd.device_fd, &mfd, blk, blk);
+		save_block(sbd.device_fd, &mfd, blk, blk, NULL);
 		save_inode_data(&mfd, blk);
 		/* In GFS1, journals aren't part of the RG space */
 		for (j = 0; j < journals_found; j++) {
@@ -842,7 +850,7 @@ void savemeta(char *out_fn, int saveoption, int gziplevel)
 			for (blk = journal_blocks[j];
 			     blk < journal_blocks[j] + gfs1_journal_size;
 			     blk++)
-				save_block(sbd.device_fd, &mfd, blk, blk);
+				save_block(sbd.device_fd, &mfd, blk, blk, NULL);
 		}
 	}
 	/* Walk through the resource groups saving everything within */
@@ -861,7 +869,7 @@ void savemeta(char *out_fn, int saveoption, int gziplevel)
 		for (blk = rgd->ri.ri_addr;
 		     blk < rgd->ri.ri_data0; blk++) {
 			warm_fuzzy_stuff(blk, FALSE);
-			save_block(sbd.device_fd, &mfd, blk, blk);
+			save_block(sbd.device_fd, &mfd, blk, blk, NULL);
 		}
 		/* Save off the other metadata: inodes, etc. if mode is not 'savergs' */
 		if (saveoption != 2) {
-- 
2.9.3

[Cluster-devel] [PATCH 2/5] gfs2_edit savemeta: Don't read rgrp blocks twice

[Andrew Price]

When savemeta iterates over the rgrps and saves the rgrp header blocks
it calls save_block which does a bread() for each block despite them
already being read in earlier. Instead, call save_bh() on the existing
rgrp buffers. This isn't likely to give a significant performance
improvement as the duplicate reads would be cache hits but it should
have a noticeable effect on very large file systems.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/savemeta.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index 3e8dae0..9723dcd 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -855,8 +855,8 @@ void savemeta(char *out_fn, int saveoption, int gziplevel)
 	}
 	/* Walk through the resource groups saving everything within */
 	for (n = osi_first(&sbd.rgtree); n; n = osi_next(n)) {
-		uint64_t blk;
 		struct rgrp_tree *rgd;
+		unsigned i;
 
 		rgd = (struct rgrp_tree *)n;
 		if (gfs2_rgrp_read(&sbd, rgd))
@@ -866,10 +866,9 @@ void savemeta(char *out_fn, int saveoption, int gziplevel)
 			  (unsigned long long)rgd->ri.ri_addr,
 			  rgd->ri.ri_length);
 		/* Save off the rg and bitmaps */
-		for (blk = rgd->ri.ri_addr;
-		     blk < rgd->ri.ri_data0; blk++) {
-			warm_fuzzy_stuff(blk, FALSE);
-			save_block(sbd.device_fd, &mfd, blk, blk, NULL);
+		for (i = 0; i < rgd->ri.ri_length; i++) {
+			warm_fuzzy_stuff(rgd->ri.ri_addr + i, FALSE);
+			save_bh(&mfd, rgd->bits[i].bi_bh, 0, NULL);
 		}
 		/* Save off the other metadata: inodes, etc. if mode is not 'savergs' */
 		if (saveoption != 2) {
-- 
2.9.3

[Cluster-devel] [PATCH 3/5] gfs2_edit savemeta: Follow lf_next

[Andrew Price]

Previously, gfs2_edit savemeta did not walk down the lf_next chain to
save the directory leaf blocks that might be there. Add a
save_leaf_chain() function that does that.

Related: rhbz#1382087

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/savemeta.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index 9723dcd..c9b51e9 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -539,12 +539,41 @@ static void save_indirect_blocks(struct metafd *mfd, osi_list_t *cur_list,
 	} /* for all data on the indirect block */
 }
 
+static int save_leaf_chain(struct metafd *mfd, struct gfs2_sbd *sdp, uint64_t blk)
+{
+	struct gfs2_buffer_head *bh;
+	struct gfs2_leaf leaf;
+
+	do {
+		if (gfs2_check_range(sdp, blk) != 0)
+			return 0;
+		bh = bread(sdp, blk);
+		if (bh == NULL) {
+			perror("Failed to read leaf block");
+			return 1;
+		}
+		warm_fuzzy_stuff(blk, FALSE);
+		if (gfs2_check_meta(bh, GFS2_METATYPE_LF) == 0) {
+			int ret = save_bh(mfd, bh, blk, NULL);
+			if (ret != 0) {
+				brelse(bh);
+				return ret;
+			}
+		}
+		gfs2_leaf_in(&leaf, bh);
+		brelse(bh);
+		blk = leaf.lf_next;
+	} while (leaf.lf_next != 0);
+
+	return 0;
+}
+
 /*
  * save_inode_data - save off important data associated with an inode
  *
  * mfd - destination file descriptor
  * iblk - block number of the inode to save the data for
- * 
+ *
  * For user files, we don't want anything except all the indirect block
  * pointers that reside on blocks on all but the highest height.
  *
@@ -626,15 +655,9 @@ static void save_inode_data(struct metafd *mfd, uint64_t iblk)
 				        (uint64_t)inode->i_di.di_num.no_addr);
 				exit(-1);
 			}
-			if (leaf_no == old_leaf ||
-			    gfs2_check_range(&sbd, leaf_no) != 0)
-				continue;
+			if (leaf_no != old_leaf && save_leaf_chain(mfd, &sbd, leaf_no) != 0)
+				exit(-1);
 			old_leaf = leaf_no;
-			mybh = bread(&sbd, leaf_no);
-			warm_fuzzy_stuff(iblk, FALSE);
-			if (gfs2_check_meta(mybh, GFS2_METATYPE_LF) == 0)
-				save_block(sbd.device_fd, mfd, leaf_no, iblk, NULL);
-			brelse(mybh);
 		}
 	}
 	if (inode->i_di.di_eattr) { /* if this inode has extended attributes */
-- 
2.9.3

[Cluster-devel] [PATCH 4/5] gfs2_edit: Fix unaligned access in restore_init()

[Andrew Price]

Scanning this buffer byte-by-byte using casts results in a quick bus
error on sparc64. Avoid this by using a memcpy. This function only runs
once at the beginning of a restoremeta and in most cases the loop will
only execute once so this adds very little overhead.

Reported-By: Valentin Vidic <Valentin.Vidic@CARNet.hr>
Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/savemeta.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index c9b51e9..ac2dd9f 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -924,8 +924,7 @@ static off_t restore_init(gzFile gzfd, struct savemeta_header *smh)
 	size_t rs;
 	char buf[256];
 	off_t startpos = 0;
-	struct saved_metablock *svb;
-	struct gfs2_meta_header *sbmh;
+	struct gfs2_meta_header sbmh;
 
 	err = read_header(gzfd, smh);
 	if (err < 0) {
@@ -943,14 +942,15 @@ static off_t restore_init(gzFile gzfd, struct savemeta_header *smh)
 		exit(1);
 	}
 	/* Scan for the beginning of the file body. Required to support old formats(?). */
-	for (i = 0; i < (256 - sizeof(*svb) - sizeof(*sbmh)); i++) {
-		svb = (struct saved_metablock *)&buf[i];
-		sbmh = (struct gfs2_meta_header *)svb->buf;
-		if (sbmh->mh_magic == cpu_to_be32(GFS2_MAGIC) &&
-		     sbmh->mh_type == cpu_to_be32(GFS2_METATYPE_SB))
+	for (i = 0; i < (256 - sizeof(struct saved_metablock) - sizeof(sbmh)); i++) {
+		off_t off = i + sizeof(struct saved_metablock);
+
+		memcpy(&sbmh, &buf[off], sizeof(sbmh));
+		if (sbmh.mh_magic == cpu_to_be32(GFS2_MAGIC) &&
+		     sbmh.mh_type == cpu_to_be32(GFS2_METATYPE_SB))
 			break;
 	}
-	if (i == (sizeof(buf) - sizeof(*svb) - sizeof(*sbmh)))
+	if (i == (sizeof(buf) - sizeof(struct saved_metablock) - sizeof(sbmh)))
 		i = 0;
 	return startpos + i; /* File offset of saved sb */
 }
-- 
2.9.3

[Cluster-devel] [PATCH 5/5] gfs2_edit: Fix unaligned accesses due to saved_metablock size

[Andrew Price]

Each saved block is prefixed by a saved_metablock structure that holds
its block number and data length. Unfortunately this structure is 10
bytes in size, which means that storing the metadata header in memory
immediately following it causes accesses to the header fields to be
unaligned. This causes testsuite failures on sparc64. Instead, on
restore, use a separate buffer for the block data to ensure aligned
accesses to those fields.

Reported-By: Valentin Vidic <Valentin.Vidic@CARNet.hr>
Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/savemeta.c | 59 +++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 31 deletions(-)

diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index ac2dd9f..f6b7ff0 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -44,7 +44,6 @@ struct savemeta_header {
 struct saved_metablock {
 	uint64_t blk;
 	uint16_t siglen; /* significant data length */
-	char buf[];
 /* This needs to be packed because old versions of gfs2_edit read and write the
    individual fields separately, so the hole after siglen must be eradicated
    before the struct reflects what's on disk. */
@@ -435,7 +434,7 @@ static int save_bh(struct metafd *mfd, struct gfs2_buffer_head *savebh, uint64_t
 	}
 	savedata->blk = cpu_to_be64(savebh->b_blocknr);
 	savedata->siglen = cpu_to_be16(blklen);
-	memcpy(savedata->buf, savebh->b_data, blklen);
+	memcpy(savedata + 1, savebh->b_data, blklen);
 
 	if (savemetawrite(mfd, savedata, outsz) != outsz) {
 		fprintf(stderr, "write error: %s from %s:%d: block %lld (0x%llx)\n",
@@ -956,7 +955,7 @@ static off_t restore_init(gzFile gzfd, struct savemeta_header *smh)
 }
 
 
-static int restore_block(gzFile gzfd, struct saved_metablock *svb, uint16_t maxlen)
+static int restore_block(gzFile gzfd, struct saved_metablock *svb, char *buf, uint16_t maxlen)
 {
 	int gzerr;
 	int ret;
@@ -988,8 +987,8 @@ static int restore_block(gzFile gzfd, struct saved_metablock *svb, uint16_t maxl
 		return -1;
 	}
 
-	if (maxlen) {
-		ret = gzread(gzfd, svb + 1, svb->siglen);
+	if (buf != NULL && maxlen != 0) {
+		ret = gzread(gzfd, buf, svb->siglen);
 		if (ret < svb->siglen) {
 			goto gzread_err;
 		}
@@ -1011,17 +1010,16 @@ gzread_err:
 static int restore_super(gzFile gzfd, off_t pos)
 {
 	int ret;
-	struct saved_metablock *svb;
+	struct saved_metablock svb = {0};
 	struct gfs2_buffer_head dummy_bh;
-	size_t len = sizeof(*svb) + sizeof(struct gfs2_sb);
 
-	svb = calloc(1, len);
-	if (svb == NULL) {
+	dummy_bh.b_data = calloc(1, sizeof(struct gfs2_sb));
+	if (dummy_bh.b_data == NULL) {
 		perror("Failed to restore super block");
 		exit(1);
 	}
 	gzseek(gzfd, pos, SEEK_SET);
-	ret = restore_block(gzfd, svb, sizeof(struct gfs2_sb));
+	ret = restore_block(gzfd, &svb, dummy_bh.b_data, sizeof(struct gfs2_sb));
 	if (ret == 1) {
 		fprintf(stderr, "Reached end of file while restoring superblock\n");
 		goto err;
@@ -1029,7 +1027,6 @@ static int restore_super(gzFile gzfd, off_t pos)
 		goto err;
 	}
 
-	dummy_bh.b_data = (char *)svb->buf;
 	gfs2_sb_in(&sbd.sd_sb, &dummy_bh);
 	sbd1 = (struct gfs_sb *)&sbd.sd_sb;
 	ret = check_sb(&sbd.sd_sb);
@@ -1040,11 +1037,11 @@ static int restore_super(gzFile gzfd, off_t pos)
 	if (ret == 1)
 		sbd.gfs1 = 1;
 	sbd.bsize = sbd.sd_sb.sb_bsize;
-	free(svb);
+	free(dummy_bh.b_data);
 	printf("Block size is %uB\n", sbd.bsize);
 	return 0;
 err:
-	free(svb);
+	free(dummy_bh.b_data);
 	return -1;
 }
 
@@ -1056,7 +1053,7 @@ static int find_highest_block(gzFile gzfd, off_t pos, uint64_t fssize)
 
 	while (1) {
 		gzseek(gzfd, pos, SEEK_SET);
-		err = restore_block(gzfd, &svb, 0);
+		err = restore_block(gzfd, &svb, NULL, 0);
 		if (err == 1)
 			break;
 		if (err != 0)
@@ -1081,12 +1078,12 @@ static int find_highest_block(gzFile gzfd, off_t pos, uint64_t fssize)
 
 static int restore_data(int fd, gzFile gzin_fd, off_t pos, int printonly)
 {
-	struct saved_metablock *savedata;
-	size_t insz = sizeof(*savedata) + sbd.bsize;
+	struct saved_metablock savedata = {0};
 	uint64_t writes = 0;
+	char *buf;
 
-	savedata = calloc(1, insz);
-	if (savedata == NULL) {
+	buf = calloc(1, sbd.bsize);
+	if (buf == NULL) {
 		perror("Failed to restore data");
 		exit(1);
 	}
@@ -1095,36 +1092,36 @@ static int restore_data(int fd, gzFile gzin_fd, off_t pos, int printonly)
 	blks_saved = 0;
 	while (TRUE) {
 		int err;
-		err = restore_block(gzin_fd, savedata, sbd.bsize);
+		err = restore_block(gzin_fd, &savedata, buf, sbd.bsize);
 		if (err == 1)
 			break;
 		if (err != 0) {
-			free(savedata);
+			free(buf);
 			return -1;
 		}
 
 		if (printonly) {
 			struct gfs2_buffer_head dummy_bh = {
-				.b_data = savedata->buf,
-				.b_blocknr = savedata->blk,
+				.b_data = buf,
+				.b_blocknr = savedata.blk,
 			};
-			if (printonly > 1 && printonly == savedata->blk) {
+			if (printonly > 1 && printonly == savedata.blk) {
 				display_block_type(&dummy_bh, TRUE);
 				display_gfs2(&dummy_bh);
 				break;
 			} else if (printonly == 1) {
-				print_gfs2("%"PRId64" (l=0x%x): ", blks_saved, savedata->siglen);
+				print_gfs2("%"PRId64" (l=0x%x): ", blks_saved, savedata.siglen);
 				display_block_type(&dummy_bh, TRUE);
 			}
 		} else {
-			warm_fuzzy_stuff(savedata->blk, FALSE);
-			memset(savedata->buf + savedata->siglen, 0, sbd.bsize - savedata->siglen);
-			if (pwrite(fd, savedata->buf, sbd.bsize, savedata->blk * sbd.bsize) != sbd.bsize) {
+			warm_fuzzy_stuff(savedata.blk, FALSE);
+			memset(buf + savedata.siglen, 0, sbd.bsize - savedata.siglen);
+			if (pwrite(fd, buf, sbd.bsize, savedata.blk * sbd.bsize) != sbd.bsize) {
 				fprintf(stderr, "write error: %s from %s:%d: block %lld (0x%llx)\n",
 					strerror(errno), __FUNCTION__, __LINE__,
-					(unsigned long long)savedata->blk,
-					(unsigned long long)savedata->blk);
-				free(savedata);
+					(unsigned long long)savedata.blk,
+					(unsigned long long)savedata.blk);
+				free(buf);
 				return -1;
 			}
 			writes++;
@@ -1135,7 +1132,7 @@ static int restore_data(int fd, gzFile gzin_fd, off_t pos, int printonly)
 	}
 	if (!printonly)
 		warm_fuzzy_stuff(sbd.fssize, 1);
-	free(savedata);
+	free(buf);
 	return 0;
 }
 
-- 
2.9.3

[Cluster-devel] [PATCH 1/5] gfs2_edit savemeta: Factor out the bh saving code

[Bob Peterson]

----- Original Message -----
| save_block() accepts a block number, reads it and saves it, which makes
| it tricky to save an already-read block without reading it a second
| time. Split out the bh saving part of save_block() into save_bh() to
| make things more flexible.
| 
| Also switch the return value of save_block to only convey error/success
| and return the block type through an optional pointer argument instead.
| 
| Signed-off-by: Andrew Price <anprice@redhat.com>
| ---
Hi,

ACK,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 2/5] gfs2_edit savemeta: Don't read rgrp blocks twice

[Bob Peterson]

----- Original Message -----
| When savemeta iterates over the rgrps and saves the rgrp header blocks
| it calls save_block which does a bread() for each block despite them
| already being read in earlier. Instead, call save_bh() on the existing
| rgrp buffers. This isn't likely to give a significant performance
| improvement as the duplicate reads would be cache hits but it should
| have a noticeable effect on very large file systems.
| 
| Signed-off-by: Andrew Price <anprice@redhat.com>
| ---

Hi,

IIRC, the reason I coded it the way I did was with the notion that
customers often save their metadata (and send it in) because it's corrupt.
If there's an rgrp block, such as a bitmap, which is corrupt, I wanted
savemeta to save the rgrp blocks regardless of the corruption so we could
see the trash with which it was overwritten. Function gfs2_rgrp_read
checks for corruption and if's not a bitmap, it frees ALL bi_bh buffers;
even the non-corrupt ones.

So we need to ensure that corrupt / blasted rgrps and bitmaps also get
saved, provided their rindex is healthy (which it also might not be).

Perhaps my fears are unfounded? You could verify this by doing:
(1) mkfs.gfs2
(2) blast / zero out a random rgrp or rindex
(3) gfs2_edit savemeta
(4) gfs2_edit printsavedmeta
(5) ensure the output contains the blasted block

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 3/5] gfs2_edit savemeta: Follow lf_next

[Bob Peterson]

----- Original Message -----
| Previously, gfs2_edit savemeta did not walk down the lf_next chain to
| save the directory leaf blocks that might be there. Add a
| save_leaf_chain() function that does that.
| 
| Related: rhbz#1382087
| 
| Signed-off-by: Andrew Price <anprice@redhat.com>
| ---
Hi,

ACK

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 4/5] gfs2_edit: Fix unaligned access in restore_init()

[Bob Peterson]

----- Original Message -----
| Scanning this buffer byte-by-byte using casts results in a quick bus
| error on sparc64. Avoid this by using a memcpy. This function only runs
| once at the beginning of a restoremeta and in most cases the loop will
| only execute once so this adds very little overhead.
| 
| Reported-By: Valentin Vidic <Valentin.Vidic@CARNet.hr>
| Signed-off-by: Andrew Price <anprice@redhat.com>
| ---

Hi,

ACK

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 5/5] gfs2_edit: Fix unaligned accesses due to saved_metablock size

[Bob Peterson]

----- Original Message -----
| Each saved block is prefixed by a saved_metablock structure that holds
| its block number and data length. Unfortunately this structure is 10
| bytes in size, which means that storing the metadata header in memory
| immediately following it causes accesses to the header fields to be
| unaligned. This causes testsuite failures on sparc64. Instead, on
| restore, use a separate buffer for the block data to ensure aligned
| accesses to those fields.
| 
| Reported-By: Valentin Vidic <Valentin.Vidic@CARNet.hr>
| Signed-off-by: Andrew Price <anprice@redhat.com>
| ---

Hi,

ACK

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH 2/5] gfs2_edit savemeta: Don't read rgrp blocks twice

[Andrew Price]

Hi Bob,

On 12/01/17 15:49, Bob Peterson wrote:
> ----- Original Message -----
> | When savemeta iterates over the rgrps and saves the rgrp header blocks
> | it calls save_block which does a bread() for each block despite them
> | already being read in earlier. Instead, call save_bh() on the existing
> | rgrp buffers. This isn't likely to give a significant performance
> | improvement as the duplicate reads would be cache hits but it should
> | have a noticeable effect on very large file systems.
> |
> | Signed-off-by: Andrew Price <anprice@redhat.com>
> | ---
>
> Hi,
>
> IIRC, the reason I coded it the way I did was with the notion that
> customers often save their metadata (and send it in) because it's corrupt.
> If there's an rgrp block, such as a bitmap, which is corrupt, I wanted
> savemeta to save the rgrp blocks regardless of the corruption so we could
> see the trash with which it was overwritten. Function gfs2_rgrp_read
> checks for corruption and if's not a bitmap, it frees ALL bi_bh buffers;
> even the non-corrupt ones.
>
> So we need to ensure that corrupt / blasted rgrps and bitmaps also get
> saved, provided their rindex is healthy (which it also might not be).
>
> Perhaps my fears are unfounded? You could verify this by doing:
> (1) mkfs.gfs2
> (2) blast / zero out a random rgrp or rindex
> (3) gfs2_edit savemeta
> (4) gfs2_edit printsavedmeta
> (5) ensure the output contains the blasted block

You're right to bring it up but it seems that it has been broken since 
before this patch set. There's a metadata header check in 
gfs2_rgrp_read() that stops the zeroed rgrp headers from being saved and 
also save_block() calls get_gfs_struct_info() which checks for the magic 
number.

The patch below fixes those two snags by special casing rgrp header 
blocks but it's messy. I think I can do better by avoiding 
gfs2_rgrp_read() altogether and just reading the rgrp headers without 
the metadata checks, so I'll try again.

Andy

diff --git a/gfs2/edit/hexedit.c b/gfs2/edit/hexedit.c
index cd3f084..233e0e4 100644
--- a/gfs2/edit/hexedit.c
+++ b/gfs2/edit/hexedit.c
@@ -409,7 +409,7 @@ int display_block_type(struct gfs2_buffer_head *dbh, 
int from_restore)

  		rgd = gfs2_blk2rgrpd(&sbd, block);
  		if (rgd) {
-			gfs2_rgrp_read(&sbd, rgd);
+			gfs2_rgrp_read(&sbd, rgd, 1);
  			if ((be32_to_cpu(mh->mh_type) == GFS2_METATYPE_RG) ||
  			    (be32_to_cpu(mh->mh_type) == GFS2_METATYPE_RB))
  				type = 4;
@@ -1325,7 +1325,7 @@ static uint64_t find_metablockoftype_rg(uint64_t 
startblk, int metatype, int pri
  	}
  	for (; !found && next; next = osi_next(next)){
  		rgd = (struct rgrp_tree *)next;
-		errblk = gfs2_rgrp_read(&sbd, rgd);
+		errblk = gfs2_rgrp_read(&sbd, rgd, 1);
  		if (errblk)
  			continue;

@@ -1767,7 +1767,7 @@ static void find_change_block_alloc(int *newval)
  	else {
  		rgd = gfs2_blk2rgrpd(&sbd, ablock);
  		if (rgd) {
-			gfs2_rgrp_read(&sbd, rgd);
+			gfs2_rgrp_read(&sbd, rgd, 1);
  			if (newval) {
  				if (gfs2_set_bitmap(rgd, ablock, *newval))
  					printf("-1 (block invalid or part of an rgrp).\n");
@@ -2328,7 +2328,7 @@ static void rg_repair(void)
  	/* Walk through the resource groups saving everything within */
  	for (n = osi_first(&sbd.rgtree); n; n = osi_next(n)) {
  		rgd = (struct rgrp_tree *)n;
-		if (gfs2_rgrp_read(&sbd, rgd) == 0) { /* was read in okay */
+		if (gfs2_rgrp_read(&sbd, rgd, 1) == 0) { /* was read in okay */
  			gfs2_rgrp_relse(rgd);
  			continue; /* ignore it */
  		}
diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index f6b7ff0..846e964 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -417,7 +417,8 @@ static int save_bh(struct metafd *mfd, struct 
gfs2_buffer_head *savebh, uint64_t
  	   because we want to know if the source inode is a system inode
  	   not the block within the inode "blk". They may or may not
  	   be the same thing. */
-	if (get_gfs_struct_info(savebh, owner, blktype, &blklen) && 
!block_is_systemfile(owner))
+	if (get_gfs_struct_info(savebh, owner, blktype, &blklen) &&
+	    !block_is_systemfile(owner) && owner != 0)
  		return 0; /* Not metadata, and not system file, so skip it */

  	/* No need to save trailing zeroes */
@@ -881,7 +882,7 @@ void savemeta(char *out_fn, int saveoption, int 
gziplevel)
  		unsigned i;

  		rgd = (struct rgrp_tree *)n;
-		if (gfs2_rgrp_read(&sbd, rgd))
+		if (gfs2_rgrp_read(&sbd, rgd, 0))
  			continue;
  		log_debug("RG at %lld (0x%llx) is %u long\n",
  			  (unsigned long long)rgd->ri.ri_addr,
diff --git a/gfs2/fsck/rgrepair.c b/gfs2/fsck/rgrepair.c
index 55057ce..6b3477c 100644
--- a/gfs2/fsck/rgrepair.c
+++ b/gfs2/fsck/rgrepair.c
@@ -1209,7 +1209,7 @@ int rg_repair(struct gfs2_sbd *sdp, int trust_lvl, 
int *rg_count, int *sane)
  		i = 0;
  		do {
  			rgd = (struct rgrp_tree *)n;
-			errblock = gfs2_rgrp_read(sdp, rgd);
+			errblock = gfs2_rgrp_read(sdp, rgd, 1);
  			if (errblock) {
  				if (errblock == prev_err)
  					break;
diff --git a/gfs2/libgfs2/fs_ops.c b/gfs2/libgfs2/fs_ops.c
index 3acfb67..9b5f461 100644
--- a/gfs2/libgfs2/fs_ops.c
+++ b/gfs2/libgfs2/fs_ops.c
@@ -186,7 +186,7 @@ static int block_alloc(struct gfs2_sbd *sdp, const 
uint64_t blksreq, int state,
  		return -1;

  	if (rgt->bits[0].bi_bh == NULL) {
-		if (gfs2_rgrp_read(sdp, rgt))
+		if (gfs2_rgrp_read(sdp, rgt, 1))
  			return -1;
  		release = 1;
  	}
diff --git a/gfs2/libgfs2/lang.c b/gfs2/libgfs2/lang.c
index 2f93c56..4f0bb13 100644
--- a/gfs2/libgfs2/lang.c
+++ b/gfs2/libgfs2/lang.c
@@ -374,7 +374,7 @@ static int ast_get_bitstate(uint64_t bn, struct 
gfs2_sbd *sbd)
  		return -1;
  	}

-	ret = gfs2_rgrp_read(sbd, rgd);
+	ret = gfs2_rgrp_read(sbd, rgd, 1);
  	if (ret != 0) {
  		fprintf(stderr, "Failed to read resource group for block %"PRIu64": 
%d\n", bn, ret);
  		return -1;
diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
index ebf6bca..c260df3 100644
--- a/gfs2/libgfs2/libgfs2.h
+++ b/gfs2/libgfs2/libgfs2.h
@@ -667,7 +667,7 @@ extern int clean_journal(struct gfs2_inode *ip, 
struct gfs2_log_header *head);
  /* rgrp.c */
  extern int gfs2_compute_bitstructs(const uint32_t bsize, struct 
rgrp_tree *rgd);
  extern struct rgrp_tree *gfs2_blk2rgrpd(struct gfs2_sbd *sdp, uint64_t 
blk);
-extern uint64_t gfs2_rgrp_read(struct gfs2_sbd *sdp, struct rgrp_tree 
*rgd);
+extern uint64_t gfs2_rgrp_read(struct gfs2_sbd *sdp, struct rgrp_tree 
*rgd, int checkmeta);
  extern void gfs2_rgrp_relse(struct rgrp_tree *rgd);
  extern struct rgrp_tree *rgrp_insert(struct osi_root *rgtree,
  				     uint64_t rgblock);
diff --git a/gfs2/libgfs2/rgrp.c b/gfs2/libgfs2/rgrp.c
index 7066a5c..52a6895 100644
--- a/gfs2/libgfs2/rgrp.c
+++ b/gfs2/libgfs2/rgrp.c
@@ -153,7 +153,7 @@ void lgfs2_rgrp_bitbuf_free(lgfs2_rgrp_t rg)
   * @rgd - resource group structure
   * returns: 0 if no error, otherwise the block number that failed
   */
-uint64_t gfs2_rgrp_read(struct gfs2_sbd *sdp, struct rgrp_tree *rgd)
+uint64_t gfs2_rgrp_read(struct gfs2_sbd *sdp, struct rgrp_tree *rgd, 
int checkmeta)
  {
  	unsigned x, length = rgd->ri.ri_length;
  	struct gfs2_buffer_head **bhs;
@@ -175,7 +175,7 @@ uint64_t gfs2_rgrp_read(struct gfs2_sbd *sdp, struct 
rgrp_tree *rgd)
  		int mtype = (x ? GFS2_METATYPE_RB : GFS2_METATYPE_RG);

  		bi->bi_bh = bhs[x];
-		if (gfs2_check_meta(bi->bi_bh, mtype)) {
+		if (checkmeta && gfs2_check_meta(bi->bi_bh, mtype)) {
  			unsigned err = x;
  			do {
  				brelse(rgd->bits[x].bi_bh);
diff --git a/gfs2/libgfs2/super.c b/gfs2/libgfs2/super.c
index 81834b3..72ad639 100644
--- a/gfs2/libgfs2/super.c
+++ b/gfs2/libgfs2/super.c
@@ -268,7 +268,7 @@ static int __ri_update(struct gfs2_sbd *sdp, int fd, 
int *rgcount, int *sane,
  		if (ra_window < RA_WINDOW/2)
  			ra_window = gfs2_rgrp_reada(sdp, ra_window, n);
  		/* Read resource group header */
-		errblock = gfs2_rgrp_read(sdp, rgd);
+		errblock = gfs2_rgrp_read(sdp, rgd, 1);
  		if (errblock)
  			return errblock;
  		ra_window--;

[Cluster-devel] [PATCH 2/5] gfs2_edit savemeta: Don't read rgrp blocks twice

[Bob Peterson]

----- Original Message -----
| Hi Bob,
| 
| On 12/01/17 15:49, Bob Peterson wrote:
| > ----- Original Message -----
| > | When savemeta iterates over the rgrps and saves the rgrp header blocks
| > | it calls save_block which does a bread() for each block despite them
| > | already being read in earlier. Instead, call save_bh() on the existing
| > | rgrp buffers. This isn't likely to give a significant performance
| > | improvement as the duplicate reads would be cache hits but it should
| > | have a noticeable effect on very large file systems.
| > |
| > | Signed-off-by: Andrew Price <anprice@redhat.com>
| > | ---
| >
| > Hi,
| >
| > IIRC, the reason I coded it the way I did was with the notion that
| > customers often save their metadata (and send it in) because it's corrupt.
| > If there's an rgrp block, such as a bitmap, which is corrupt, I wanted
| > savemeta to save the rgrp blocks regardless of the corruption so we could
| > see the trash with which it was overwritten. Function gfs2_rgrp_read
| > checks for corruption and if's not a bitmap, it frees ALL bi_bh buffers;
| > even the non-corrupt ones.
| >
| > So we need to ensure that corrupt / blasted rgrps and bitmaps also get
| > saved, provided their rindex is healthy (which it also might not be).
| >
| > Perhaps my fears are unfounded? You could verify this by doing:
| > (1) mkfs.gfs2
| > (2) blast / zero out a random rgrp or rindex
| > (3) gfs2_edit savemeta
| > (4) gfs2_edit printsavedmeta
| > (5) ensure the output contains the blasted block
| 
| You're right to bring it up but it seems that it has been broken since
| before this patch set. There's a metadata header check in
| gfs2_rgrp_read() that stops the zeroed rgrp headers from being saved and
| also save_block() calls get_gfs_struct_info() which checks for the magic
| number.
| 
| The patch below fixes those two snags by special casing rgrp header
| blocks but it's messy. I think I can do better by avoiding
| gfs2_rgrp_read() altogether and just reading the rgrp headers without
| the metadata checks, so I'll try again.
| 
| Andy
(snip)
Hi Andy,

ACK

This new patch looks good. Thanks for cleaning this up.

Regards,

Bob Peterson
Red Hat File Systems