[PATCH 0/3] Overlayfs use index dir as work dir

[PATCH 0/3] Overlayfs use index dir as work dir

[Amir Goldstein]

Miklos,

I know its a bit early in the cycle for patches, but those are
needed for the whiteout sharing patch by Chengguang Xu and they are
quite trivial, so I figured I'll just post them early to see if there
are any comments.

Thanks,
Amir.

Amir Goldstein (3):
  ovl: cleanup non-empty directories in ovl_indexdir_cleanup()
  ovl: prepare to copy up without workdir
  ovl: index dir act as work dir

 fs/overlayfs/copy_up.c   |  8 ++++++--
 fs/overlayfs/overlayfs.h |  4 ++--
 fs/overlayfs/readdir.c   | 13 +++++++------
 fs/overlayfs/super.c     | 14 +++++++++-----
 4 files changed, 24 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH 1/3] ovl: cleanup non-empty directories in ovl_indexdir_cleanup()

[Amir Goldstein]

Teach ovl_indexdir_cleanup() to remove temp directories containing
whiteouts to prepare for using index dir instead of work dir for
removing merge directories.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/overlayfs.h |  4 ++--
 fs/overlayfs/readdir.c   | 13 +++++++------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index e6f3670146ed..e00b1ff6dea9 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -394,8 +394,8 @@ void ovl_cleanup_whiteouts(struct dentry *upper, struct list_head *list);
 void ovl_cache_free(struct list_head *list);
 void ovl_dir_cache_free(struct inode *inode);
 int ovl_check_d_type_supported(struct path *realpath);
-void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
-			 struct dentry *dentry, int level);
+int ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
+			struct dentry *dentry, int level);
 int ovl_indexdir_cleanup(struct ovl_fs *ofs);
 
 /* inode.c */
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index e452ff7d583d..d6b601caf122 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1071,14 +1071,13 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
 	ovl_cache_free(&list);
 }
 
-void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
+int ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
 			 struct dentry *dentry, int level)
 {
 	int err;
 
 	if (!d_is_dir(dentry) || level > 1) {
-		ovl_cleanup(dir, dentry);
-		return;
+		return ovl_cleanup(dir, dentry);
 	}
 
 	err = ovl_do_rmdir(dir, dentry);
@@ -1088,8 +1087,10 @@ void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
 		inode_unlock(dir);
 		ovl_workdir_cleanup_recurse(&path, level + 1);
 		inode_lock_nested(dir, I_MUTEX_PARENT);
-		ovl_cleanup(dir, dentry);
+		err = ovl_cleanup(dir, dentry);
 	}
+
+	return err;
 }
 
 int ovl_indexdir_cleanup(struct ovl_fs *ofs)
@@ -1132,8 +1133,8 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 		if (!err) {
 			goto next;
 		} else if (err == -ESTALE) {
-			/* Cleanup stale index entries */
-			err = ovl_cleanup(dir, index);
+			/* Cleanup stale index entries and leftover whiteouts */
+			err = ovl_workdir_cleanup(dir, path.mnt, index, 1);
 		} else if (err != -ENOENT) {
 			/*
 			 * Abort mount to avoid corrupting the index if
-- 
2.17.1

[PATCH 2/3] ovl: prepare to copy up without workdir

[Amir Goldstein]

With index=on, we copy up lower hardlinks to work dir and move them
into index dir. Fix locking to allow work dir and index dir to be the
same directory.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 9709cf22cab3..e523e63f604f 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -576,7 +576,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir);
 	struct dentry *temp, *upper;
 	struct ovl_cu_creds cc;
-	int err;
+	int err = 0;
 	struct ovl_cattr cattr = {
 		/* Can't properly set mode on creation because of the umask */
 		.mode = c->stat.mode & S_IFMT,
@@ -584,7 +584,11 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 		.link = c->link
 	};
 
