---- 在 星期五, 2020-04-03 17:18:06 Amir Goldstein <amir73il@xxxxxxxxx> 撰写 ----
> On Fri, Apr 3, 2020 at 9:45 AM Chengguang Xu <cgxu519@xxxxxxxxxxxx> wrote:
> >
> > Sharing inode with different whiteout files for saving
> > inode and speeding up delete operation.
> >
> > Signed-off-by: Chengguang Xu <cgxu519@xxxxxxxxxxxx>
>
> A few more nits.
> Please wait with v3 until I fix my patches, so you can test in top of
> them.
> Please run the overlay xfstests to test your patch.
>
> I suspect this part of test overlay/031 will fail and will need to fix
> the test to expect at most single whiteout 'residue' in work dir:
>
> # try to remove test dir from overlay dir, trigger ovl_remove_and_whiteout,
> # it will not clean up the dir and lead to residue.
> rm -rf $SCRATCH_MNT/testdir 2>&1 | _filter_scratch
> ls $workdir/work
It seems no effect to current test case, I passed all test cases in overlay dir
except nfs_export and mmap related cases.
>
> And you should start writing a test.
> I suggest setting /sys/module/overlay/parameters/whiteout_link_max to 2
> (test should _not_run if param does not exist)
> removing a bunch of files and verify after unmount that:
> - whiteouts have nlink 2
> - there is no more than single residue whiteout in work dir
>
I‘ll do.
> > ---
> > fs/overlayfs/dir.c | 50 ++++++++++++++++++++++++++++++++--------
> > fs/overlayfs/overlayfs.h | 11 +++++++--
> > fs/overlayfs/ovl_entry.h | 4 ++++
> > fs/overlayfs/readdir.c | 3 ++-
> > fs/overlayfs/super.c | 10 ++++++++
> > fs/overlayfs/util.c | 3 ++-
> > 6 files changed, 68 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index 279009dee366..e48ba7de1bcb 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -62,35 +62,66 @@ struct dentry *ovl_lookup_temp(struct dentry *workdir)
> > }
> >
> > /* caller holds i_mutex on workdir */
> > -static struct dentry *ovl_whiteout(struct dentry *workdir)
> > +static struct dentry *ovl_whiteout(struct ovl_fs *ofs, struct dentry *workdir)
> > {
> > int err;
> > + bool same = true;
>
> 'same' is not the best name, but I expect you won't need it anyway.
> I would replace this with:
> bool should_link = (ofs->whiteout_link_max > 1);
I'll fix in V3.
>
> > + bool again = true;
> > struct dentry *whiteout;
> > struct inode *wdir = workdir->d_inode;
> >
> > + if (ofs->workdir != workdir)
> > + same = false;
> > +retry:
> > whiteout = ovl_lookup_temp(workdir);
> > if (IS_ERR(whiteout))
> > return whiteout;
> >
> > + if (same && ofs->whiteout) {
> > + if (ovl_whiteout_linkable(ofs, workdir, ofs->whiteout)) {
> > + err = ovl_do_link(ofs->whiteout, wdir, whiteout);
> > + if (!err)
> > + return whiteout;
> > +
> > + if (!again)
> > + goto out;
> > + }
> > +
>
> We actually need to also cleanup this whiteout:
>
I slightly changed the logic here and in this case it will be work just like before.
> ovl_cleanup(wdir, ofs->whiteout);
>
> > + dput(ofs->whiteout);
> > + ofs->whiteout = NULL;
> > + }
> > +
> > err = ovl_do_whiteout(wdir, whiteout);
> > - if (err) {
> > - dput(whiteout);
> > - whiteout = ERR_PTR(err);
> > + if (!err) {
>
> save the nesting:
> if (err)
> goto out;
>
I'll fix in V3.
> > + if (!same || ofs->whiteout_link_max == 1)
> > + return whiteout;
> > +
> > + if (!again) {
> > + WARN_ON_ONCE(1);
>
> Definitely no WARN on this case.
> We can consider setting whiteout_link_max to 0 and pr_warn()
> about auto disabling whiteout linking due to unexpected failure.
>
I'll fix in V3.
> > + return whiteout;
> > + }
> > +
> > + dget(whiteout);
> > + ofs->whiteout = whiteout;
>
> Shorter:
> ofs->whiteout = dget(whiteout);
>
Here, we don't need to grab the dentry again, I think we have already got
the reference in lookup.
> > + again = false;
> > + goto retry;
> > }
> >
> > - return whiteout;
> > +out:
> > + dput(whiteout);
> > + return ERR_PTR(err);
> > }
> >
> > /* Caller must hold i_mutex on both workdir and dir */
> > -int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
> > - struct dentry *dentry)
> > +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *workdir,
> > + struct inode *dir, struct dentry *dentry)
> > {
> > struct inode *wdir = workdir->d_inode;
> > struct dentry *whiteout;
> > int err;
> > int flags = 0;
> >
> > - whiteout = ovl_whiteout(workdir);
> > + whiteout = ovl_whiteout(ofs, workdir);
> > err = PTR_ERR(whiteout);
> > if (IS_ERR(whiteout))
> > return err;
> > @@ -715,6 +746,7 @@ static bool ovl_matches_upper(struct dentry *dentry, struct dentry *upper)
> > static int ovl_remove_and_whiteout(struct dentry *dentry,
> > struct list_head *list)
> > {
> > + struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > struct dentry *workdir = ovl_workdir(dentry);
> > struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
> > struct dentry *upper;
> > @@ -748,7 +780,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
> > goto out_dput_upper;
> > }
> >
> > - err = ovl_cleanup_and_whiteout(workdir, d_inode(upperdir), upper);
> > + err = ovl_cleanup_and_whiteout(ofs, workdir, d_inode(upperdir), upper);
> > if (err)
> > goto out_d_drop;
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index e6f3670146ed..cc7bcc3fb916 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -225,6 +225,13 @@ static inline bool ovl_open_flags_need_copy_up(int flags)
> > return ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC));
> > }
> >
> > +static inline bool ovl_whiteout_linkable(struct ovl_fs *ofs,
> > + struct dentry *workdir,
>
> workdir unused.
I'll fix in V3.
>
> > + struct dentry *dentry)
> > +{
> > + return dentry->d_inode->i_nlink < ofs->whiteout_link_max;
> > +}
> > +
> > /* util.c */
> > int ovl_want_write(struct dentry *dentry);
> > void ovl_drop_write(struct dentry *dentry);
> > @@ -455,8 +462,8 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
> >
> > /* dir.c */
> > extern const struct inode_operations ovl_dir_inode_operations;
> > -int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
> > - struct dentry *dentry);
> > +int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *workdir,
> > + struct inode *dir, struct dentry *dentry);
> > struct ovl_cattr {
> > dev_t rdev;
> > umode_t mode;
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index 5762d802fe01..de5f230b6e6b 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -77,6 +77,10 @@ struct ovl_fs {
> > int xino_mode;
> > /* For allocation of non-persistent inode numbers */
> > atomic_long_t last_ino;
> > + /* Whiteout dentry cache */
> > + struct dentry *whiteout;
> > + /* Whiteout max link count */
> > + unsigned int whiteout_link_max;
> > };
> >
> > static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index e452ff7d583d..1e921115e6aa 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -1146,7 +1146,8 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> > * Whiteout orphan index to block future open by
> > * handle after overlay nlink dropped to zero.
> > */
> > - err = ovl_cleanup_and_whiteout(indexdir, dir, index);
> > + err = ovl_cleanup_and_whiteout(ofs, indexdir, dir,
> > + index);
> > } else {
> > /* Cleanup orphan index entries */
> > err = ovl_cleanup(dir, index);
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index 732ad5495c92..340c4c05c410 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -26,6 +26,10 @@ struct ovl_dir_cache;
> >
> > #define OVL_MAX_STACK 500
> >
> > +static unsigned int ovl_whiteout_link_max_def = 60000;
> > +module_param_named(whiteout_link_max, ovl_whiteout_link_max_def, uint, 0644);
> > +MODULE_PARM_DESC(whiteout_link_max, "Maximum count of whiteout file link");
> > +
> > static bool ovl_redirect_dir_def = IS_ENABLED(CONFIG_OVERLAY_FS_REDIRECT_DIR);
> > module_param_named(redirect_dir, ovl_redirect_dir_def, bool, 0644);
> > MODULE_PARM_DESC(redirect_dir,
> > @@ -219,6 +223,7 @@ static void ovl_free_fs(struct ovl_fs *ofs)
> > iput(ofs->upperdir_trap);
> > dput(ofs->indexdir);
> > dput(ofs->workdir);
> > + dput(ofs->whiteout);
> > if (ofs->workdir_locked)
> > ovl_inuse_unlock(ofs->workbasedir);
> > dput(ofs->workbasedir);
> > @@ -1762,6 +1767,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
> >
> > if (!ofs->workdir)
> > sb->s_flags |= SB_RDONLY;
> > + else
> > + ofs->whiteout_link_max = min_not_zero(
> > + ofs->workdir->d_sb->s_max_links,
> > + ovl_whiteout_link_max_def ?
> > + ovl_whiteout_link_max_def : 1);
> >
>
> ovl_whiteout_link_max_def ?: 1);
I'll fix in V3.
Thanks,
cgxu
