[RFC] ovl: System-wide unique st_dev

[RFC] ovl: System-wide unique st_dev

[Chandan Rajendra]

The following sequence of commands causes a lowerdir file and an
overlay file to have the same values for st_ino/st_dev,

$ xfs_io -f -c "pwrite 0 4k" /mnt/lowerdir/testfile -c sync
$ xfs_io -a -c "pwrite 4k 4k" /mnt/ovl/testfile # Copy-up occurs here

This commit solves the bug by reporting pseudo device numbers for files
on lowerdir.

Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
---
 fs/overlayfs/inode.c     | 23 ++++++++++++++++++++---
 fs/overlayfs/ovl_entry.h |  8 +++++++-
 fs/overlayfs/super.c     | 33 +++++++++++++++++++++++++--------
 3 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 5f285c1..154c87892 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -13,6 +13,19 @@
 #include <linux/xattr.h>
 #include <linux/posix_acl.h>
 #include "overlayfs.h"
+#include "ovl_entry.h"
+
+static dev_t get_lowerdir_pseudo_dev(struct ovl_fs *ufs, dev_t dev)
+{
+	int i;
+
+	for (i = 0; i < ufs->numlower; i++) {
+		if (ufs->lower_mnt[i].real_dev == dev)
+			return ufs->lower_mnt[i].pseudo_dev;
+	}
+
+	BUG();
+}
 
 int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 {
@@ -61,6 +74,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 		u32 request_mask, unsigned int flags)
 {
 	struct dentry *dentry = path->dentry;
+	struct ovl_fs *ufs;
 	enum ovl_path_type type;
 	struct path realpath;
 	const struct cred *old_cred;
@@ -103,10 +117,13 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
 			if (is_dir || lowerstat.nlink == 1)
 				stat->ino = lowerstat.ino;
 
-			if (samefs)
+			if (samefs) {
 				WARN_ON_ONCE(stat->dev != lowerstat.dev);
-			else
-				stat->dev = lowerstat.dev;
+			} else {
+				ufs = dentry->d_sb->s_fs_info;
+				stat->dev = get_lowerdir_pseudo_dev(ufs,
+								lowerstat.dev);
+			}
 		}
 		if (samefs)
 			stat->dev = dentry->d_sb->s_dev;
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index 34bc4a9..92aebe9 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -16,11 +16,17 @@ struct ovl_config {
 	bool redirect_dir;
 };
 
