Hi Abhi,

On 15/04/15 14:37, Abhi Das wrote:
This patch reverses the recent set of i_goal fixes for fsck.gfs2.
This is because of two problems.
1. It is not possible to determine if a valid block within the fs
is the correct goal block for a given inode.
2. Conversely, given an inode, it is also not possible to accurately
determine what its goal block should be.

The previous patches assumed that the last block of a file is its
goal block, but that is not true if the file is a directory or if
its blocks are not allocated sequentially. fsck.gfs2 would flag
these inodes incorrectly as having bad i_goal values.

This patch takes a simple approach. It checks if the i_goal of a
given inode is out of bounds of the fs. If so, we can be certain
that it is wrong and we set it to the inode metadata block. This
is a safe starting point for gfs2 to determine where to allocate
from next.

Resolves: rhbz#1186515
Signed-off-by: Abhi Das <[email protected]>

This looks good to me, and it fixes the test failures that I was seeing due to the non-sequential last block issue.

Thanks,
Andy

---
  gfs2/fsck/metawalk.c   | 92 +++-----------------------------------------------
  gfs2/fsck/metawalk.h   |  5 ---
  gfs2/fsck/pass1.c      | 35 ++++++++++++++++---
  gfs2/libgfs2/libgfs2.h |  1 -
  4 files changed, 34 insertions(+), 99 deletions(-)

diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index f05fb51..4d5a660 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1428,8 +1428,7 @@ static int build_and_check_metalist(struct gfs2_inode 
*ip, osi_list_t *mlp,
   */
  static int check_data(struct gfs2_inode *ip, struct metawalk_fxns *pass,
                      struct gfs2_buffer_head *bh, int head_size,
-                     uint64_t *last_block, uint64_t *blks_checked,
-                     uint64_t *error_blk)
+                     uint64_t *blks_checked, uint64_t *error_blk)
  {
        int error = 0, rc = 0;
        uint64_t block, *ptr;
@@ -1444,7 +1443,7 @@ static int check_data(struct gfs2_inode *ip, struct 
metawalk_fxns *pass,

                if (skip_this_pass || fsck_abort)
                        return error;
-               *last_block = block =  be64_to_cpu(*ptr);
+               block =  be64_to_cpu(*ptr);
                /* It's important that we don't call valid_block() and
                   bypass calling check_data on invalid blocks because that
                   would defeat the rangecheck_block related functions in
@@ -1548,15 +1547,12 @@ int check_metatree(struct gfs2_inode *ip, struct 
metawalk_fxns *pass)
        struct gfs2_buffer_head *bh;
        uint32_t height = ip->i_di.di_height;
        int  i, head_size;
-       uint64_t blks_checked = 0, last_block = 0;
+       uint64_t blks_checked = 0;
        int error, rc;
        int metadata_clean = 0;
        uint64_t error_blk = 0;
        int hit_error_blk = 0;

-       if (!height && pass->check_i_goal)
-               pass->check_i_goal(ip, ip->i_di.di_num.no_addr,
-                                  pass->private);
        if (!height && !is_dir(&ip->i_di, ip->i_sbd->gfs1))
                return 0;

@@ -1575,9 +1571,6 @@ int check_metatree(struct gfs2_inode *ip, struct 
metawalk_fxns *pass)
         * comprise the directory hash table, so we perform the directory
         * checks and exit. */
          if (is_dir(&ip->i_di, ip->i_sbd->gfs1)) {
-               last_block = ip->i_di.di_num.no_addr;
-               if (pass->check_i_goal)
-                       pass->check_i_goal(ip, last_block, pass->private);
                if (!(ip->i_di.di_flags & GFS2_DIF_EXHASH))
                        goto out;
                /* check validity of leaf blocks and leaf chains */
@@ -1604,7 +1597,7 @@ int check_metatree(struct gfs2_inode *ip, struct 
metawalk_fxns *pass)

                if (pass->check_data)
                        error = check_data(ip, pass, bh, head_size,
-                                          &last_block, &blks_checked, 
&error_blk);
+                                          &blks_checked, &error_blk);
                if (pass->big_file_msg && ip->i_di.di_blocks > COMFORTABLE_BLKS)
                        pass->big_file_msg(ip, blks_checked);
        }
@@ -1616,8 +1609,6 @@ int check_metatree(struct gfs2_inode *ip, struct 
metawalk_fxns *pass)
                            (unsigned long long)ip->i_di.di_num.no_addr);
                fflush(stdout);
        }
-       if (!error && pass->check_i_goal)
-               pass->check_i_goal(ip, last_block, pass->private);
  undo_metalist:
        if (!error)
                goto out;
@@ -1958,80 +1949,6 @@ static int alloc_leaf(struct gfs2_inode *ip, uint64_t 
block, void *private)
        return 0;
  }

