Re: [PATCH V2] ovl: Allocate anonymous devs for lowerdirs

[Chandan Rajendra <chandan@linux.vnet.ibm.com>]

On Friday, July 14, 2017 3:08:31 PM IST Miklos Szeredi wrote:
> On Fri, Jun 23, 2017 at 1:01 PM, Chandan Rajendra
> <chandan@linux.vnet.ibm.com> wrote:
> > For stat(2) on lowerdir non-dir entries in non-samefs case, this commit
> > provides unique values for st_dev. The unique values are obtained by
> > allocating anonymous bdevs for each of the lowerdirs in the overlayfs
> > instance.
> >
> > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> > ---
> > Changelog:
> > v1->v2: Drop code that provided unique st_dev across copy up.
> >
> >  fs/overlayfs/inode.c     | 20 ++++++++++++++++++++
> >  fs/overlayfs/ovl_entry.h |  8 +++++++-
> >  fs/overlayfs/super.c     | 33 +++++++++++++++++++++++++--------
> >  3 files changed, 52 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index d613e2c..e30bdca 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -8,11 +8,29 @@
> >   */
> >
> >  #include <linux/fs.h>
> > +#include <linux/mount.h>
> >  #include <linux/slab.h>
> >  #include <linux/cred.h>
> >  #include <linux/xattr.h>
> >  #include <linux/posix_acl.h>
> >  #include "overlayfs.h"
> > +#include "ovl_entry.h"
> > +
> > +static dev_t ovl_get_pseudo_dev(struct dentry *dentry, dev_t dev)
> > +{
> > +       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > +       int i;
> > +
> > +       if (ofs->upper_mnt && ofs->upper_mnt->mnt_sb->s_dev == dev)
> > +               return dev;
> > +
> > +       for (i = 0; i < ofs->numlower; i++) {
> > +               if (ofs->lower_mnt[i].real_dev == dev)
> > +                       return ofs->lower_mnt[i].pseudo_dev;
> > +       }
> > +
> > +       return dev;
> > +}
> 
> This is a good way to test out the functionality.  But there's a
> trivial way to optimize away the O(num of layers) from this function:
> instead of using struct path for lower stack, use
> 
> struct ovl_lower {
>         struct ovl_lower_mnt *layer;
>         struct dentry *dentry;
> };
> 
> That means one more dereference to get the vfsmount, but I don't think
> we need to care about that.
> 

Hi Miklos,

IIUC, 'struct ovl_entry' would then be,

/* private information held for every overlayfs dentry */
struct ovl_entry {
        union {
                struct {
                        unsigned long has_upper;
                        bool opaque;
                };
                struct rcu_head rcu;
        };
        unsigned numlower;
        struct ovl_lower lowerstack[];
};


... ovl_path_lower() would then be having a "struct ovl_lower *" as its
second argument.

With the above listed changes, ovl_get_pseudo_dev() can then be fed a "struct
ovl_path_lower *" and hence, as you had indicated the 'for' loop can be
replaced with a statement returning the value of ovl_lower->layer->pseudo_dev.

> >
> >  int ovl_setattr(struct dentry *dentry, struct iattr *attr)
> >  {
> > @@ -116,6 +134,8 @@ int ovl_getattr(const struct path *path, struct kstat *stat,
> >                  */
> >                 stat->dev = dentry->d_sb->s_dev;
> >                 stat->ino = dentry->d_inode->i_ino;
> > +       } else {
> > +               stat->dev = ovl_get_pseudo_dev(dentry, stat->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;
> 
> Why store the real_dev?  It's stored in mnt->mnt_sb->s_dev, right?

You are right. I will fix it up in the next version of this patch.

Thanks for your review comments.
> 
> > +       dev_t pseudo_dev;
> > +};
> > +


-- 
chandan