Re: [PATCH 5/5] ovl: implement lazy lookup of lowerdata in data-only layers

[Alexander Larsson <alexl@redhat.com>]

On Wed, 2023-04-12 at 16:54 +0300, Amir Goldstein wrote:
> Defer lookup of lowerdata in the data-only layers to first data
> access
> or before copy up.
> 
> We perform lowerdata lookup before copy up even if copy up is
> metadata
> only copy up.  We can further optimize this lookup later if needed.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

LGTM 

Reviewed-by: Alexander Larsson <alexl@redhat.com>

> ---
>  fs/overlayfs/copy_up.c   |  9 +++++++
>  fs/overlayfs/file.c      | 18 ++++++++++---
>  fs/overlayfs/namei.c     | 56 +++++++++++++++++++++++++++++++++++---
> --
>  fs/overlayfs/overlayfs.h |  2 ++
>  fs/overlayfs/ovl_entry.h |  2 +-
>  fs/overlayfs/util.c      | 31 +++++++++++++++++++++-
>  6 files changed, 105 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 7bf101e756c8..eb266fb68730 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -1074,6 +1074,15 @@ static int ovl_copy_up_flags(struct dentry
> *dentry, int flags)
>         if (WARN_ON(disconnected && d_is_dir(dentry)))
>                 return -EIO;
>  
> +       /*
> +        * We may not need lowerdata if we are only doing metacopy
> up, but it is
> +        * not very important to optimize this case, so do lazy
> lowerdata lookup
> +        * before any copy up, so we can do it before taking
> ovl_inode_lock().
> +        */
> +       err = ovl_maybe_lookup_lowerdata(dentry);
> +       if (err)
> +               return err;
> +
>         old_cred = ovl_override_creds(dentry->d_sb);
>         while (!err) {
>                 struct dentry *next;
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 951683a66ff6..39737c2aaa84 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -107,15 +107,21 @@ static int ovl_real_fdget_meta(const struct
> file *file, struct fd *real,
>  {
>         struct dentry *dentry = file_dentry(file);
>         struct path realpath;
> +       int err;
>  
>         real->flags = 0;
>         real->file = file->private_data;
>  
> -       if (allow_meta)
> +       if (allow_meta) {
>                 ovl_path_real(dentry, &realpath);
> -       else
> +       } else {
> +               /* lazy lookup of lowerdata */
> +               err = ovl_maybe_lookup_lowerdata(dentry);
> +               if (err)
> +                       return err;
> +
>                 ovl_path_realdata(dentry, &realpath);
> -       /* TODO: lazy lookup of lowerdata */
> +       }
>         if (!realpath.dentry)
>                 return -EIO;
>  
> @@ -153,6 +159,11 @@ static int ovl_open(struct inode *inode, struct
> file *file)
>         struct path realpath;
>         int err;
>  
> +       /* lazy lookup of lowerdata */
> +       err = ovl_maybe_lookup_lowerdata(dentry);
> +       if (err)
> +               return err;
> +
>         err = ovl_maybe_copy_up(dentry, file->f_flags);
>         if (err)
>                 return err;
> @@ -161,7 +172,6 @@ static int ovl_open(struct inode *inode, struct
> file *file)
>         file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
>  
>         ovl_path_realdata(dentry, &realpath);
> -       /* TODO: lazy lookup of lowerdata */
>         if (!realpath.dentry)
>                 return -EIO;
>  
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 82e103e2308b..ba2b156162ca 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -889,6 +889,52 @@ static int ovl_fix_origin(struct ovl_fs *ofs,
> struct dentry *dentry,
>         return err;
>  }
>  
> +/* Lazy lookup of lowerdata */
> +int ovl_maybe_lookup_lowerdata(struct dentry *dentry)
> +{
> +       struct inode *inode = d_inode(dentry);
> +       const char *redirect = ovl_lowerdata_redirect(inode);
> +       struct ovl_path datapath = {};
> +       const struct cred *old_cred;
> +       int err;
> +
> +       if (!redirect || ovl_dentry_lowerdata(dentry))
> +               return 0;
> +
> +       if (redirect[0] != '/')
> +               return -EIO;
> +
> +       err = ovl_inode_lock_interruptible(inode);
> +       if (err)
> +               return err;
> +
> +       err = 0;
> +       /* Someone got here before us? */
> +       if (ovl_dentry_lowerdata(dentry))
> +               goto out;
> +
> +       old_cred = ovl_override_creds(dentry->d_sb);
> +       err = ovl_lookup_data_layers(dentry, redirect, &datapath);
> +       revert_creds(old_cred);
> +       if (err)
> +               goto out_err;
> +
> +       err = ovl_dentry_set_lowerdata(dentry, &datapath);
> +       if (err)
> +               goto out_err;
> +
> +out:
> +       ovl_inode_unlock(inode);
> +       dput(datapath.dentry);
> +
> +       return err;
> +
> +out_err:
> +       pr_warn_ratelimited("lazy lowerdata lookup failed (%pd2,
> err=%i)\n",
> +                           dentry, err);
> +       goto out;
> +}
> +
>  struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                           unsigned int flags)
>  {
> @@ -1074,14 +1120,10 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
>                 }
>         }
>  
> -       /* Lookup absolute redirect from lower metacopy in data-only
> layers */
> +       /* Defer lookup of lowerdata in data-only layers to first
> access */
>         if (d.metacopy && ctr && ofs->numdatalayer &&
> d.absolute_redirect) {
> -               err = ovl_lookup_data_layers(dentry, d.redirect,
> -                                            &stack[ctr]);
> -               if (!err) {
> -                       d.metacopy = false;
> -                       ctr++;
> -               }
> +               d.metacopy = false;
> +               ctr++;
>         }
>  
>         /*
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 011b7b466f70..4e327665c316 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -400,6 +400,7 @@ enum ovl_path_type ovl_path_realdata(struct
> dentry *dentry, struct path *path);
>  struct dentry *ovl_dentry_upper(struct dentry *dentry);
>  struct dentry *ovl_dentry_lower(struct dentry *dentry);
>  struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
> +int ovl_dentry_set_lowerdata(struct dentry *dentry, struct ovl_path
> *datapath);
>  const struct ovl_layer *ovl_i_layer_lower(struct inode *inode);
>  const struct ovl_layer *ovl_layer_lower(struct dentry *dentry);
>  struct dentry *ovl_dentry_real(struct dentry *dentry);
> @@ -562,6 +563,7 @@ struct dentry *ovl_get_index_fh(struct ovl_fs
> *ofs, struct ovl_fh *fh);
>  struct dentry *ovl_lookup_index(struct ovl_fs *ofs, struct dentry
> *upper,
>                                 struct dentry *origin, bool verify);
>  int ovl_path_next(int idx, struct dentry *dentry, struct path
> *path);
> +int ovl_maybe_lookup_lowerdata(struct dentry *dentry);
>  struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                           unsigned int flags);
>  bool ovl_lower_positive(struct dentry *dentry);
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 25fabb3175cf..a7b1006c5321 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -145,7 +145,7 @@ static inline struct dentry
> *ovl_lowerdata_dentry(struct ovl_entry *oe)
>  {
>         struct ovl_path *lowerdata = ovl_lowerdata(oe);
>  
> -       return lowerdata ? lowerdata->dentry : NULL;
> +       return lowerdata ? READ_ONCE(lowerdata->dentry) : NULL;
>  }
>  
>  /* private information held for every overlayfs dentry */
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 284b5ba4fcf6..9a042768013e 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -250,7 +250,13 @@ void ovl_path_lowerdata(struct dentry *dentry,
> struct path *path)
>  
>         if (lowerdata_dentry) {
>                 path->dentry = lowerdata_dentry;
> -               path->mnt = lowerdata->layer->mnt;
> +               /*
> +                * Pairs with smp_wmb() in
> ovl_dentry_set_lowerdata().
> +                * Make sure that if lowerdata->dentry is visible,
> then
> +                * datapath->layer is visible as well.
> +                */
> +               smp_rmb();
> +               path->mnt = READ_ONCE(lowerdata->layer)->mnt;
>         } else {
>                 *path = (struct path) { };
>         }
> @@ -312,6 +318,29 @@ struct dentry *ovl_dentry_lowerdata(struct
> dentry *dentry)
>         return ovl_lowerdata_dentry(OVL_E(dentry));
>  }
>  
> +int ovl_dentry_set_lowerdata(struct dentry *dentry, struct ovl_path
> *datapath)
> +{
> +       struct ovl_entry *oe = OVL_E(dentry);
> +       struct ovl_path *lowerdata = ovl_lowerdata(oe);
> +       struct dentry *datadentry = datapath->dentry;
> +
> +       if (WARN_ON_ONCE(ovl_numlower(oe) <= 1))
> +               return -EIO;
> +
> +       WRITE_ONCE(lowerdata->layer, datapath->layer);
> +       /*
> +        * Pairs with smp_rmb() in ovl_path_lowerdata().
> +        * Make sure that if lowerdata->dentry is visible, then
> +        * lowerdata->layer is visible as well.
> +        */
> +       smp_wmb();
> +       WRITE_ONCE(lowerdata->dentry, dget(datadentry));
> +
> +       ovl_dentry_update_reval(dentry, datadentry);
> +
> +       return 0;
> +}
> +
>  struct dentry *ovl_dentry_real(struct dentry *dentry)
>  {
>         return ovl_dentry_upper(dentry) ?: ovl_dentry_lower(dentry);

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an unconventional arachnophobic ex-con fleeing from a secret 
government programme. She's a strong-willed motormouth politician from 
out of town. They fight crime!