+struct ovl_lower_mnt {
+	struct vfsmount *mnt;
+	dev_t real_dev;
+	dev_t pseudo_dev;
+};
+
 /* private information held for overlayfs's superblock */
 struct ovl_fs {
 	struct vfsmount *upper_mnt;
 	unsigned numlower;
-	struct vfsmount **lower_mnt;
+	struct ovl_lower_mnt *lower_mnt;
 	struct dentry *workdir;
 	long namelen;
 	/* pathnames of lower and upper dirs, for show_options */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 4882ffb..c922a92 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -172,8 +172,10 @@ static void ovl_put_super(struct super_block *sb)
 
 	dput(ufs->workdir);
 	mntput(ufs->upper_mnt);
-	for (i = 0; i < ufs->numlower; i++)
-		mntput(ufs->lower_mnt[i]);
+	for (i = 0; i < ufs->numlower; i++) {
+		mntput(ufs->lower_mnt[i].mnt);
+		free_anon_bdev(ufs->lower_mnt[i].pseudo_dev);
+	}
 	kfree(ufs->lower_mnt);
 
 	kfree(ufs->config.lowerdir);
@@ -908,15 +910,25 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	err = -ENOMEM;
-	ufs->lower_mnt = kcalloc(numlower, sizeof(struct vfsmount *), GFP_KERNEL);
+	ufs->lower_mnt = kcalloc(numlower, sizeof(struct ovl_lower_mnt),
+				GFP_KERNEL);
 	if (ufs->lower_mnt == NULL)
 		goto out_put_workdir;
 	for (i = 0; i < numlower; i++) {
-		struct vfsmount *mnt = clone_private_mount(&stack[i]);
+		struct vfsmount *mnt;
+		dev_t dev;
 
+		err = get_anon_bdev(&dev);
+		if (err) {
+			pr_err("overlayfs: failed to get anonymous bdev for lowerpath\n");
+			goto out_put_lower_mnt;
+		}
+
+		mnt = clone_private_mount(&stack[i]);
 		err = PTR_ERR(mnt);
 		if (IS_ERR(mnt)) {
 			pr_err("overlayfs: failed to clone lowerpath\n");
+			free_anon_bdev(dev);
 			goto out_put_lower_mnt;
 		}
 		/*
@@ -925,7 +937,9 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		 */
 		mnt->mnt_flags |= MNT_READONLY | MNT_NOATIME;
 
-		ufs->lower_mnt[ufs->numlower] = mnt;
+		ufs->lower_mnt[ufs->numlower].mnt = mnt;
+		ufs->lower_mnt[ufs->numlower].real_dev = mnt->mnt_sb->s_dev;
+		ufs->lower_mnt[ufs->numlower].pseudo_dev = dev;
 		ufs->numlower++;
 
 		/* Check if all lower layers are on same sb */
@@ -980,7 +994,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	}
 	for (i = 0; i < numlower; i++) {
 		oe->lowerstack[i].dentry = stack[i].dentry;
-		oe->lowerstack[i].mnt = ufs->lower_mnt[i];
+		oe->lowerstack[i].mnt = ufs->lower_mnt[i].mnt;
 	}
 	kfree(stack);
 
@@ -999,8 +1013,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 out_put_cred:
 	put_cred(ufs->creator_cred);
 out_put_lower_mnt:
-	for (i = 0; i < ufs->numlower; i++)
-		mntput(ufs->lower_mnt[i]);
+	for (i = 0; i < ufs->numlower; i++) {
+		if (ufs->lower_mnt[i].mnt)
+			free_anon_bdev(ufs->lower_mnt[i].pseudo_dev);
+		mntput(ufs->lower_mnt[i].mnt);
+	}
 	kfree(ufs->lower_mnt);
 out_put_workdir:
 	dput(ufs->workdir);
-- 
2.9.4

Re: [RFC] ovl: System-wide unique st_dev

[Amir Goldstein]

On Sun, Jun 11, 2017 at 4:44 PM, Chandan Rajendra
<chandan@linux.vnet.ibm.com> wrote:
> The following sequence of commands causes a lowerdir file and an
> overlay file to have the same values for st_ino/st_dev,

You meant different st_dev values...

All in all, looks very good to me. Nice work!
See some nits and suggestions below.

Did you get any progress with tests yet?

>
> $ xfs_io -f -c "pwrite 0 4k" /mnt/lowerdir/testfile -c sync
> $ xfs_io -a -c "pwrite 4k 4k" /mnt/ovl/testfile # Copy-up occurs here

Honestly, I think you could find a simpler example and also one that
demonstrates
the bug, something like:

$ echo 123 > /mnt/lowerdir/testfile
$ echo 456 > /mnt/ovl/testfile
$ cat /mnt/lowerdir/testfile /mnt/ovl/testfile
123
456
$ diff /mnt/lowerdir/testfile /mnt/ovl/testfile && echo "123  == 456 ???"

But that diff example will only demonstrate a bug on top of my branch,
and I don't think that is the right order for this patch. it should
come *before*
the relax samefs patch, so maybe just drop the example?

>
> This commit solves the bug by reporting pseudo device numbers for files
> on lowerdir.
>
> Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> ---
>  fs/overlayfs/inode.c     | 23 ++++++++++++++++++++---
>  fs/overlayfs/ovl_entry.h |  8 +++++++-
>  fs/overlayfs/super.c     | 33 +++++++++++++++++++++++++--------
>  3 files changed, 52 insertions(+), 12 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 5f285c1..154c87892 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -13,6 +13,19 @@
>  #include <linux/xattr.h>
>  #include <linux/posix_acl.h>
>  #include "overlayfs.h"
> +#include "ovl_entry.h"
> +
> +static dev_t get_lowerdir_pseudo_dev(struct ovl_fs *ufs, dev_t dev)

I suggest to call this ovl_get_pseudo_dev (see below why) and
I suggest to check if dev == upper real dev and return the overlay dev.

> +{
> +       int i;
> +
> +       for (i = 0; i < ufs->numlower; i++) {
> +               if (ufs->lower_mnt[i].real_dev == dev)
> +                       return ufs->lower_mnt[i].pseudo_dev;
> +       }
> +
> +       BUG();

No need to BUG() here WARN() and return dev will not cause any
catastrophic bug. overlay will just behave as it does today..

> +}
>
>  int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>  {
> @@ -61,6 +74,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>                 u32 request_mask, unsigned int flags)
>  {
>         struct dentry *dentry = path->dentry;
> +       struct ovl_fs *ufs;

I prefer ofs. The use of ufs is mostly when the info is not yet attached to sb.
Not sure if this was intentional, but its convenient.

>         enum ovl_path_type type;
>         struct path realpath;
>         const struct cred *old_cred;
> @@ -103,10 +117,13 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
>                         if (is_dir || lowerstat.nlink == 1)
>                                 stat->ino = lowerstat.ino;
>
> -                       if (samefs)
> +                       if (samefs) {
>                                 WARN_ON_ONCE(stat->dev != lowerstat.dev);
> -                       else
> -                               stat->dev = lowerstat.dev;
> +                       } else {
> +                               ufs = dentry->d_sb->s_fs_info;
> +                               stat->dev = get_lowerdir_pseudo_dev(ufs,
> +                                                               lowerstat.dev);

if (!is_dir && !samefs) { stat->dev = ovl_get_pseudo_dev ... }
should be outside the OVL_TYPE_ORIGIN condition, because
it also applies to pure lower and pure upper inodes (not copied up).

For pure upper it is just nicer and du -xs will work better on
pure upper dirs.
For lower, it is a must, because overlay inode needs to preserve
st_dev across copy up, so it needs to have the pseudo lower st_dev
before that lower becomes the origin of a copy up.


> +                       }
>                 }
>                 if (samefs)
>                         stat->dev = dentry->d_sb->s_dev;

Re: [RFC] ovl: System-wide unique st_dev

[Chandan Rajendra]

On Sunday, June 11, 2017 9:03:54 PM IST Amir Goldstein wrote:
> On Sun, Jun 11, 2017 at 4:44 PM, Chandan Rajendra
> <chandan@linux.vnet.ibm.com> wrote:
> > The following sequence of commands causes a lowerdir file and an
> > overlay file to have the same values for st_ino/st_dev,
> 
> You meant different st_dev values...
> 
> All in all, looks very good to me. Nice work!
> See some nits and suggestions below.
> 
> Did you get any progress with tests yet?

Thanks for the review comments.

I spent some time to read and figure out the workings of 
unionmount-testsuite framework. I should be able to post the
the new test along with the non-RFC version of this patch.

> 
> >
> > $ xfs_io -f -c "pwrite 0 4k" /mnt/lowerdir/testfile -c sync
> > $ xfs_io -a -c "pwrite 4k 4k" /mnt/ovl/testfile # Copy-up occurs here
> 
> Honestly, I think you could find a simpler example and also one that
> demonstrates
> the bug, something like:
> 
> $ echo 123 > /mnt/lowerdir/testfile
> $ echo 456 > /mnt/ovl/testfile
> $ cat /mnt/lowerdir/testfile /mnt/ovl/testfile
> 123
> 456
> $ diff /mnt/lowerdir/testfile /mnt/ovl/testfile && echo "123  == 456 ???"
> 
> But that diff example will only demonstrate a bug on top of my branch,
> and I don't think that is the right order for this patch. it should
> come *before*
> the relax samefs patch, so maybe just drop the example?

Ok. Understood. I will do that.

> 
> >
> > This commit solves the bug by reporting pseudo device numbers for files
> > on lowerdir.
> >
> > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > ---
> >  fs/overlayfs/inode.c     | 23 ++++++++++++++++++++---
> >  fs/overlayfs/ovl_entry.h |  8 +++++++-
> >  fs/overlayfs/super.c     | 33 +++++++++++++++++++++++++--------
> >  3 files changed, 52 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 5f285c1..154c87892 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -13,6 +13,19 @@
> >  #include <linux/xattr.h>
> >  #include <linux/posix_acl.h>
> >  #include "overlayfs.h"
> > +#include "ovl_entry.h"
> > +
> > +static dev_t get_lowerdir_pseudo_dev(struct ovl_fs *ufs, dev_t dev)
> 
> I suggest to call this ovl_get_pseudo_dev (see below why) and
> I suggest to check if dev == upper real dev and return the overlay dev.

Ok. I agree.
> 
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ufs->numlower; i++) {
> > +               if (ufs->lower_mnt[i].real_dev == dev)
> > +                       return ufs->lower_mnt[i].pseudo_dev;
> > +       }
> > +
> > +       BUG();
> 
> No need to BUG() here WARN() and return dev will not cause any
> catastrophic bug. overlay will just behave as it does today..