-	err = ovl_lock_rename_workdir(c->workdir, c->destdir);
+	/* Are we copying up to indexdir which is also workdir? */
+	if (c->indexed && c->workdir == c->destdir)
+		inode_lock_nested(wdir, I_MUTEX_PARENT);
+	else
+		err = ovl_lock_rename_workdir(c->workdir, c->destdir);
 	if (err)
 		return err;
 
-- 
2.17.1

[PATCH 3/3] ovl: index dir act as work dir

[Amir Goldstein]

With index=on, let index dir act as the work dir for copy up and
cleanups.  This will help implementing whiteout inode sharing.

We still create the "work" dir on mount regardless of index=on
and it is used to test the features supported by upper fs.
One reason is that before the feature tests, we do not know if
index could be enabled or not.

The reason we do not use "index" directory also as workdir with
index=off is because the existence of the "index" directory acts
as a simple persistent signal that index was enabled on this
filesystem and tools may want to use that signal.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Miklos,

It is worth mentioning that I contemplated about the right point to
overload workdir with indexdir.

I decided to go for ofs->workdir and not ovl_workdir(), because
it makes the patch touch less code and avoids future uninterntional
uses of ofs->workdir after 'work' dir has been retired.

That said, I do not feel strongly about it, so I could go for
ovl_workdir() if you prefer.

I do feel strongly about the decision to keep 'work' dir for
index=off case.

Thanks,
Amir.

 fs/overlayfs/super.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 732ad5495c92..b91b23a0366c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1777,17 +1777,21 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		sb->s_flags |= SB_RDONLY;
 
 	if (!(ovl_force_readonly(ofs)) && ofs->config.index) {
+		/* index dir will act also as workdir */
+		dput(ofs->workdir);
+		ofs->workdir = NULL;
+		iput(ofs->workdir_trap);
+		ofs->workdir_trap = NULL;
+
 		err = ovl_get_indexdir(sb, ofs, oe, &upperpath);
 		if (err)
 			goto out_free_oe;
 
 		/* Force r/o mount with no index dir */
-		if (!ofs->indexdir) {
-			dput(ofs->workdir);
-			ofs->workdir = NULL;
+		if (ofs->indexdir)
+			ofs->workdir = dget(ofs->indexdir);
+		else
 			sb->s_flags |= SB_RDONLY;
-		}
-
 	}
 
 	err = ovl_check_overlapping_layers(sb, ofs);
-- 
2.17.1

Re: [PATCH 1/3] ovl: cleanup non-empty directories in ovl_indexdir_cleanup()

[Miklos Szeredi]

