[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Cluster-devel] [Patch 09/44] fsck.gfs2: Rename the nlink functions to make them more intuitive



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 <rpeterso redhat com>
> 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) {



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]