Re: [PATCH v4] ovl: whiteout inode sharing

[Amir Goldstein <amir73il@gmail.com>]

On Tue, Apr 28, 2020 at 3:21 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, Apr 24, 2020 at 05:49:00PM +0300, Amir Goldstein wrote:
>
> > I didn't mean we need to check if link_max  is valid range for upper fs.
> > We anyway use minimum of user requested value and upper fs max.
> >
> > Frankly, I think there are only two sane options for system wide configuration:
> > 1. disable whiteout link
> > 2. enable whiteout link with ofs->workdir->d_sb->s_max_links
>
> And that one doesn't work, see for example ext4, which defines EXT4_LINK_MAX but
> doesn't set s_max_links.  This could be fixed, but using EMLINK to detect the
> max-link condition is simpler and more reliable.
>
> And I don't really see a reason to disable whiteout hard links.  What scenario
> would that be useful in?

I have a vague memory of e2fsck excessive memory consumption
in face of many hardlinks created by rsync backups.
Now I suppose it was a function of number of files with nlink > 1 and not
a function of nlink itself and could be a non issue for a long time, but I am
just being careful about introducing non-standard setups which may end up
exposing filesystem corner case bugs (near the edge of s_max_links).
Yeh that is very defensive, so I don't mind ignoring that concern and addressing
it in case somebody shouts.

>
> Updated patch below.  Changes from v5:
>
>  - fix a missing dput on shutdown
>  - don't poass workdir to ovl_cleanup_and_whiteout/ovl_whiteout
>  - flatten out retry loop in ovl_whiteout
>  - use EMLINK to distinguish max-links from misc failure
>
> Thanks,
> Miklos
>
> ---
> From: Chengguang Xu <cgxu519@mykernel.net>
> Subject: ovl: whiteout inode sharing
> Date: Fri, 24 Apr 2020 10:55:17 +0800
>
> Share inode with different whiteout files for saving inode and speeding up
> delete operation.
>
> If EMLINK is encountered when linking a shared whiteout, create a new one.
> In case of any other error, disable sharing for this super block.
>
> Note: ofs->whiteout is protected by inode lock on workdir.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/dir.c       |   49 +++++++++++++++++++++++++++++++++++------------
>  fs/overlayfs/overlayfs.h |    2 -
>  fs/overlayfs/ovl_entry.h |    3 ++
>  fs/overlayfs/readdir.c   |    2 -
>  fs/overlayfs/super.c     |    4 +++
>  fs/overlayfs/util.c      |    3 +-
>  6 files changed, 48 insertions(+), 15 deletions(-)
>
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -62,35 +62,59 @@ struct dentry *ovl_lookup_temp(struct de
>  }
>
>  /* caller holds i_mutex on workdir */
> -static struct dentry *ovl_whiteout(struct dentry *workdir)
> +static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
>  {
>         int err;
>         struct dentry *whiteout;
> +       struct dentry *workdir = ofs->workdir;
>         struct inode *wdir = workdir->d_inode;
>
> -       whiteout = ovl_lookup_temp(workdir);
> -       if (IS_ERR(whiteout))
> -               return whiteout;
> +       if (!ofs->whiteout) {
> +               whiteout = ovl_lookup_temp(workdir);
> +               if (IS_ERR(whiteout))
> +                       return whiteout;
> +
> +               err = ovl_do_whiteout(wdir, whiteout);
> +               if (err) {
> +                       dput(whiteout);
> +                       return ERR_PTR(err);
> +               }
> +               ofs->whiteout = whiteout;
> +       }
>
> -       err = ovl_do_whiteout(wdir, whiteout);
> -       if (err) {
> +       if (ofs->share_whiteout) {
> +               whiteout = ovl_lookup_temp(workdir);
> +               if (IS_ERR(whiteout))
> +                       goto fallback;
> +
> +               err = ovl_do_link(ofs->whiteout, wdir, whiteout);
> +               if (!err)
> +                       goto success;
> +
> +               if (err != -EMLINK) {
> +                       pr_warn("Failed to link whiteout - disabling whiteout inode sharing(nlink=%u, err=%i)\n",
> +                               ofs->whiteout->d_inode->i_nlink, err);
> +                       ofs->share_whiteout = false;
> +               }
>                 dput(whiteout);
> -               whiteout = ERR_PTR(err);
>         }
> -
> +fallback:
> +       whiteout = ofs->whiteout;
> +       ofs->whiteout = NULL;
> +success:

This label is a bit strange, but fine.

>         return whiteout;
>  }
>

Thanks,
Amir.