On Thu, Apr 2, 2020 at 11:58 AM Chengguang Xu <cgxu519@xxxxxxxxxxxx> wrote:
>
> Sharing inode with different whiteout files for saving
> inode and speeding up delete opration.
>
> Signed-off-by: Chengguang Xu <cgxu519@xxxxxxxxxxxx>
> ---
>
> Hi Miklos, Amir
>
> This is another inode sharing approach for whiteout files compare
> to Tao's previous patch. I didn't receive feedback from Tao for
> further update and this new approach seems more simple and reliable.
> Could you have a look at this patch?
>
I like the simplification, but there are some parts of Tao's patch you
removed without understanding they need to be restored.
The main think you missed is that it is not safe to protect ofs->whiteout
with i_mutex on workdir, because workdir in ovl_whiteout() can be
one of 2 directories.
This is the point were the discussion on V3 got derailed.
I will try to work on a patch unifying index/work dirs to solve this
problem, so you won't need to change anything in your patch,
but it will depend on this prerequisite.
As alternative, if you do not wish to wait for my patch,
please see the check for (workdir == ofs->workdir) in Tao's patch.
More below...
>
> fs/overlayfs/dir.c | 53 ++++++++++++++++++++++++++++++++++------
> fs/overlayfs/overlayfs.h | 2 +-
> fs/overlayfs/ovl_entry.h | 2 ++
> fs/overlayfs/readdir.c | 3 ++-
> fs/overlayfs/super.c | 3 +++
> fs/overlayfs/util.c | 3 ++-
> 6 files changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 279009dee366..d5c2e1ada624 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -61,36 +61,74 @@ struct dentry *ovl_lookup_temp(struct dentry *workdir)
> return temp;
> }
>
> +const unsigned int ovl_whiteout_link_max = 60000;
Please keep this a module param as in V3.
A module param is also a way to disable whiteout linking
if for some reason it causes problems.
> +
> +static bool ovl_whiteout_linkable(struct dentry *dentry)
> +{
> + unsigned int max;
> +
> + max = min_not_zero(dentry->d_sb->s_max_links, ovl_whiteout_link_max);
> + if (dentry->d_inode->i_nlink >= max)
> + return false;
> + return true;
return (dentry->d_inode->i_nlink < max);
> +}
> +
> /* caller holds i_mutex on workdir */
> -static struct dentry *ovl_whiteout(struct dentry *workdir)
> +static struct dentry *ovl_whiteout(struct dentry *workdir, struct ovl_fs *ofs)
Please keep ofs argument first as per convention.
> {
> int err;
> + bool again = true;
bool again = (ovl_whiteout_link_max > 1);
assuming that it is changed to a module param.
> struct dentry *whiteout;
> struct inode *wdir = workdir->d_inode;
>
> +retry:
> whiteout = ovl_lookup_temp(workdir);
> if (IS_ERR(whiteout))
> return whiteout;
>
> +
> + if (ofs->whiteout) {
> + if (ovl_whiteout_linkable(ofs->whiteout)) {
> + err = ovl_do_link(ofs->whiteout, wdir, whiteout);
> + if (!err)
> + return whiteout;
> +
> + if (!again)
> + goto out;
> + }
> +
> + err = ovl_do_unlink(ofs->workdir->d_inode, ofs->whiteout);
use 'wdir'
> + ofs->whiteout = NULL;
dput(ofs->whiteout); before reset
> + if (err)
> + goto out;
> + }
> +
> err = ovl_do_whiteout(wdir, whiteout);
> - if (err) {
> - dput(whiteout);
> - whiteout = ERR_PTR(err);
> + if (!err) {
> + ofs->whiteout = whiteout;
only set ofs->whiteout if (again) and get a reference.
otherwise return the whiteout.
> + if (again) {
> + again = false;
dget(whiteout);
> + goto retry;
> + }
> + return ERR_PTR(-EIO);
Why fail? just return the whiteout.
> }
>
> +out:
> + dput(whiteout);
> + whiteout = ERR_PTR(err);
> return 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)
> + struct dentry *dentry, struct ovl_fs *ofs)
ofs arg first
> {
> struct inode *wdir = workdir->d_inode;
> struct dentry *whiteout;
> int err;
> int flags = 0;
>
> - whiteout = ovl_whiteout(workdir);
> + whiteout = ovl_whiteout(workdir, ofs);
> err = PTR_ERR(whiteout);
> if (IS_ERR(whiteout))
> return err;
> @@ -715,6 +753,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 +787,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(workdir, d_inode(upperdir), upper, ofs);
> if (err)
> goto out_d_drop;
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index e6f3670146ed..6212feef36c5 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -456,7 +456,7 @@ 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);
> + struct dentry *dentry, struct ovl_fs *ofs);
> 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..408aa6c7a3bd 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -77,6 +77,8 @@ struct ovl_fs {
> int xino_mode;
> /* For allocation of non-persistent inode numbers */
> atomic_long_t last_ino;
> + /* Whiteout dentry cache */
> + struct dentry *whiteout;
> };
>
> 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..0c8c7ff4fc9e 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(indexdir, dir, index,
> + ofs);
> } else {
> /* Cleanup orphan index entries */
> err = ovl_cleanup(dir, index);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 732ad5495c92..fae9729ff018 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -240,6 +240,9 @@ static void ovl_free_fs(struct ovl_fs *ofs)
> kfree(ofs->config.redirect_mode);
> if (ofs->creator_cred)
> put_cred(ofs->creator_cred);
> + if (ofs->whiteout)
> + ovl_do_unlink(ofs->workdir->d_inode,
> + ofs->whiteout);
You cannot and should not do that here.
It will be cleaned up on next mount.
You need here unconditional:
dputt(whiteout);
Thanks,
Amir.
