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 <anpr...@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--;

Reply via email to