On Tue, Apr 28, 2020 at 3:21 PM Miklos Szeredi <miklos@xxxxxxxxxx> 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@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
> Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> ---
> 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.
