Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 21 May 2023 10:08:55 +0200
From:   Andrea Righi <andrea.righi@canonical.com>
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     Miklos Szeredi <miklos@szeredi.hu>, linux-unionfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] ovl: make consistent use of OVL_FS()
Message-ID: <ZGnRl7gvto2KvpWr@righiandr-XPS-13-7390>
References: <20230520184114.77725-1-andrea.righi@canonical.com>
 <20230520184114.77725-3-andrea.righi@canonical.com>
 <CAOQ4uxh-ApuxzCG57BeSDveg34LQWD48xKHKO9vCX=5NTn647A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAOQ4uxh-ApuxzCG57BeSDveg34LQWD48xKHKO9vCX=5NTn647A@mail.gmail.com>
Precedence: bulk

On Sun, May 21, 2023 at 09:51:46AM +0300, Amir Goldstein wrote:
> On Sat, May 20, 2023 at 9:41 PM Andrea Righi <andrea.righi@canonical.com> wrote:
> >
> > Always use OVL_FS() to retrieve the corresponding struct ovl_fs from a
> > struct super_block.
> >
> > Moreover, make sure that it is exclusively used with an overlayfs
> > superblock when CONFIG_OVERLAY_FS_DEBUG is enabled (otherwise trigger a
> > WARN_ON_ONCE).
> 
> Seems that you do not mind learning how we usually do things...
> so "Moreover", "Also" is usually an indication that this change needs to be
> in a separate patch.
> I think this is one of those cases.

OK, I'll split the "moreover" part in a separate patch. :)