-/**
- * rgrp_contains_block - Check if the rgrp provided contains the
- * given block. Taken directly from the gfs2 kernel code
- * @rgd: The rgrp to search within
- * @block: The block to search for
- *
- * Returns: 1 if present, 0 if not.
- */
-static inline int rgrp_contains_block(struct rgrp_tree *rgd, uint64_t block)
-{
-       uint64_t first = rgd->ri.ri_data0;
-       uint64_t last = first + rgd->ri.ri_data;
-       return first <= block && block < last;
-}
-
-/**
- * check_i_goal
- * @ip
- * @goal_blk: What the goal block should be for this inode
- *
- * The goal block for a regular file is typically the last
- * data block of the file. If we can't get the right value,
- * the inode metadata block is the next best thing.
- *
- * Returns: 0 if corrected, 1 if not corrected
- */
-int check_i_goal(struct gfs2_inode *ip, uint64_t goal_blk,
-                       void *private)
-{
-       struct gfs2_sbd *sdp = ip->i_sbd;
-       uint64_t i_block = ip->i_di.di_num.no_addr;
-
-       /* Don't fix gfs1 inodes, system inodes or inodes whose goal blocks are
-        * set to the inode blocks themselves. */
-       if (sdp->gfs1 || ip->i_di.di_flags & GFS2_DIF_SYSTEM ||
-               ip->i_di.di_goal_meta == i_block)
-               return 0;
-       /* Don't fix directory goal blocks unless we know they're wrong.
-        * i.e. out of bounds of the fs. Directories can easily have blocks
-        * outside of the dinode's rgrp and thus we have no way of knowing
-        * if the goal block is bogus or not. */
-       if (is_dir(&ip->i_di, ip->i_sbd->gfs1) &&
-           (ip->i_di.di_goal_meta > sdp->sb_addr &&
-            ip->i_di.di_goal_meta <= sdp->fssize))
-               return 0;
-       /* We default to the inode block */
-       if (!goal_blk)
-               goal_blk = i_block;
-
-       if (ip->i_di.di_goal_meta != goal_blk) {
-               /* If the existing goal block is in the same rgrp as the inode,
-                * we give the benefit of doubt and assume the value is correct 
*/
-               if (ip->i_rgd &&
-                   rgrp_contains_block(ip->i_rgd, ip->i_di.di_goal_meta))
-                       goto skip;
-               log_err( _("Error: inode %llu (0x%llx) has invalid "
-                          "allocation goal block %llu (0x%llx). Should"
-                          " be %llu (0x%llx)\n"),
-                        (unsigned long long)i_block, (unsigned long 
long)i_block,
-                        (unsigned long long)ip->i_di.di_goal_meta,
-                        (unsigned long long)ip->i_di.di_goal_meta,
-                        (unsigned long long)goal_blk, (unsigned long 
long)goal_blk);
-               if (query( _("Fix the invalid goal block? (y/n) "))) {
-                       ip->i_di.di_goal_meta = ip->i_di.di_goal_data = 
goal_blk;
-                       bmodified(ip->i_bh);
-               } else {
-                       log_err(_("Invalid goal block not fixed.\n"));
-                       return 1;
-               }
-       }
-skip:
-       return 0;
-}
-
  struct metawalk_fxns alloc_fxns = {
        .private = NULL,
        .check_leaf = alloc_leaf,
@@ -2042,7 +1959,6 @@ struct metawalk_fxns alloc_fxns = {
        .check_dentry = NULL,
        .check_eattr_entry = NULL,
        .check_eattr_extentry = NULL,
-       .check_i_goal = check_i_goal,
        .finish_eattr_indir = NULL,
  };

diff --git a/gfs2/fsck/metawalk.h b/gfs2/fsck/metawalk.h
index 779360e..fa4c850 100644
--- a/gfs2/fsck/metawalk.h
+++ b/gfs2/fsck/metawalk.h
@@ -50,8 +50,6 @@ extern int _fsck_blockmap_set(struct gfs2_inode *ip, uint64_t 
bblock,
                              const char *caller, int line);
  extern int check_n_fix_bitmap(struct gfs2_sbd *sdp, uint64_t blk,
                              int error_on_dinode, int new_blockmap_state);
-extern int check_i_goal(struct gfs2_inode *ip, uint64_t goal_blk,
-                       void *private);
  extern void reprocess_inode(struct gfs2_inode *ip, const char *desc);
  extern struct duptree *dupfind(uint64_t block);
  extern struct gfs2_inode *fsck_system_inode(struct gfs2_sbd *sdp,
@@ -91,7 +89,6 @@ enum meta_check_rc {
   * check_dentry:
   * check_eattr_entry:
   * check_eattr_extentry:
- * check_i_goal:
   */
  struct metawalk_fxns {
        void *private;
@@ -143,8 +140,6 @@ struct metawalk_fxns {
                                     struct gfs2_ea_header *ea_hdr,
                                     struct gfs2_ea_header *ea_hdr_prev,
                                     void *private);
-       int (*check_i_goal) (struct gfs2_inode *ip, uint64_t goal_blk,
-                            void *private);
        int (*finish_eattr_indir) (struct gfs2_inode *ip, int leaf_pointers,
                                   int leaf_pointer_errors, void *private);
        void (*big_file_msg) (struct gfs2_inode *ip, uint64_t blks_checked);
diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c
index 69c88f4..0909873 100644
--- a/gfs2/fsck/pass1.c
+++ b/gfs2/fsck/pass1.c
@@ -100,7 +100,6 @@ struct metawalk_fxns pass1_fxns = {
        .check_dentry = NULL,
        .check_eattr_entry = check_eattr_entries,
        .check_eattr_extentry = check_extended_leaf_eattr,
-       .check_i_goal = check_i_goal,
        .finish_eattr_indir = finish_eattr_indir,
        .big_file_msg = big_file_comfort,
        .repair_leaf = pass1_repair_leaf,
@@ -1205,12 +1204,37 @@ bad_dinode:
        return -1;
  }

+static void check_i_goal(struct gfs2_sbd *sdp, struct gfs2_inode *ip)
+{
+       if (sdp->gfs1 || ip->i_di.di_flags & GFS2_DIF_SYSTEM)
+               return;
+
+       if (ip->i_di.di_goal_meta <= sdp->sb_addr ||
+           ip->i_di.di_goal_meta > sdp->fssize) {
+               log_err(_("Inode #%llu (0x%llx): Bad allocation goal block "
+                         "found: %llu (0x%llx)\n"),
+                       (unsigned long long)ip->i_di.di_num.no_addr,
+                       (unsigned long long)ip->i_di.di_num.no_addr,
+                       (unsigned long long)ip->i_di.di_goal_meta,
+                       (unsigned long long)ip->i_di.di_goal_meta);
+               if (query( _("Fix goal block in inode #%llu (0x%llx)? (y/n) "),
+                          (unsigned long long)ip->i_di.di_num.no_addr,
+                          (unsigned long long)ip->i_di.di_num.no_addr)) {
+                       ip->i_di.di_goal_meta = ip->i_di.di_num.no_addr;
+                       bmodified(ip->i_bh);
+               } else
+                       log_err(_("Allocation goal block in inode #%lld "
+                                 "(0x%llx) not fixed\n"),
+                               (unsigned long long)ip->i_di.di_num.no_addr,
+                               (unsigned long long)ip->i_di.di_num.no_addr);
+       }
+}
+
  /*
   * handle_di - This is now a wrapper function that takes a gfs2_buffer_head
   *             and calls handle_ip, which takes an in-code dinode structure.
   */
-static int handle_di(struct gfs2_sbd *sdp, struct gfs2_buffer_head *bh,
-                    struct rgrp_tree *rgd)
+static int handle_di(struct gfs2_sbd *sdp, struct gfs2_buffer_head *bh)
  {
        int error = 0;
        uint64_t block = bh->b_blocknr;
@@ -1252,7 +1276,7 @@ static int handle_di(struct gfs2_sbd *sdp, struct 
gfs2_buffer_head *bh,
                                 (unsigned long long)block,
                                 (unsigned long long)block);
        }
-       ip->i_rgd = rgd;
+       check_i_goal(sdp, ip);
        error = handle_ip(sdp, ip);
        fsck_inode_put(&ip);
        return error;
@@ -1378,6 +1402,7 @@ static int check_system_inode(struct gfs2_sbd *sdp,
                                          "directory entries.\n"), filename);
                }
        }
+       check_i_goal(sdp, *sysinode);
        error = handle_ip(sdp, *sysinode);
        return error ? error : err;
  }
@@ -1602,7 +1627,7 @@ static int pass1_process_bitmap(struct gfs2_sbd *sdp, 
struct rgrp_tree *rgd, uin
                                 (unsigned long long)block,
                                 (unsigned long long)block);
                        check_n_fix_bitmap(sdp, block, 0, GFS2_BLKST_FREE);
-               } else if (handle_di(sdp, bh, rgd) < 0) {
+               } else if (handle_di(sdp, bh) < 0) {
                        stack;
                        brelse(bh);
                        gfs2_special_free(&gfs1_rindex_blks);
diff --git a/gfs2/libgfs2/libgfs2.h b/gfs2/libgfs2/libgfs2.h
index f1f81d3..ccae721 100644
--- a/gfs2/libgfs2/libgfs2.h
+++ b/gfs2/libgfs2/libgfs2.h
@@ -233,7 +233,6 @@ struct gfs2_inode {
        struct gfs2_dinode i_di;
        struct gfs2_buffer_head *i_bh;
        struct gfs2_sbd *i_sbd;
-       struct rgrp_tree *i_rgd; /* The rgrp this inode is in */
  };

  struct master_dir


Reply via email to