* [PATCH] erofs-utils: update i_nlink stat for directories
[not found] <20201205055732.14276-1-hsiangkao.ref@aol.com>
@ 2020-12-05 5:57 ` Gao Xiang via Linux-erofs
2020-12-05 8:32 ` Li GuiFu via Linux-erofs
0 siblings, 1 reply; 6+ messages in thread
From: Gao Xiang via Linux-erofs @ 2020-12-05 5:57 UTC (permalink / raw)
To: linux-erofs
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;
#define S_SHIFT 12
-static unsigned char erofs_type_by_mode[S_IFMT >> S_SHIFT] = {
+static unsigned char erofs_ftype_by_mode[S_IFMT >> S_SHIFT] = {
[S_IFREG >> S_SHIFT] = EROFS_FT_REG_FILE,
[S_IFDIR >> S_SHIFT] = EROFS_FT_DIR,
[S_IFCHR >> S_SHIFT] = EROFS_FT_CHRDEV,
@@ -35,6 +35,11 @@ static unsigned char erofs_type_by_mode[S_IFMT >> S_SHIFT] = {
[S_IFLNK >> S_SHIFT] = EROFS_FT_SYMLINK,
};
+static unsigned char erofs_mode_to_ftype(umode_t mode)
+{
+ return erofs_ftype_by_mode[(mode & S_IFMT) >> S_SHIFT];
+}
+
#define NR_INODE_HASHTABLE 16384
struct list_head inode_hashtable[NR_INODE_HASHTABLE];
@@ -156,7 +161,7 @@ static int __allocate_inode_bh_data(struct erofs_inode *inode,
int erofs_prepare_dir_file(struct erofs_inode *dir)
{
struct erofs_dentry *d;
- unsigned int d_size;
+ unsigned int d_size, i_nlink;
int ret;
/* dot is pointed to the current dir inode */
@@ -169,16 +174,28 @@ int erofs_prepare_dir_file(struct erofs_inode *dir)
d->inode = erofs_igrab(dir->i_parent);
d->type = EROFS_FT_DIR;
- /* let's calculate dir size */
+ /* let's calculate dir size and update i_nlink */
d_size = 0;
+ i_nlink = 0;
list_for_each_entry(d, &dir->i_subdirs, d_child) {
int len = strlen(d->name) + sizeof(struct erofs_dirent);
if (d_size % EROFS_BLKSIZ + len > EROFS_BLKSIZ)
d_size = round_up(d_size, EROFS_BLKSIZ);
d_size += len;
+
+ i_nlink += (d->type == EROFS_FT_DIR);
}
dir->i_size = d_size;
+ /*
+ * if there're too many subdirs as compact form, set nlink=1
+ * rather than upgrade to use extented form instead.
+ */
+ if (i_nlink > USHRT_MAX &&
+ dir->inode_isize == sizeof(struct erofs_inode_compact))
+ dir->i_nlink = 1;
+ else
+ dir->i_nlink = i_nlink;
/* no compression for all dirs */
dir->datalayout = EROFS_INODE_FLAT_INLINE;
@@ -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);
}
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,
--
2.24.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] erofs-utils: update i_nlink stat for directories
2020-12-05 5:57 ` [PATCH] erofs-utils: update i_nlink stat for directories Gao Xiang via Linux-erofs
@ 2020-12-05 8:32 ` Li GuiFu via Linux-erofs
2020-12-05 8:38 ` Gao Xiang
0 siblings, 1 reply; 6+ messages in thread
From: Li GuiFu via Linux-erofs @ 2020-12-05 8:32 UTC (permalink / raw)
To: Gao Xiang, linux-erofs
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] erofs-utils: update i_nlink stat for directories
2020-12-05 8:32 ` Li GuiFu via Linux-erofs
@ 2020-12-05 8:38 ` Gao Xiang
2020-12-05 8:43 ` Gao Xiang
0 siblings, 1 reply; 6+ messages in thread
From: Gao Xiang @ 2020-12-05 8:38 UTC (permalink / raw)
To: Li GuiFu; +Cc: linux-erofs
On Sat, Dec 05, 2020 at 04:32:44PM +0800, Li GuiFu via Linux-erofs wrote:
...
>
> > @@ -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
It's just set temporarily, since only dirs are useful when counting subdirs, so
only needs to differ dirs and non-dirs here. (Previously d->type is unused
at this time.)
...
> > - 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;
The real on-disk d->type will be set here rather than the above.
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] erofs-utils: update i_nlink stat for directories
2020-12-05 8:38 ` Gao Xiang
@ 2020-12-05 8:43 ` Gao Xiang
2020-12-05 9:05 ` Li GuiFu via Linux-erofs
0 siblings, 1 reply; 6+ messages in thread
From: Gao Xiang @ 2020-12-05 8:43 UTC (permalink / raw)
To: Li GuiFu; +Cc: linux-erofs
On Sat, Dec 05, 2020 at 04:38:37PM +0800, Gao Xiang wrote:
> On Sat, Dec 05, 2020 at 04:32:44PM +0800, Li GuiFu via Linux-erofs wrote:
>
> ...
>
> >
> > > @@ -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
>
> It's just set temporarily, since only dirs are useful when counting subdirs, so
> only needs to differ dirs and non-dirs here. (Previously d->type is unused
> at this time.)
btw, I once tried to set up d->type via dp->d_type here, but it increases a
lot of code and seems unnecessary (since deriving from i_mode is enough).
So again, here we only cares about dir and non-dirs (we don't care much about
the specific kind of non-dirs here).
Thanks,
Gao Xiang
>
> ...
>
> > > - 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;
>
> The real on-disk d->type will be set here rather than the above.
>
> Thanks,
> Gao Xiang
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] erofs-utils: update i_nlink stat for directories
2020-12-05 8:43 ` Gao Xiang
@ 2020-12-05 9:05 ` Li GuiFu via Linux-erofs
2020-12-05 9:09 ` Gao Xiang
0 siblings, 1 reply; 6+ messages in thread
From: Li GuiFu via Linux-erofs @ 2020-12-05 9:05 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-erofs
On 2020/12/5 16:43, Gao Xiang wrote:
> On Sat, Dec 05, 2020 at 04:38:37PM +0800, Gao Xiang wrote:
>> On Sat, Dec 05, 2020 at 04:32:44PM +0800, Li GuiFu via Linux-erofs wrote:
>>
>> ...
>>
>>>
>>>> @@ -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
>>
>> It's just set temporarily, since only dirs are useful when counting subdirs, so
>> only needs to differ dirs and non-dirs here. (Previously d->type is unused
>> at this time.)
>
> btw, I once tried to set up d->type via dp->d_type here, but it increases a
> lot of code and seems unnecessary (since deriving from i_mode is enough).
> So again, here we only cares about dir and non-dirs (we don't care much about
> the specific kind of non-dirs here).
>
> Thanks,
> Gao Xiang
>
>>
>> ...
>>
>>>> - 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;
>>
>> The real on-disk d->type will be set here rather than the above.
Yes, what it makes confused is here, EROFS_FT_UNKNOWN is just temporary.
So how about change to ASSERT at EROFS_FT_DIR
DBG_BUGON(d->type == EROFS_FT_DIR && ftype != EROFS_FT_DIR);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] erofs-utils: update i_nlink stat for directories
2020-12-05 9:05 ` Li GuiFu via Linux-erofs
@ 2020-12-05 9:09 ` Gao Xiang
0 siblings, 0 replies; 6+ messages in thread
From: Gao Xiang @ 2020-12-05 9:09 UTC (permalink / raw)
To: Li GuiFu; +Cc: linux-erofs
On Sat, Dec 05, 2020 at 05:05:35PM +0800, Li GuiFu wrote:
...
> >>>
> >>>> @@ -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
> >>
> >> It's just set temporarily, since only dirs are useful when counting subdirs, so
> >> only needs to differ dirs and non-dirs here. (Previously d->type is unused
> >> at this time.)
> >
> > btw, I once tried to set up d->type via dp->d_type here, but it increases a
> > lot of code and seems unnecessary (since deriving from i_mode is enough).
> > So again, here we only cares about dir and non-dirs (we don't care much about
> > the specific kind of non-dirs here).
> >
> > Thanks,
> > Gao Xiang
> >
> >>
> >> ...
> >>
> >>>> - 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;
> >>
> >> The real on-disk d->type will be set here rather than the above.
> Yes, what it makes confused is here, EROFS_FT_UNKNOWN is just temporary.
> So how about change to ASSERT at EROFS_FT_DIR
>
> DBG_BUGON(d->type == EROFS_FT_DIR && ftype != EROFS_FT_DIR);
>
Ok, how about the following statement:
DBG_BUGON(ftype == EROFS_FT_DIR && d->type != ftype);
It will save some words. I will send the next version soon.
Thanks,
Gao Xiang
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-05 9:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20201205055732.14276-1-hsiangkao.ref@aol.com>
2020-12-05 5:57 ` [PATCH] erofs-utils: update i_nlink stat for directories Gao Xiang via Linux-erofs
2020-12-05 8:32 ` Li GuiFu via Linux-erofs
2020-12-05 8:38 ` Gao Xiang
2020-12-05 8:43 ` Gao Xiang
2020-12-05 9:05 ` Li GuiFu via Linux-erofs
2020-12-05 9:09 ` Gao Xiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.