> 
> >
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> > ---
> >  fs/overlayfs/copy_up.c   |  2 +-
> >  fs/overlayfs/export.c    | 10 +++++-----
> >  fs/overlayfs/inode.c     |  8 ++++----
> >  fs/overlayfs/namei.c     |  2 +-
> >  fs/overlayfs/ovl_entry.h | 16 ++++++++++++++++
> >  fs/overlayfs/super.c     | 12 ++++++------
> >  fs/overlayfs/util.c      | 18 +++++++++---------
> >  7 files changed, 42 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index f658cc8ea492..60aa615820e7 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -905,7 +905,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
> >  static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
> >                                   int flags)
> >  {
> > -       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> >
> >         if (!ofs->config.metacopy)
> >                 return false;
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index defd4e231ad2..f5f0ef8e3ce8 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -182,7 +182,7 @@ static int ovl_connect_layer(struct dentry *dentry)
> >   */
> >  static int ovl_check_encode_origin(struct dentry *dentry)
> >  {
> > -       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> >
> >         /* Upper file handle for pure upper */
> >         if (!ovl_dentry_lower(dentry))
> > @@ -434,7 +434,7 @@ static struct dentry *ovl_lookup_real_inode(struct super_block *sb,
> >                                             struct dentry *real,
> >                                             const struct ovl_layer *layer)
> >  {
> > -       struct ovl_fs *ofs = sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(sb);
> >         struct dentry *index = NULL;
> >         struct dentry *this = NULL;
> >         struct inode *inode;
> > @@ -655,7 +655,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
> >                                      struct ovl_path *lowerpath,
> >                                      struct dentry *index)
> >  {
> > -       struct ovl_fs *ofs = sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(sb);
> >         const struct ovl_layer *layer = upper ? &ofs->layers[0] : lowerpath->layer;
> >         struct dentry *real = upper ?: (index ?: lowerpath->dentry);
> >
> > @@ -680,7 +680,7 @@ static struct dentry *ovl_get_dentry(struct super_block *sb,
> >  static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
> >                                         struct ovl_fh *fh)
> >  {
> > -       struct ovl_fs *ofs = sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(sb);
> >         struct dentry *dentry;
> >         struct dentry *upper;
> >
> > @@ -700,7 +700,7 @@ static struct dentry *ovl_upper_fh_to_d(struct super_block *sb,
> >  static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
> >                                         struct ovl_fh *fh)
> >  {
> > -       struct ovl_fs *ofs = sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(sb);
> >         struct ovl_path origin = { };
> >         struct ovl_path *stack = &origin;
> >         struct dentry *dentry = NULL;
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 541cf3717fc2..c27823f6e7aa 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -334,7 +334,7 @@ static const char *ovl_get_link(struct dentry *dentry,
> >
> >  bool ovl_is_private_xattr(struct super_block *sb, const char *name)
> >  {
> > -       struct ovl_fs *ofs = sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(sb);
> >
> >         if (ofs->config.userxattr)
> >                 return strncmp(name, OVL_XATTR_USER_PREFIX,
> > @@ -689,7 +689,7 @@ int ovl_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
> >  int ovl_update_time(struct inode *inode, struct timespec64 *ts, int flags)
> >  {
> >         if (flags & S_ATIME) {
> > -               struct ovl_fs *ofs = inode->i_sb->s_fs_info;
> > +               struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> >                 struct path upperpath = {
> >                         .mnt = ovl_upper_mnt(ofs),
> >                         .dentry = ovl_upperdentry_dereference(OVL_I(inode)),
> > @@ -952,7 +952,7 @@ static inline void ovl_lockdep_annotate_inode_mutex_key(struct inode *inode)
> >
> >  static void ovl_next_ino(struct inode *inode)
> >  {
> > -       struct ovl_fs *ofs = inode->i_sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(inode->i_sb);
> >
> >         inode->i_ino = atomic_long_inc_return(&ofs->last_ino);
> >         if (unlikely(!inode->i_ino))
> > @@ -1284,7 +1284,7 @@ struct inode *ovl_get_trap_inode(struct super_block *sb, struct dentry *dir)
> >  static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
> >                              struct dentry *lower, bool index)
> >  {
> > -       struct ovl_fs *ofs = sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(sb);
> >
> >         /* No, if pure upper */
> >         if (!lower)
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index cfb3420b7df0..d0f196b85541 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -832,7 +832,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >  {
> >         struct ovl_entry *oe;
> >         const struct cred *old_cred;
> > -       struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
> > +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> >         struct ovl_entry *poe = dentry->d_parent->d_fsdata;
> >         struct ovl_entry *roe = dentry->d_sb->s_root->d_fsdata;
> >         struct ovl_path *stack = NULL, *origin_path = NULL;
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index fd11fe6d6d45..0b93b1d9ad79 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -95,8 +95,24 @@ static inline struct mnt_idmap *ovl_upper_mnt_idmap(struct ovl_fs *ofs)
> >         return mnt_idmap(ovl_upper_mnt(ofs));
> >  }
> >
> > +extern struct file_system_type ovl_fs_type;
> > +
> > +#ifdef CONFIG_OVERLAY_FS_DEBUG
> > +static inline bool is_ovl_fs_sb(struct super_block *sb)
> > +{
> > +       return sb->s_type == &ovl_fs_type;
> > +}
> > +#else
> > +static inline bool is_ovl_fs_sb(struct super_block *sb)
> > +{
> > +       return true;
> > +}
> > +#endif
> > +
> >  static inline struct ovl_fs *OVL_FS(struct super_block *sb)
> >  {
> > +       WARN_ON_ONCE(!is_ovl_fs_sb(sb));
> > +
> >         return (struct ovl_fs *)sb->s_fs_info;
> >  }
> >
> 
> IMO, is_ovl_fs_sb() is useful and no reason to hide it under
> CONFIG_OVERLAY_FS_DEBUG.
> IMO, only the fortification of OVL_FS() needs to be hidden inside
> CONFIG_OVERLAY_FS_DEBUG.
> 
> With those minor comments fixed you may add:
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Makes sense. Will send a v3 with your changes.

Thanks for the review!
-Andrea