From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Fri, 12 Aug 2011 10:22:48 +0100 Subject: [Cluster-devel] [Patch 09/44] fsck.gfs2: Rename the nlink functions to make them more intuitive In-Reply-To: <689476509.544664.1313096489387.JavaMail.root@zmail06.collab.prod.int.phx2.redhat.com> References: <689476509.544664.1313096489387.JavaMail.root@zmail06.collab.prod.int.phx2.redhat.com> Message-ID: <1313140968.2704.26.camel@menhir> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Hi, Why is the link count only 16 bits? Where is it checked for overflow? Steve. On Thu, 2011-08-11 at 17:01 -0400, Bob Peterson wrote: > >From 55e442c79adec0fa7f6d4e7f6700f14a630d4e3e Mon Sep 17 00:00:00 2001 > From: Bob Peterson > Date: Mon, 8 Aug 2011 14:01:18 -0500 > Subject: [PATCH 09/44] fsck.gfs2: Rename the nlink functions to make them > more intuitive > > Part of fsck's checks is to verify the count of links for directories, > but the variable names and function names are too confusing to understand > without in-depth analysis of what the code is doing. > > This patch renames the structure variable "link_count" to something that > makes more intuitive sense: di_nlink, which matches the variable name in > the dinode. That distinguishes it from the number of links that fsck is > trying to count manually. It also renames the link count functions to > make them more intuitively obvious as well: set_di_nlink sets the di_nlink > based on the value passed in from the dinode, and incr_link_count and > decr_link_count increment and decrement the counted links respectively. > > rhbz#675723 > --- > gfs2/fsck/fsck.h | 4 ++-- > gfs2/fsck/link.c | 16 ++++++++++------ > gfs2/fsck/link.h | 10 +++++----- > gfs2/fsck/lost_n_found.c | 29 ++++++++++++++--------------- > gfs2/fsck/pass1.c | 2 +- > gfs2/fsck/pass2.c | 20 ++++++++++---------- > gfs2/fsck/pass3.c | 4 ++-- > gfs2/fsck/pass4.c | 14 +++++++------- > 8 files changed, 51 insertions(+), 48 deletions(-) > > diff --git a/gfs2/fsck/fsck.h b/gfs2/fsck/fsck.h > index 25bc3b9..6353dfc 100644 > --- a/gfs2/fsck/fsck.h > +++ b/gfs2/fsck/fsck.h > @@ -29,8 +29,8 @@ struct inode_info > { > struct osi_node node; > uint64_t inode; > - uint16_t link_count; /* the number of links the inode > - * thinks it has */ > + uint16_t di_nlink; /* the number of links the inode > + * thinks it has */ > uint16_t counted_links; /* the number of links we've found */ > }; > > diff --git a/gfs2/fsck/link.c b/gfs2/fsck/link.c > index 08ea94c..e49f3af 100644 > --- a/gfs2/fsck/link.c > +++ b/gfs2/fsck/link.c > @@ -13,22 +13,26 @@ > #include "inode_hash.h" > #include "link.h" > > -int set_link_count(uint64_t inode_no, uint32_t count) > +int set_di_nlink(struct gfs2_inode *ip) > { > struct inode_info *ii; > + uint64_t inode_no = ip->i_di.di_num.no_addr; > + > + /*log_debug( _("Setting link count to %u for %" PRIu64 > + " (0x%" PRIx64 ")\n"), count, inode_no, inode_no);*/ > /* If the list has entries, look for one that matches inode_no */ > ii = inodetree_find(inode_no); > if (!ii) > ii = inodetree_insert(inode_no); > if (ii) > - ii->link_count = count; > + ii->di_nlink = ip->i_di.di_nlink; > else > return -1; > return 0; > } > > -int increment_link(uint64_t inode_no, uint64_t referenced_from, > - const char *why) > +int incr_link_count(uint64_t inode_no, uint64_t referenced_from, > + const char *why) > { > struct inode_info *ii = NULL; > > @@ -61,8 +65,8 @@ int increment_link(uint64_t inode_no, uint64_t referenced_from, > return 0; > } > > -int decrement_link(uint64_t inode_no, uint64_t referenced_from, > - const char *why) > +int decr_link_count(uint64_t inode_no, uint64_t referenced_from, > + const char *why) > { > struct inode_info *ii = NULL; > > diff --git a/gfs2/fsck/link.h b/gfs2/fsck/link.h > index f890575..ad040e6 100644 > --- a/gfs2/fsck/link.h > +++ b/gfs2/fsck/link.h > @@ -1,10 +1,10 @@ > #ifndef _LINK_H > #define _LINK_H > > -int set_link_count(uint64_t inode_no, uint32_t count); > -int increment_link(uint64_t inode_no, uint64_t referenced_from, > - const char *why); > -int decrement_link(uint64_t inode_no, uint64_t referenced_from, > - const char *why); > +int set_di_nlink(struct gfs2_inode *ip); > +int incr_link_count(uint64_t inode_no, uint64_t referenced_from, > + const char *why); > +int decr_link_count(uint64_t inode_no, uint64_t referenced_from, > + const char *why); > > #endif /* _LINK_H */ > diff --git a/gfs2/fsck/lost_n_found.c b/gfs2/fsck/lost_n_found.c > index 4eff83b..32f3c5c 100644 > --- a/gfs2/fsck/lost_n_found.c > +++ b/gfs2/fsck/lost_n_found.c > @@ -51,8 +51,7 @@ int add_inode_to_lf(struct gfs2_inode *ip){ > the root directory. We must increment the nlink value > in the hash table to keep them in sync so that pass4 can > detect and fix any descrepancies. */ > - set_link_count(sdp->sd_sb.sb_root_dir.no_addr, > - sdp->md.rooti->i_di.di_nlink); > + set_di_nlink(sdp->md.rooti); > > q = block_type(lf_dip->i_di.di_num.no_addr); > if (q != gfs2_inode_dir) { > @@ -68,15 +67,15 @@ int add_inode_to_lf(struct gfs2_inode *ip){ > _("lost+found dinode"), > gfs2_inode_dir); > /* root inode links to lost+found */ > - increment_link(sdp->md.rooti->i_di.di_num.no_addr, > + incr_link_count(sdp->md.rooti->i_di.di_num.no_addr, > lf_dip->i_di.di_num.no_addr, _("root")); > /* lost+found link for '.' from itself */ > - increment_link(lf_dip->i_di.di_num.no_addr, > - lf_dip->i_di.di_num.no_addr, "\".\""); > + incr_link_count(lf_dip->i_di.di_num.no_addr, > + lf_dip->i_di.di_num.no_addr, "\".\""); > /* lost+found link for '..' back to root */ > - increment_link(lf_dip->i_di.di_num.no_addr, > - sdp->md.rooti->i_di.di_num.no_addr, > - "\"..\""); > + incr_link_count(lf_dip->i_di.di_num.no_addr, > + sdp->md.rooti->i_di.di_num.no_addr, > + "\"..\""); > } > log_info( _("lost+found directory is dinode %lld (0x%llx)\n"), > (unsigned long long)lf_dip->i_di.di_num.no_addr, > @@ -113,9 +112,9 @@ int add_inode_to_lf(struct gfs2_inode *ip){ > (unsigned long long)ip->i_di.di_num.no_addr, > (unsigned long long)di->dotdot_parent, > (unsigned long long)di->dotdot_parent); > - decrement_link(di->dotdot_parent, > - ip->i_di.di_num.no_addr, > - _(".. unlinked, moving to lost+found")); > + decr_link_count(di->dotdot_parent, > + ip->i_di.di_num.no_addr, > + _(".. unlinked, moving to lost+found")); > dip = fsck_load_inode(sdp, di->dotdot_parent); > if (dip->i_di.di_nlink > 0) { > dip->i_di.di_nlink--; > @@ -203,12 +202,12 @@ int add_inode_to_lf(struct gfs2_inode *ip){ > reprocess_inode(lf_dip, "lost+found"); > > /* This inode is linked from lost+found */ > - increment_link(ip->i_di.di_num.no_addr, lf_dip->i_di.di_num.no_addr, > - _("from lost+found")); > + incr_link_count(ip->i_di.di_num.no_addr, lf_dip->i_di.di_num.no_addr, > + _("from lost+found")); > /* If it's a directory, lost+found is back-linked to it via .. */ > if (S_ISDIR(ip->i_di.di_mode)) > - increment_link(lf_dip->i_di.di_num.no_addr, > - ip->i_di.di_mode, _("to lost+found")); > + incr_link_count(lf_dip->i_di.di_num.no_addr, > + ip->i_di.di_mode, _("to lost+found")); > > log_notice( _("Added inode #%llu (0x%llx) to lost+found dir\n"), > (unsigned long long)ip->i_di.di_num.no_addr, > diff --git a/gfs2/fsck/pass1.c b/gfs2/fsck/pass1.c > index f0e7277..1bd8464 100644 > --- a/gfs2/fsck/pass1.c > +++ b/gfs2/fsck/pass1.c > @@ -1060,7 +1060,7 @@ static int handle_ip(struct gfs2_sbd *sdp, struct gfs2_inode *ip) > goto bad_dinode; > return 0; > } > - if (set_link_count(ip->i_di.di_num.no_addr, ip->i_di.di_nlink)) > + if (set_di_nlink(ip)) > goto bad_dinode; > > if (S_ISDIR(ip->i_di.di_mode) && > diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c > index 72bd107..614c963 100644 > --- a/gfs2/fsck/pass2.c > +++ b/gfs2/fsck/pass2.c > @@ -219,7 +219,7 @@ static int check_dentry(struct gfs2_inode *ip, struct gfs2_dirent *dent, > (*count)++; > ds->entry_count++; > /* can't do this because the block is out of range: > - increment_link(entryblock); */ > + incr_link_count(entryblock); */ > return 0; > } > } > @@ -306,7 +306,7 @@ static int check_dentry(struct gfs2_inode *ip, struct gfs2_dirent *dent, > > /* Don't decrement the link here: Here in pass2, we increment > only when we know it's okay. > - decrement_link(ip->i_di.di_num.no_addr); */ > + decr_link_count(ip->i_di.di_num.no_addr); */ > /* If it was previously marked invalid (i.e. known > to be bad, not just a free block, etc.) then the temptation > would be to delete any metadata it holds. The trouble is: > @@ -504,8 +504,8 @@ static int check_dentry(struct gfs2_inode *ip, struct gfs2_dirent *dent, > } > dentry_is_valid: > /* This directory inode links to this inode via this dentry */ > - increment_link(entryblock, ip->i_di.di_num.no_addr, > - _("valid reference")); > + incr_link_count(entryblock, ip->i_di.di_num.no_addr, > + _("valid reference")); > (*count)++; > ds->entry_count++; > /* End of checks */ > @@ -601,9 +601,9 @@ static int check_system_dir(struct gfs2_inode *sysinode, const char *dirname, > if (cur_blks != sysinode->i_di.di_blocks) > reprocess_inode(sysinode, dirname); > /* This system inode is linked to itself via '.' */ > - increment_link(sysinode->i_di.di_num.no_addr, > - sysinode->i_di.di_num.no_addr, > - "sysinode \".\""); > + incr_link_count(sysinode->i_di.di_num.no_addr, > + sysinode->i_di.di_num.no_addr, > + "sysinode \".\""); > ds.entry_count++; > free(filename); > } else > @@ -820,9 +820,9 @@ int pass2(struct gfs2_sbd *sdp) > reprocess_inode(ip, dirname); > } > /* directory links to itself via '.' */ > - increment_link(ip->i_di.di_num.no_addr, > - ip->i_di.di_num.no_addr, > - _("\". (itself)\"")); > + incr_link_count(ip->i_di.di_num.no_addr, > + ip->i_di.di_num.no_addr, > + _("\". (itself)\"")); > ds.entry_count++; > free(filename); > log_err( _("The directory was fixed.\n")); > diff --git a/gfs2/fsck/pass3.c b/gfs2/fsck/pass3.c > index ff045de..ea8c51d 100644 > --- a/gfs2/fsck/pass3.c > +++ b/gfs2/fsck/pass3.c > @@ -52,7 +52,7 @@ static int attach_dotdot_to(struct gfs2_sbd *sdp, uint64_t newdotdot, > if (gfs2_dirent_del(ip, filename, filename_len)) > log_warn( _("Unable to remove \"..\" directory entry.\n")); > else > - decrement_link(olddotdot, block, _("old \"..\"")); > + decr_link_count(olddotdot, block, _("old \"..\"")); > cur_blks = ip->i_di.di_blocks; > err = dir_add(ip, filename, filename_len, &pip->i_di.di_num, DT_DIR); > if (err) { > @@ -68,7 +68,7 @@ static int attach_dotdot_to(struct gfs2_sbd *sdp, uint64_t newdotdot, > (unsigned long long)ip->i_di.di_num.no_addr); > reprocess_inode(ip, dirname); > } > - increment_link(newdotdot, block, _("new \"..\"")); > + incr_link_count(newdotdot, block, _("new \"..\"")); > fsck_inode_put(&ip); > fsck_inode_put(&pip); > free(filename); > diff --git a/gfs2/fsck/pass4.c b/gfs2/fsck/pass4.c > index 4a1566d..0614372 100644 > --- a/gfs2/fsck/pass4.c > +++ b/gfs2/fsck/pass4.c > @@ -142,11 +142,11 @@ static int scan_inode_list(struct gfs2_sbd *sdp) { > log_err( _("Unlinked inode left unlinked\n")); > fsck_inode_put(&ip); > } /* if (ii->counted_links == 0) */ > - else if (ii->link_count != ii->counted_links) { > + else if (ii->di_nlink != ii->counted_links) { > log_err( _("Link count inconsistent for inode %llu" > " (0x%llx) has %u but fsck found %u.\n"), > (unsigned long long)ii->inode, > - (unsigned long long)ii->inode, ii->link_count, > + (unsigned long long)ii->inode, ii->di_nlink, > ii->counted_links); > /* Read in the inode, adjust the link count, > * and write it back out */ > @@ -156,13 +156,13 @@ static int scan_inode_list(struct gfs2_sbd *sdp) { > (unsigned long long)ii->inode)) { > ip = fsck_load_inode(sdp, ii->inode); /* bread, inode_get */ > fix_link_count(ii, ip); > - ii->link_count = ii->counted_links; > + ii->di_nlink = ii->counted_links; > fsck_inode_put(&ip); /* out, brelse, free */ > log_warn( _("Link count updated to %d for " > "inode %llu (0x%llx)\n"), > - ii->link_count, > - (unsigned long long)ii->inode, > - (unsigned long long)ii->inode); > + ii->di_nlink, > + (unsigned long long)ii->inode, > + (unsigned long long)ii->inode); > } else { > log_err( _("Link count for inode %llu (0x%llx) still incorrect\n"), > (unsigned long long)ii->inode, > @@ -171,7 +171,7 @@ static int scan_inode_list(struct gfs2_sbd *sdp) { > } > log_debug( _("block %llu (0x%llx) has link count %d\n"), > (unsigned long long)ii->inode, > - (unsigned long long)ii->inode, ii->link_count); > + (unsigned long long)ii->inode, ii->di_nlink); > } /* osi_list_foreach(tmp, list) */ > > if (lf_addition) {