From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Price Date: Wed, 15 Apr 2015 16:23:59 +0100 Subject: [Cluster-devel] [GFS2] fsck.gfs2: replace recent i_goal fixes with simple logic In-Reply-To: <1429105068-6052-1-git-send-email-adas@redhat.com> References: <1429105068-6052-1-git-send-email-adas@redhat.com> Message-ID: <552E828F.1060105@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 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 >