I agree.
> 
> > +}
> >
> >  int ovl_setattr(struct dentry *dentry, struct iattr *attr)
> >  {
> > @@ -61,6 +74,7 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >                 u32 request_mask, unsigned int flags)
> >  {
> >         struct dentry *dentry = path->dentry;
> > +       struct ovl_fs *ufs;
> 
> I prefer ofs. The use of ufs is mostly when the info is not yet attached to sb.
> Not sure if this was intentional, but its convenient.

I will fix this.
> 
> >         enum ovl_path_type type;
> >         struct path realpath;
> >         const struct cred *old_cred;
> > @@ -103,10 +117,13 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >                         if (is_dir || lowerstat.nlink == 1)
> >                                 stat->ino = lowerstat.ino;
> >
> > -                       if (samefs)
> > +                       if (samefs) {
> >                                 WARN_ON_ONCE(stat->dev != lowerstat.dev);
> > -                       else
> > -                               stat->dev = lowerstat.dev;
> > +                       } else {
> > +                               ufs = dentry->d_sb->s_fs_info;
> > +                               stat->dev = get_lowerdir_pseudo_dev(ufs,
> > +                                                               lowerstat.dev);
> 
> if (!is_dir && !samefs) { stat->dev = ovl_get_pseudo_dev ... }
> should be outside the OVL_TYPE_ORIGIN condition, because
> it also applies to pure lower and pure upper inodes (not copied up).
> 
> For pure upper it is just nicer and du -xs will work better on
> pure upper dirs.
> For lower, it is a must, because overlay inode needs to preserve
> st_dev across copy up, so it needs to have the pseudo lower st_dev
> before that lower becomes the origin of a copy up.

I will fix this.
> 
> 
> > +                       }
> >                 }
> >                 if (samefs)
> >                         stat->dev = dentry->d_sb->s_dev;
> 
> 


-- 
chandan