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
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
> ---
> 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);
> + 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:
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;
> + 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.
> + return whiteout;
> + }
> +
> + dget(whiteout);
> + ofs->whiteout = whiteout;
Shorter:
ofs->whiteout = dget(whiteout);
> + 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.
> + 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);
Thanks,
Amir.
