Re: [PATCH] erofs-utils: update i_nlink stat for directories - Li GuiFu via Linux-erofs

From: Li GuiFu via Linux-erofs <linux-erofs@lists.ozlabs.org>
To: Gao Xiang <hsiangkao@aol.com>, linux-erofs@lists.ozlabs.org
Subject: Re: [PATCH] erofs-utils: update i_nlink stat for directories
Date: Sat, 5 Dec 2020 16:32:44 +0800	[thread overview]
Message-ID: <ed88d60a-77a9-f189-3586-a6d6aef510d9@aliyun.com> (raw)
In-Reply-To: <20201205055732.14276-1-hsiangkao@aol.com>


On 2020/12/5 13:57, Gao Xiang via Linux-erofs wrote:
> From: Gao Xiang <hsiangkao@aol.com>
> 
> Previously, nlink of directories is treated as 1 for simplicity.
> 
> Since st_nlink for dirs is actualy not well defined, nlink=1 seems
> to pacify `find' (even without -noleaf option) and other utilities.
> AFAICT, isofs, romfs and cramfs always set it to 1, Overlayfs sets
> it to 1 conditionally, btrfs[1], ceph[2] and FUSE client historically
> set it to 1.
> 
> The convention under unix is that it's # of subdirs including "."
> and "..". This patch tries to follow such convention if possible to
> optimize `find' performance since it's not quite hard for local fs.
> 
> [1] https://lore.kernel.org/r/20100124003336.GP23006@think
> [2] https://lore.kernel.org/r/20180521092729.17470-1-lhenriques@suse.com
> Signed-off-by: Gao Xiang <hsiangkao@aol.com>
> ---
>  lib/inode.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/inode.c b/lib/inode.c
> index 618eb284550f..357ac480154a 100644
> --- a/lib/inode.c
> +++ b/lib/inode.c
> @@ -25,7 +25,7 @@
>  struct erofs_sb_info sbi;
>  

> @@ -957,6 +974,10 @@ struct erofs_inode *erofs_mkfs_build_tree(struct erofs_inode *dir)
>  			ret = PTR_ERR(d);
>  			goto err_closedir;
>  		}
> +
> +		/* to count i_nlink for directories */
> +		d->type = (dp->d_type == DT_DIR ?
> +			EROFS_FT_DIR : EROFS_FT_UNKNOWN);
>  	}
>  
It's confused that d->type was set to EROFS_FT_UNKNOWN when not a dir
It's not clearness whether the program goes wrong or get the wrong data
Actually it's a correct procedure


>  	if (errno) {
> @@ -978,6 +999,7 @@ struct erofs_inode *erofs_mkfs_build_tree(struct erofs_inode *dir)
>  
>  	list_for_each_entry(d, &dir->i_subdirs, d_child) {
>  		char buf[PATH_MAX];
> +		unsigned char ftype;
>  
>  		if (is_dot_dotdot(d->name)) {
>  			erofs_d_invalidate(d);
> @@ -1000,7 +1022,10 @@ fail:
>  			goto err;
>  		}
>  
> -		d->type = erofs_type_by_mode[d->inode->i_mode >> S_SHIFT];
> +		ftype = erofs_mode_to_ftype(d->inode->i_mode);
> +		DBG_BUGON(d->type != EROFS_FT_UNKNOWN && d->type != ftype);
> +		d->type = ftype;
> +
>  		erofs_d_invalidate(d);
>  		erofs_info("add file %s/%s (nid %llu, type %d)",
>  			   dir->i_srcpath, d->name, (unsigned long long)d->nid,
>