On Fri, Apr 10, 2020 at 10:25 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Teach ovl_indexdir_cleanup() to remove temp directories containing
> whiteouts to prepare for using index dir instead of work dir for
> removing merge directories.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/overlayfs.h |  4 ++--
>  fs/overlayfs/readdir.c   | 13 +++++++------
>  2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index e6f3670146ed..e00b1ff6dea9 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -394,8 +394,8 @@ void ovl_cleanup_whiteouts(struct dentry *upper, struct list_head *list);
>  void ovl_cache_free(struct list_head *list);
>  void ovl_dir_cache_free(struct inode *inode);
>  int ovl_check_d_type_supported(struct path *realpath);
> -void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> -                        struct dentry *dentry, int level);
> +int ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> +                       struct dentry *dentry, int level);
>  int ovl_indexdir_cleanup(struct ovl_fs *ofs);
>
>  /* inode.c */
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index e452ff7d583d..d6b601caf122 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -1071,14 +1071,13 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
>         ovl_cache_free(&list);
>  }
>
> -void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> +int ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
>                          struct dentry *dentry, int level)
>  {
>         int err;
>
>         if (!d_is_dir(dentry) || level > 1) {
> -               ovl_cleanup(dir, dentry);
> -               return;
> +               return ovl_cleanup(dir, dentry);
>         }
>
>         err = ovl_do_rmdir(dir, dentry);
> @@ -1088,8 +1087,10 @@ void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
>                 inode_unlock(dir);
>                 ovl_workdir_cleanup_recurse(&path, level + 1);
>                 inode_lock_nested(dir, I_MUTEX_PARENT);
> -               ovl_cleanup(dir, dentry);
> +               err = ovl_cleanup(dir, dentry);
>         }
> +
> +       return err;
>  }
>
>  int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> @@ -1132,8 +1133,8 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
>                 if (!err) {
>                         goto next;
>                 } else if (err == -ESTALE) {
> -                       /* Cleanup stale index entries */
> -                       err = ovl_cleanup(dir, index);
> +                       /* Cleanup stale index entries and leftover whiteouts */
> +                       err = ovl_workdir_cleanup(dir, path.mnt, index, 1);

I'd much prefer if cleanup of temp files were handled separately from
stale index entries.  I.e. only call ovl_verify_index() on things that
might actually be index files.


>                 } else if (err != -ENOENT) {
>                         /*
>                          * Abort mount to avoid corrupting the index if
> --
> 2.17.1
>

Re: [PATCH 2/3] ovl: prepare to copy up without workdir

[Miklos Szeredi]

On Fri, Apr 10, 2020 at 10:25 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> With index=on, we copy up lower hardlinks to work dir and move them
> into index dir. Fix locking to allow work dir and index dir to be the
> same directory.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/copy_up.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 9709cf22cab3..e523e63f604f 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -576,7 +576,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>         struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir);
>         struct dentry *temp, *upper;
>         struct ovl_cu_creds cc;
> -       int err;
> +       int err = 0;
>         struct ovl_cattr cattr = {
>                 /* Can't properly set mode on creation because of the umask */
>                 .mode = c->stat.mode & S_IFMT,
> @@ -584,7 +584,11 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
>                 .link = c->link
>         };
>
> -       err = ovl_lock_rename_workdir(c->workdir, c->destdir);
> +       /* Are we copying up to indexdir which is also workdir? */
> +       if (c->indexed && c->workdir == c->destdir)
> +               inode_lock_nested(wdir, I_MUTEX_PARENT);
> +       else
> +               err = ovl_lock_rename_workdir(c->workdir, c->destdir);

This is confusing.  What about just

if (!lock_rename(c->workdir, c->destdir))
    return -EIO;

?

Thanks,
Miklos

Re: [PATCH 3/3] ovl: index dir act as work dir

[Miklos Szeredi]

On Fri, Apr 10, 2020 at 10:25 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> With index=on, let index dir act as the work dir for copy up and
> cleanups.  This will help implementing whiteout inode sharing.
>
> We still create the "work" dir on mount regardless of index=on
> and it is used to test the features supported by upper fs.
> One reason is that before the feature tests, we do not know if
> index could be enabled or not.
>
> The reason we do not use "index" directory also as workdir with
> index=off is because the existence of the "index" directory acts
> as a simple persistent signal that index was enabled on this
> filesystem and tools may want to use that signal.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Miklos,
>
> It is worth mentioning that I contemplated about the right point to
> overload workdir with indexdir.
>
> I decided to go for ofs->workdir and not ovl_workdir(), because
> it makes the patch touch less code and avoids future uninterntional
> uses of ofs->workdir after 'work' dir has been retired.
>
> That said, I do not feel strongly about it, so I could go for
> ovl_workdir() if you prefer.
>
> I do feel strongly about the decision to keep 'work' dir for
> index=off case.

Yeah, this looks good.

Thanks,
Miklos

Re: [PATCH 2/3] ovl: prepare to copy up without workdir

[Amir Goldstein]

