From mboxrd@z Thu Jan 1 00:00:00 1970 From: Abhijith Das Date: Thu, 15 Jan 2015 13:08:04 -0500 (EST) Subject: [Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Reprocess nodes if anything changed In-Reply-To: <1398128522.10066130.1421340921225.JavaMail.zimbra@redhat.com> References: <1398128522.10066130.1421340921225.JavaMail.zimbra@redhat.com> Message-ID: <1768884218.10132371.1421345284338.JavaMail.zimbra@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, ACK. Cheers! --Abhi ----- Original Message ----- > From: "Bob Peterson" > To: "cluster-devel" > Sent: Thursday, January 15, 2015 10:55:21 AM > Subject: [Cluster-devel] [fsck.gfs2 PATCH] fsck.gfs2: Reprocess nodes if anything changed > > Hi, > > Before this patch, fsck.gfs2 called "reprocess_inode" in several > places, especially when the number of blocks had changed from the > original. That works in almost all cases. However, there's a corner > case where a block may be deleted, due to errors (for example, a > bad directory leaf), and another block takes its place. In that > case the count of the number of blocks is still the same, but the > inode should be reprocessed anyway, because the deleted blocks > and replacement blocks may be at different locations or maybe of > different types. A hash table block may be "data" when it's freed, > but if the block is reused, it may be repurposed as a "leaf" block. > That may confuse subsequent processing. Another reason to call > reprocess_inode is when the goal block changes to a value outside > the resource group, due to block allocations for repairs. > > Regards, > > Bob Peterson > Red Hat File Systems > > Signed-off-by: Bob Peterson > --- > diff --git a/gfs2/fsck/lost_n_found.c b/gfs2/fsck/lost_n_found.c > index 9672c7a..b2e35e2 100644 > --- a/gfs2/fsck/lost_n_found.c > +++ b/gfs2/fsck/lost_n_found.c > @@ -174,7 +174,6 @@ void make_sure_lf_exists(struct gfs2_inode *ip) > int add_inode_to_lf(struct gfs2_inode *ip){ > char tmp_name[256]; > __be32 inode_type; > - uint64_t lf_blocks; > struct gfs2_sbd *sdp = ip->i_sbd; > int err = 0; > uint32_t mode; > @@ -184,7 +183,6 @@ int add_inode_to_lf(struct gfs2_inode *ip){ > log_err( _("Trying to add lost+found to itself...skipping")); > return 0; > } > - lf_blocks = lf_dip->i_di.di_blocks; > > if (sdp->gfs1) > mode = gfs_to_gfs2_mode(ip); > @@ -242,10 +240,6 @@ int add_inode_to_lf(struct gfs2_inode *ip){ > tmp_name, strerror(errno)); > exit(FSCK_ERROR); > } > - /* If the lf directory had new blocks added we have to mark them > - properly in the bitmap so they're not freed. */ > - if (lf_dip->i_di.di_blocks != lf_blocks) > - reprocess_inode(lf_dip, "lost+found"); > > /* This inode is linked from lost+found */ > incr_link_count(ip->i_di.di_num, lf_dip, _("from lost+found")); > diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c > index 16faafb..217bb07 100644 > --- a/gfs2/fsck/metawalk.c > +++ b/gfs2/fsck/metawalk.c > @@ -1693,11 +1693,11 @@ int check_dir(struct gfs2_sbd *sdp, uint64_t block, > struct metawalk_fxns *pass) > { > struct gfs2_inode *ip; > int error = 0; > - uint64_t cur_blks; > + struct alloc_state as; > > ip = fsck_load_inode(sdp, block); > > - cur_blks = ip->i_di.di_blocks; > + astate_save(ip, &as); > > if (ip->i_di.di_flags & GFS2_DIF_EXHASH) > error = check_leaf_blks(ip, pass); > @@ -1707,7 +1707,7 @@ int check_dir(struct gfs2_sbd *sdp, uint64_t block, > struct metawalk_fxns *pass) > if (error < 0) > stack; > > - if (ip->i_di.di_blocks != cur_blks) > + if (astate_changed(ip, &as)) > reprocess_inode(ip, _("Current")); > > fsck_inode_put(&ip); /* does a brelse */ > diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c > index 4ea322a..74bcc26 100644 > --- a/gfs2/fsck/pass2.c > +++ b/gfs2/fsck/pass2.c > @@ -1723,7 +1723,8 @@ build_it: > static int check_system_dir(struct gfs2_inode *sysinode, const char > *dirname, > int builder(struct gfs2_sbd *sdp)) > { > - uint64_t iblock = 0, cur_blks; > + uint64_t iblock = 0; > + struct alloc_state as; > struct dir_status ds = {0}; > int error = 0; > > @@ -1740,14 +1741,14 @@ static int check_system_dir(struct gfs2_inode > *sysinode, const char *dirname, > > pass2_fxns.private = (void *) &ds; > if (ds.q == gfs2_bad_block) { > - cur_blks = sysinode->i_di.di_blocks; > + astate_save(sysinode, &as); > /* First check that the directory's metatree is valid */ > error = check_metatree(sysinode, &pass2_fxns); > if (error < 0) { > stack; > return error; > } > - if (sysinode->i_di.di_blocks != cur_blks) > + if (astate_changed(sysinode, &as)) > reprocess_inode(sysinode, _("System inode")); > } > error = check_dir(sysinode->i_sbd, iblock, &pass2_fxns); > @@ -1768,7 +1769,7 @@ static int check_system_dir(struct gfs2_inode > *sysinode, const char *dirname, > if (!ds.dotdir) { > log_err( _("No '.' entry found for %s directory.\n"), dirname); > if (query( _("Is it okay to add '.' entry? (y/n) "))) { > - cur_blks = sysinode->i_di.di_blocks; > + astate_save(sysinode, &as); > log_warn( _("Adding '.' entry\n")); > error = dir_add(sysinode, ".", 1, &(sysinode->i_di.di_num), > (sysinode->i_sbd->gfs1 ? GFS_FILE_DIR : DT_DIR)); > @@ -1777,7 +1778,7 @@ static int check_system_dir(struct gfs2_inode > *sysinode, const char *dirname, > strerror(errno)); > return -errno; > } > - if (cur_blks != sysinode->i_di.di_blocks) > + if (astate_changed(sysinode, &as)) > reprocess_inode(sysinode, dirname); > /* This system inode is linked to itself via '.' */ > incr_link_count(sysinode->i_di.di_num, sysinode, > @@ -1859,7 +1860,8 @@ static inline int is_system_dir(struct gfs2_sbd *sdp, > uint64_t block) > */ > int pass2(struct gfs2_sbd *sdp) > { > - uint64_t dirblk, cur_blks; > + uint64_t dirblk; > + struct alloc_state as; > uint8_t q; > struct dir_status ds = {0}; > struct gfs2_inode *ip; > @@ -1926,14 +1928,14 @@ int pass2(struct gfs2_sbd *sdp) > /* First check that the directory's metatree > * is valid */ > ip = fsck_load_inode(sdp, dirblk); > - cur_blks = ip->i_di.di_blocks; > + astate_save(ip, &as); > error = check_metatree(ip, &pass2_fxns); > if (error < 0) { > stack; > fsck_inode_put(&ip); > return error; > } > - if (ip->i_di.di_blocks != cur_blks) > + if (astate_changed(ip, &as)) > reprocess_inode(ip, "current"); > fsck_inode_put(&ip); > } > @@ -1994,7 +1996,7 @@ int pass2(struct gfs2_sbd *sdp) > (unsigned long long)dirblk); > > if (query( _("Is it okay to add '.' entry? (y/n) "))) { > - cur_blks = ip->i_di.di_blocks; > + astate_save(ip, &as); > error = dir_add(ip, ".", 1, &(ip->i_di.di_num), > (sdp->gfs1 ? GFS_FILE_DIR : DT_DIR)); > if (error) { > @@ -2003,7 +2005,7 @@ int pass2(struct gfs2_sbd *sdp) > fsck_inode_put(&ip); > return -errno; > } > - if (cur_blks != ip->i_di.di_blocks) { > + if (astate_changed(ip, &as)) { > char dirname[80]; > > sprintf(dirname, _("Directory at %lld " > diff --git a/gfs2/fsck/pass3.c b/gfs2/fsck/pass3.c > index 33865df..3e183e5 100644 > --- a/gfs2/fsck/pass3.c > +++ b/gfs2/fsck/pass3.c > @@ -24,7 +24,7 @@ static int attach_dotdot_to(struct gfs2_sbd *sdp, uint64_t > newdotdot, > int filename_len = 2; > int err; > struct gfs2_inode *ip, *pip; > - uint64_t cur_blks; > + struct alloc_state as; > > ip = fsck_load_inode(sdp, block); > pip = fsck_load_inode(sdp, newdotdot); > @@ -39,7 +39,7 @@ static int attach_dotdot_to(struct gfs2_sbd *sdp, uint64_t > newdotdot, > log_warn( _("Unable to remove \"..\" directory entry.\n")); > else > decr_link_count(olddotdot, block, _("old \"..\"")); > - cur_blks = ip->i_di.di_blocks; > + astate_save(ip, &as); > err = dir_add(ip, filename, filename_len, &pip->i_di.di_num, > (sdp->gfs1 ? GFS_FILE_DIR : DT_DIR)); > if (err) { > @@ -47,7 +47,7 @@ static int attach_dotdot_to(struct gfs2_sbd *sdp, uint64_t > newdotdot, > filename, strerror(errno)); > exit(FSCK_ERROR); > } > - if (cur_blks != ip->i_di.di_blocks) { > + if (astate_changed(ip, &as)) { > char dirname[80]; > > sprintf(dirname, _("Directory at %lld (0x%llx)"), > @@ -179,6 +179,7 @@ int pass3(struct gfs2_sbd *sdp) > struct dir_info *di, *tdi; > struct gfs2_inode *ip; > uint8_t q; > + struct alloc_state lf_as = {.as_blocks = 0, .as_meta_goal = 0}; > > di = dirtree_find(sdp->md.rooti->i_di.di_num.no_addr); > if (di) { > @@ -225,6 +226,8 @@ int pass3(struct gfs2_sbd *sdp) > */ > log_info( _("Checking directory linkage.\n")); > for (tmp = osi_first(&dirtree); tmp; tmp = next) { > + if (lf_dip && lf_as.as_blocks == 0) > + astate_save(lf_dip, &lf_as); > next = osi_next(tmp); > di = (struct dir_info *)tmp; > while (!di->checked) { > @@ -330,8 +333,14 @@ int pass3(struct gfs2_sbd *sdp) > break; > } > } > - if (lf_dip) > + if (lf_dip) { > + /* If the lf directory had new blocks added we have to mark > + them properly in the blockmap so they're not freed. */ > + if (astate_changed(lf_dip, &lf_as)) > + reprocess_inode(lf_dip, "lost+found"); > + > log_debug( _("At end of pass3, lost+found entries is %u\n"), > lf_dip->i_di.di_entries); > + } > return FSCK_OK; > } > diff --git a/gfs2/fsck/pass4.c b/gfs2/fsck/pass4.c > index f9d08dc..324ea9f 100644 > --- a/gfs2/fsck/pass4.c > +++ b/gfs2/fsck/pass4.c > @@ -48,12 +48,15 @@ static int scan_inode_list(struct gfs2_sbd *sdp) { > struct gfs2_inode *ip; > int lf_addition = 0; > uint8_t q; > + struct alloc_state lf_as = {.as_blocks = 0, .as_meta_goal = 0}; > > /* FIXME: should probably factor this out into a generic > * scanning fxn */ > for (tmp = osi_first(&inodetree); tmp; tmp = next) { > if (skip_this_pass || fsck_abort) /* if asked to skip the rest */ > return 0; > + if (lf_dip && lf_as.as_blocks == 0) > + astate_save(lf_dip, &lf_as); > next = osi_next(tmp); > ii = (struct inode_info *)tmp; > /* Don't check reference counts on the special gfs files */ > @@ -179,6 +182,9 @@ static int scan_inode_list(struct gfs2_sbd *sdp) { > (unsigned long long)ii->di_num.no_addr, ii->di_nlink); > } /* osi_list_foreach(tmp, list) */ > > + if (lf_dip && astate_changed(lf_dip, &lf_as)) > + reprocess_inode(lf_dip, "lost+found"); > + > if (lf_addition) { > if (!(ii = inodetree_find(lf_dip->i_di.di_num.no_addr))) { > log_crit( _("Unable to find lost+found inode in inode_hash!!\n")); > diff --git a/gfs2/fsck/util.h b/gfs2/fsck/util.h > index 276c726..66b9c7a 100644 > --- a/gfs2/fsck/util.h > +++ b/gfs2/fsck/util.h > @@ -12,6 +12,11 @@ > #define INODE_VALID 1 > #define INODE_INVALID 0 > > +struct alloc_state { > + uint64_t as_blocks; > + uint64_t as_meta_goal; > +}; > + > struct di_info *search_list(osi_list_t *list, uint64_t addr); > void big_file_comfort(struct gfs2_inode *ip, uint64_t blks_checked); > void warm_fuzzy_stuff(uint64_t block); > @@ -27,6 +32,21 @@ extern const char *reftypes[ref_types + 1]; > #define BLOCKMAP_BYTE_OFFSET4(x) (((x) & 0x0000000000000001) << 2) > #define BLOCKMAP_MASK4 (0xf) > > +static inline void astate_save(struct gfs2_inode *ip, struct alloc_state > *as) > +{ > + as->as_blocks = ip->i_di.di_blocks; > + as->as_meta_goal = ip->i_di.di_goal_meta; > +} > + > +static inline int astate_changed(struct gfs2_inode *ip, struct alloc_state > *as) > +{ > + if (as->as_blocks != ip->i_di.di_blocks) > + return 1; > + if (as->as_meta_goal != ip->i_di.di_goal_meta) > + return 1; > + return 0; > +} > + > static inline uint8_t block_type(uint64_t bblock) > { > static unsigned char *byte; > >