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@xxxxxxxxx>
LGTM
Reviewed-by: Alexander Larsson <alexl@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx alexander.larsson@xxxxxxxxx
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!
