On Sat, 2023-04-08 at 19:43 +0300, Amir Goldstein wrote:
> The lower stacks of all the ovl inode aliases should be identical
> and there is redundant information in ovl_entry and ovl_inode.
>
> Move lowerstack into ovl_inode and keep only the OVL_E_FLAGS
> per overlay dentry.
>
> Following patches will deduplicate redundant ovl_inode fields.
>
> Note that for a negative dentry, OVL_E(dentry) can now be NULL,
> so it is imporatnt to use the ovl_numlower() accessor.
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
Reviewed-by: Alexander Larsson <alexl@xxxxxxxxxx>
> ---
> fs/overlayfs/dir.c | 2 +-
> fs/overlayfs/export.c | 22 ++++++++++++----------
> fs/overlayfs/inode.c | 8 ++++----
> fs/overlayfs/namei.c | 5 ++---
> fs/overlayfs/overlayfs.h | 6 ++++--
> fs/overlayfs/ovl_entry.h | 36 ++++++++++++++++++------------------
> fs/overlayfs/super.c | 18 ++++--------------
> fs/overlayfs/util.c | 8 ++++----
> 8 files changed, 49 insertions(+), 56 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 9be52d8013c8..92bdcedfaaec 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -269,7 +269,7 @@ static int ovl_instantiate(struct dentry *dentry,
> struct inode *inode,
>
> ovl_dir_modified(dentry->d_parent, false);
> ovl_dentry_set_upper_alias(dentry);
> - ovl_dentry_init_reval(dentry, newdentry);
> + ovl_dentry_init_reval(dentry, newdentry, NULL);
>
> if (!hardlink) {
> /*
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index ddb546627749..d4caf57c8e17 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -286,7 +286,7 @@ static struct dentry *ovl_obtain_alias(struct
> super_block *sb,
> struct dentry *lower = lowerpath ? lowerpath->dentry : NULL;
> struct dentry *upper = upper_alias ?: index;
> struct dentry *dentry;
> - struct inode *inode;
> + struct inode *inode = NULL;
> struct ovl_entry *oe;
> struct ovl_inode_params oip = {
> .lowerpath = lowerpath,
> @@ -298,9 +298,19 @@ static struct dentry *ovl_obtain_alias(struct
> super_block *sb,
> if (d_is_dir(upper ?: lower))
> return ERR_PTR(-EIO);
>
> + oe = ovl_alloc_entry(!!lower);
> + if (!oe)
> + goto nomem;
You goto nomem here, but that will dput(dentry), and dentry is not
initialized yet. I think you should just return an error directly here.
> +
> oip.upperdentry = dget(upper);
> + if (lower) {
> + ovl_lowerstack(oe)->dentry = dget(lower);
> + ovl_lowerstack(oe)->layer = lowerpath->layer;
> + }
> + oip.oe = oe;
> inode = ovl_get_inode(sb, &oip);
> if (IS_ERR(inode)) {
> + ovl_free_entry(oe);
> dput(upper);
> return ERR_CAST(inode);
> }
> @@ -315,19 +325,11 @@ static struct dentry *ovl_obtain_alias(struct
> super_block *sb,
> dentry = d_alloc_anon(inode->i_sb);
> if (unlikely(!dentry))
> goto nomem;
> - oe = ovl_alloc_entry(lower ? 1 : 0);
> - if (!oe)
> - goto nomem;
>
> - if (lower) {
> - ovl_lowerstack(oe)->dentry = dget(lower);
> - ovl_lowerstack(oe)->layer = lowerpath->layer;
> - }
> - dentry->d_fsdata = oe;
> if (upper_alias)
> ovl_dentry_set_upper_alias(dentry);
>
> - ovl_dentry_init_reval(dentry, upper);
> + ovl_dentry_init_reval(dentry, upper, OVL_I_E(inode));
>
> return d_instantiate_anon(dentry, inode);
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 541cf3717fc2..c296bd656858 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -1003,8 +1003,9 @@ void ovl_inode_init(struct inode *inode, struct
> ovl_inode_params *oip,
> struct inode *realinode;
> struct ovl_inode *oi = OVL_I(inode);
>
> - if (oip->upperdentry)
> - oi->__upperdentry = oip->upperdentry;
> + oi->__upperdentry = oip->upperdentry;
> + oi->oe = oip->oe;
> + oi->redirect = oip->redirect;
> if (oip->lowerpath && oip->lowerpath->dentry) {
> oi->lowerpath.dentry = dget(oip->lowerpath->dentry);
> oi->lowerpath.layer = oip->lowerpath->layer;
> @@ -1369,6 +1370,7 @@ struct inode *ovl_get_inode(struct super_block
> *sb,
> }
>
> dput(upperdentry);
> + ovl_free_entry(oip->oe);
> kfree(oip->redirect);
> goto out;
> }
> @@ -1398,8 +1400,6 @@ struct inode *ovl_get_inode(struct super_block
> *sb,
> if (oip->index)
> ovl_set_flag(OVL_INDEX, inode);
>
> - OVL_I(inode)->redirect = oip->redirect;
> -
> if (bylower)
> ovl_set_flag(OVL_CONST_INO, inode);
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index c237c8dbff09..a0a1c498dbd1 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1073,7 +1073,6 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
> goto out_put;
>
> ovl_stack_cpy(ovl_lowerstack(oe), stack, ctr);
> - dentry->d_fsdata = oe;
>
> if (upperopaque)
> ovl_dentry_set_opaque(dentry);
> @@ -1107,6 +1106,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
> struct ovl_inode_params oip = {
> .upperdentry = upperdentry,
> .lowerpath = stack,
> + .oe = oe,
> .index = index,
> .numlower = ctr,
> .redirect = upperredirect,
> @@ -1122,7 +1122,7 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
> ovl_set_flag(OVL_UPPERDATA, inode);
> }
>
> - ovl_dentry_init_reval(dentry, upperdentry);
> + ovl_dentry_init_reval(dentry, upperdentry, OVL_I_E(inode));
>
> revert_creds(old_cred);
> if (origin_path) {
> @@ -1135,7 +1135,6 @@ struct dentry *ovl_lookup(struct inode *dir,
> struct dentry *dentry,
> return d_splice_alias(inode, dentry);
>
> out_free_oe:
> - dentry->d_fsdata = NULL;
> ovl_free_entry(oe);
> out_put:
> dput(index);
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 6a50296fef8f..e14ca0fd1063 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -381,9 +381,10 @@ struct ovl_entry *ovl_alloc_entry(unsigned int
> numlower);
> void ovl_free_entry(struct ovl_entry *oe);
> bool ovl_dentry_remote(struct dentry *dentry);
> void ovl_dentry_update_reval(struct dentry *dentry, struct dentry
> *realdentry);
> -void ovl_dentry_init_reval(struct dentry *dentry, struct dentry
> *upperdentry);
> +void ovl_dentry_init_reval(struct dentry *dentry, struct dentry
> *upperdentry,
> + struct ovl_entry *oe);
> void ovl_dentry_init_flags(struct dentry *dentry, struct dentry
> *upperdentry,
> - unsigned int mask);
> + struct ovl_entry *oe, unsigned int mask);
> bool ovl_dentry_weird(struct dentry *dentry);
> enum ovl_path_type ovl_path_type(struct dentry *dentry);
> void ovl_path_upper(struct dentry *dentry, struct path *path);
> @@ -653,6 +654,7 @@ struct ovl_inode_params {
> struct inode *newinode;
> struct dentry *upperdentry;
> struct ovl_path *lowerpath;
> + struct ovl_entry *oe;
> bool index;
> unsigned int numlower;
> char *redirect;
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index 754f8ae4ce62..201de9da45d3 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -47,6 +47,11 @@ struct ovl_path {
> struct dentry *dentry;
> };
>
> +struct ovl_entry {
> + unsigned int __numlower;
> + struct ovl_path __lowerstack[];
> +};
> +
> /* private information held for overlayfs's superblock */
> struct ovl_fs {
> unsigned int numlayer;
> @@ -105,18 +110,6 @@ static inline bool ovl_should_sync(struct ovl_fs
> *ofs)
> return !ofs->config.ovl_volatile;
> }
>
> -/* private information held for every overlayfs dentry */
> -struct ovl_entry {
> - union {
> - struct {
> - unsigned long flags;
> - };
> - struct rcu_head rcu;
> - };
> - unsigned int __numlower;
> - struct ovl_path __lowerstack[];
> -};
> -
> static inline unsigned int ovl_numlower(struct ovl_entry *oe)
> {
> return oe ? oe->__numlower : 0;
> @@ -127,14 +120,10 @@ static inline struct ovl_path
> *ovl_lowerstack(struct ovl_entry *oe)
> return oe ? oe->__lowerstack : NULL;
> }
>
> -static inline struct ovl_entry *OVL_E(struct dentry *dentry)
> -{
> - return (struct ovl_entry *) dentry->d_fsdata;
> -}
> -
> +/* private information held for every overlayfs dentry */
> static inline unsigned long *OVL_E_FLAGS(struct dentry *dentry)
> {
> - return &OVL_E(dentry)->flags;
> + return (unsigned long *) &dentry->d_fsdata;
> }
>
> struct ovl_inode {
> @@ -148,6 +137,7 @@ struct ovl_inode {
> struct inode vfs_inode;
> struct dentry *__upperdentry;
> struct ovl_path lowerpath;
> + struct ovl_entry *oe;
>
> /* synchronize copy up and more */
> struct mutex lock;
> @@ -158,6 +148,16 @@ static inline struct ovl_inode *OVL_I(struct
> inode *inode)
> return container_of(inode, struct ovl_inode, vfs_inode);
> }
>
> +static inline struct ovl_entry *OVL_I_E(struct inode *inode)
> +{
> + return inode ? OVL_I(inode)->oe : NULL;
> +}
> +
> +static inline struct ovl_entry *OVL_E(struct dentry *dentry)
> +{
> + return OVL_I_E(d_inode(dentry));
> +}
> +
> static inline struct dentry *ovl_upperdentry_dereference(struct
> ovl_inode *oi)
> {
> return READ_ONCE(oi->__upperdentry);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index d8fe857bd7e1..b9e62ccd609f 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -59,16 +59,6 @@ module_param_named(metacopy, ovl_metacopy_def,
> bool, 0644);
> MODULE_PARM_DESC(metacopy,
> "Default to on or off for the metadata only copy up
> feature");
>
> -static void ovl_dentry_release(struct dentry *dentry)
> -{
> - struct ovl_entry *oe = dentry->d_fsdata;
> -
> - if (oe) {
> - ovl_stack_put(ovl_lowerstack(oe), ovl_numlower(oe));
> - kfree_rcu(oe, rcu);
> - }
> -}
> -
> static struct dentry *ovl_d_real(struct dentry *dentry,
> const struct inode *inode)
> {
> @@ -162,7 +152,6 @@ static int ovl_dentry_weak_revalidate(struct
> dentry *dentry, unsigned int flags)
> }
>
> static const struct dentry_operations ovl_dentry_operations = {
> - .d_release = ovl_dentry_release,
> .d_real = ovl_d_real,
> .d_revalidate = ovl_dentry_revalidate,
> .d_weak_revalidate = ovl_dentry_weak_revalidate,
> @@ -182,6 +171,7 @@ static struct inode *ovl_alloc_inode(struct
> super_block *sb)
> oi->version = 0;
> oi->flags = 0;
> oi->__upperdentry = NULL;
> + oi->oe = NULL;
> oi->lowerpath.dentry = NULL;
> oi->lowerpath.layer = NULL;
> oi->lowerdata = NULL;
> @@ -205,6 +195,7 @@ static void ovl_destroy_inode(struct inode
> *inode)
>
> dput(oi->__upperdentry);
> dput(oi->lowerpath.dentry);
> + ovl_free_entry(oi->oe);
> if (S_ISDIR(inode->i_mode))
> ovl_dir_cache_free(inode);
> else
> @@ -1857,14 +1848,13 @@ static struct dentry *ovl_get_root(struct
> super_block *sb,
> struct ovl_inode_params oip = {
> .upperdentry = upperdentry,
> .lowerpath = lowerpath,
> + .oe = oe,
> };
>
> root = d_make_root(ovl_new_inode(sb, S_IFDIR, 0));
> if (!root)
> return NULL;
>
> - root->d_fsdata = oe;
> -
> if (upperdentry) {
> /* Root inode uses upper st_ino/i_ino */
> ino = d_inode(upperdentry)->i_ino;
> @@ -1879,7 +1869,7 @@ static struct dentry *ovl_get_root(struct
> super_block *sb,
> ovl_dentry_set_flag(OVL_E_CONNECTED, root);
> ovl_set_upperdata(d_inode(root));
> ovl_inode_init(d_inode(root), &oip, ino, fsid);
> - ovl_dentry_init_flags(root, upperdentry,
> DCACHE_OP_WEAK_REVALIDATE);
> + ovl_dentry_init_flags(root, upperdentry, oe,
> DCACHE_OP_WEAK_REVALIDATE);
>
> return root;
> }
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 1ba6dbea808c..f5e2c70a57f8 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -145,15 +145,15 @@ void ovl_dentry_update_reval(struct dentry
> *dentry, struct dentry *realdentry)
> spin_unlock(&dentry->d_lock);
> }
>
> -void ovl_dentry_init_reval(struct dentry *dentry, struct dentry
> *upperdentry)
> +void ovl_dentry_init_reval(struct dentry *dentry, struct dentry
> *upperdentry,
> + struct ovl_entry *oe)
> {
> - return ovl_dentry_init_flags(dentry, upperdentry,
> OVL_D_REVALIDATE);
> + return ovl_dentry_init_flags(dentry, upperdentry, oe,
> OVL_D_REVALIDATE);
> }
>
> void ovl_dentry_init_flags(struct dentry *dentry, struct dentry
> *upperdentry,
> - unsigned int mask)
> + struct ovl_entry *oe, unsigned int mask)
> {
> - struct ovl_entry *oe = OVL_E(dentry);
> struct ovl_path *lowerstack = ovl_lowerstack(oe);
> unsigned int i, flags = 0;
>
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
Alexander Larsson Red Hat,
Inc
alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
He's a notorious small-town vagrant haunted by an iconic dead American
confidante She's a radical junkie research scientist who don't take no
shit from nobody. They fight crime!