On Fri, Apr 17, 2020 at 5:16 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, Apr 10, 2020 at 10:25 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > With index=on, we copy up lower hardlinks to work dir and move them
> > into index dir. Fix locking to allow work dir and index dir to be the
> > same directory.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/overlayfs/copy_up.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 9709cf22cab3..e523e63f604f 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -576,7 +576,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >         struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir);
> >         struct dentry *temp, *upper;
> >         struct ovl_cu_creds cc;
> > -       int err;
> > +       int err = 0;
> >         struct ovl_cattr cattr = {
> >                 /* Can't properly set mode on creation because of the umask */
> >                 .mode = c->stat.mode & S_IFMT,
> > @@ -584,7 +584,11 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
> >                 .link = c->link
> >         };
> >
> > -       err = ovl_lock_rename_workdir(c->workdir, c->destdir);
> > +       /* Are we copying up to indexdir which is also workdir? */
> > +       if (c->indexed && c->workdir == c->destdir)
> > +               inode_lock_nested(wdir, I_MUTEX_PARENT);
> > +       else
> > +               err = ovl_lock_rename_workdir(c->workdir, c->destdir);
>
> This is confusing.  What about just
>
> if (!lock_rename(c->workdir, c->destdir))
>     return -EIO;
>
> ?

Sure. Simple.

Thanks,
Amir.

Re: [PATCH 1/3] ovl: cleanup non-empty directories in ovl_indexdir_cleanup()

[Amir Goldstein]

On Fri, Apr 17, 2020 at 5:08 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, Apr 10, 2020 at 10:25 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Teach ovl_indexdir_cleanup() to remove temp directories containing
> > whiteouts to prepare for using index dir instead of work dir for
> > removing merge directories.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/overlayfs/overlayfs.h |  4 ++--
> >  fs/overlayfs/readdir.c   | 13 +++++++------
> >  2 files changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index e6f3670146ed..e00b1ff6dea9 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -394,8 +394,8 @@ void ovl_cleanup_whiteouts(struct dentry *upper, struct list_head *list);
> >  void ovl_cache_free(struct list_head *list);
> >  void ovl_dir_cache_free(struct inode *inode);
> >  int ovl_check_d_type_supported(struct path *realpath);
> > -void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> > -                        struct dentry *dentry, int level);
> > +int ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> > +                       struct dentry *dentry, int level);
> >  int ovl_indexdir_cleanup(struct ovl_fs *ofs);
> >
> >  /* inode.c */
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index e452ff7d583d..d6b601caf122 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -1071,14 +1071,13 @@ static void ovl_workdir_cleanup_recurse(struct path *path, int level)
> >         ovl_cache_free(&list);
> >  }
> >
> > -void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> > +int ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> >                          struct dentry *dentry, int level)
> >  {
> >         int err;
> >
> >         if (!d_is_dir(dentry) || level > 1) {
> > -               ovl_cleanup(dir, dentry);
> > -               return;
> > +               return ovl_cleanup(dir, dentry);
> >         }
> >
> >         err = ovl_do_rmdir(dir, dentry);
> > @@ -1088,8 +1087,10 @@ void ovl_workdir_cleanup(struct inode *dir, struct vfsmount *mnt,
> >                 inode_unlock(dir);
> >                 ovl_workdir_cleanup_recurse(&path, level + 1);
> >                 inode_lock_nested(dir, I_MUTEX_PARENT);
> > -               ovl_cleanup(dir, dentry);
> > +               err = ovl_cleanup(dir, dentry);
> >         }
> > +
> > +       return err;
> >  }
> >
> >  int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> > @@ -1132,8 +1133,8 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
> >                 if (!err) {
> >                         goto next;
> >                 } else if (err == -ESTALE) {
> > -                       /* Cleanup stale index entries */
> > -                       err = ovl_cleanup(dir, index);
> > +                       /* Cleanup stale index entries and leftover whiteouts */
> > +                       err = ovl_workdir_cleanup(dir, path.mnt, index, 1);
>
> I'd much prefer if cleanup of temp files were handled separately from
> stale index entries.  I.e. only call ovl_verify_index() on things that
> might actually be index files.
>

Ok. fixed tested and pushed to ovl-workdir.

Thanks,
Amir.

Re: [PATCH 1/3] ovl: cleanup non-empty directories in ovl_indexdir_cleanup()

[Miklos Szeredi]

On Fri, Apr 17, 2020 at 6:56 PM Amir Goldstein <amir73il@gmail.com> wrote:

> Ok. fixed tested and pushed to ovl-workdir.

Thanks.   Pushed to #overlayfs-next.

Thanks,
Miklos