Re: [PATCH] netfs: Fix gcc-12 warning by embedding vfs inode in netfs_i_context

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 4292 bytes --]

On Thu, Jun 9, 2022 at 1:46 PM David Howells <dhowells@redhat.com> wrote:
>
>         struct my_inode {
> -               struct {
> -                       /* These must be contiguous */
> -                       struct inode            vfs_inode;
> -                       struct netfs_i_context  netfs_ctx;
> -               };
> +               struct netfs_inode netfs; /* Netfslib context and vfs inode */
>                 ...

Side note: I think this could have been done with an unnamed union as

        struct my_inode {
                union {
                        struct inode            vfs_inode;
                        struct netfs_inode netfs_inode;
                };
        [...]

instead, with the rule that 'netfs_inode' always starts with a 'struct inode'.

The advantage would have been that the old 'vfs_inode' syntax would
have continued to work, and much less of the ugliness.

So a fair amount of the noise in this patch could have been avoided.

That said, I think the end result is fine (and maybe less complicated
than using that union trick), so that's not the big deal.

But what I actually *really* detest in this patch is that

        struct netfs_inode *ctx = netfs_inode(file_inode(file));

pattern.

In several cases that's just a different syntax for almost the same
problem that gcc-12 already complained about.

And yes, in some cases you do need it - particularly in the
"mapping->host" situation, you really have lost sight of the fact that
you really have a "struct netfs_inode *", and all you have is the
"struct inode *".

But in a lot of cases you really could do so much better: you *have* a
"struct netfs_inode" to begin with, but you converted it to just
"struct inode *", and now you're converting it back.

Look at that AFS code, for example, where we have afs_vnode_cache() doing

        return netfs_i_cookie(&vnode->netfs.inode);

and look how it *had* a netfs structure, and it was passing it to a
netfs function, but it explicitly passed the WRONG TYPE, so now we've
lost the type information and it is using that cast to fake it all
back.

So I think the 'netfs' functions should take a 'struct netfs_inode
*ctx' as their argument.

Because the callers know what kind of inode they have, and they can -
and should - then pass the proper netfs context down.

IOW, I think you really should do something like the attached on top
of this all.

Only *very* lightly build-tested, but let me quote part of the diff to explain:

  -static inline struct fscache_cookie *netfs_i_cookie(struct inode *inode)
  +static inline struct fscache_cookie *netfs_i_cookie(struct netfs_inode *ctx)
   {
   #if IS_ENABLED(CONFIG_FSCACHE)
  -       struct netfs_inode *ctx = netfs_inode(inode);
          return ctx->cache;
   #else


look, this part is obvious. If you are doing a "netfs_i_cookie()"
call, you had *better* know that you actually have a netfs_inode, not
some random "inode".

And most of the users already knew exactly that, so other paths of the
patch actually get cleaner too:

  -       return netfs_i_cookie(&v9inode->netfs.inode);
  +       return netfs_i_cookie(&v9inode->netfs);

but even when that wasn't the case, as in netfs_inode_init() use, we have:

   static void v9fs_set_netfs_context(struct inode *inode)
   {
  -       netfs_inode_init(inode, &v9fs_req_ops);
  +       struct v9fs_inode *v9inode = V9FS_I(inode);
  +       netfs_inode_init(&v9inode->netfs, &v9fs_req_ops);
   }

and now we're basically doing that same "taek inode pointer, convert
it to someting else" that I'm complaining about wrt the netfs code,
but notice how we are now doing it within the context of the 9p
filesystem.

So now we're converting not a 'random inode pointer that could come
from many different filesystems', but an *actual* well-defined 'this
is a 9p inode, so doing that V9FS_I(inode) conversion is normal' kind
of situation.

And at that point, we now have that 'struct netfs_inode' directly, and
don't need to play any other games.

Yes, a few 'netfs_inode()' users still remain. I don't love them
either, but they tend to be places where we really did get just the
raw inode pointer from the VFS layer (eg netfs_readahead is just used
directly as the ".readahead" function for filesystems).

Hmm?

                    Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 8703 bytes --]

 fs/9p/v9fs.h             |  2 +-
 fs/9p/vfs_addr.c         |  2 +-
 fs/9p/vfs_inode.c        |  3 ++-
 fs/afs/dynroot.c         |  2 +-
 fs/afs/inode.c           |  2 +-
 fs/afs/internal.h        |  2 +-
 fs/afs/write.c           |  2 +-
 fs/ceph/addr.c           |  3 ++-
 fs/ceph/cache.h          |  2 +-
 fs/ceph/inode.c          |  2 +-
 fs/cifs/fscache.h        |  2 +-
 fs/netfs/buffered_read.c |  4 ++--
 include/linux/netfs.h    | 16 ++++++----------
 13 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/fs/9p/v9fs.h b/fs/9p/v9fs.h
index 1b219c21d15e..6acabc2e7dc9 100644
--- a/fs/9p/v9fs.h
+++ b/fs/9p/v9fs.h
@@ -124,7 +124,7 @@ static inline struct v9fs_inode *V9FS_I(const struct inode *inode)
 static inline struct fscache_cookie *v9fs_inode_cookie(struct v9fs_inode *v9inode)
 {
 #ifdef CONFIG_9P_FSCACHE
-	return netfs_i_cookie(&v9inode->netfs.inode);
+	return netfs_i_cookie(&v9inode->netfs);
 #else
 	return NULL;
 #endif
diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
index 90c6c1ba03ab..c004b9a73a92 100644
--- a/fs/9p/vfs_addr.c
+++ b/fs/9p/vfs_addr.c
@@ -274,7 +274,7 @@ static int v9fs_write_begin(struct file *filp, struct address_space *mapping,
 	 * file.  We need to do this before we get a lock on the page in case
 	 * there's more than one writer competing for the same cache block.
 	 */
-	retval = netfs_write_begin(filp, mapping, pos, len, &folio, fsdata);
+	retval = netfs_write_begin(&v9inode->netfs, filp, mapping, pos, len, &folio, fsdata);
 	if (retval < 0)
 		return retval;
 
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index e660c6348b9d..419d2f3cf2c2 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -252,7 +252,8 @@ void v9fs_free_inode(struct inode *inode)
  */
 static void v9fs_set_netfs_context(struct inode *inode)
 {
-	netfs_inode_init(inode, &v9fs_req_ops);
+	struct v9fs_inode *v9inode = V9FS_I(inode);
+	netfs_inode_init(&v9inode->netfs, &v9fs_req_ops);
 }
 
 int v9fs_init_inode(struct v9fs_session_info *v9ses,
diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
index 3a5bbffdf053..d7d9402ff718 100644
--- a/fs/afs/dynroot.c
+++ b/fs/afs/dynroot.c
@@ -76,7 +76,7 @@ struct inode *afs_iget_pseudo_dir(struct super_block *sb, bool root)
 	/* there shouldn't be an existing inode */
 	BUG_ON(!(inode->i_state & I_NEW));
 
-	netfs_inode_init(inode, NULL);
+	netfs_inode_init(&vnode->netfs, NULL);
 	inode->i_size		= 0;
 	inode->i_mode		= S_IFDIR | S_IRUGO | S_IXUGO;
 	if (root) {
diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 22811e9eacf5..89630acbc2cc 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -58,7 +58,7 @@ static noinline void dump_vnode(struct afs_vnode *vnode, struct afs_vnode *paren
  */
 static void afs_set_netfs_context(struct afs_vnode *vnode)
 {
-	netfs_inode_init(&vnode->netfs.inode, &afs_req_ops);
+	netfs_inode_init(&vnode->netfs, &afs_req_ops);
 }
 
 /*
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 984b113a9107..a6f25d9e75b5 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -670,7 +670,7 @@ struct afs_vnode {
 static inline struct fscache_cookie *afs_vnode_cache(struct afs_vnode *vnode)
 {
 #ifdef CONFIG_AFS_FSCACHE
-	return netfs_i_cookie(&vnode->netfs.inode);
+	return netfs_i_cookie(&vnode->netfs);
 #else
 	return NULL;
 #endif
diff --git a/fs/afs/write.c b/fs/afs/write.c
index f80a6096d91c..2c885b22de34 100644
--- a/fs/afs/write.c
+++ b/fs/afs/write.c
@@ -60,7 +60,7 @@ int afs_write_begin(struct file *file, struct address_space *mapping,
 	 * file.  We need to do this before we get a lock on the page in case
 	 * there's more than one writer competing for the same cache block.
 	 */
-	ret = netfs_write_begin(file, mapping, pos, len, &folio, fsdata);
+	ret = netfs_write_begin(&vnode->netfs, file, mapping, pos, len, &folio, fsdata);
 	if (ret < 0)
 		return ret;
 
diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index f5f116ed1b9e..9763e7ea8148 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -1322,10 +1322,11 @@ static int ceph_write_begin(struct file *file, struct address_space *mapping,
 			    struct page **pagep, void **fsdata)
 {
 	struct inode *inode = file_inode(file);
+	struct ceph_inode_info *ci = ceph_inode(inode);
 	struct folio *folio = NULL;
 	int r;
 
-	r = netfs_write_begin(file, inode->i_mapping, pos, len, &folio, NULL);
+	r = netfs_write_begin(&ci->netfs, file, inode->i_mapping, pos, len, &folio, NULL);
 	if (r == 0)
 		folio_wait_fscache(folio);
 	if (r < 0) {
diff --git a/fs/ceph/cache.h b/fs/ceph/cache.h
index 26c6ae06e2f4..dc502daac49a 100644
--- a/fs/ceph/cache.h
+++ b/fs/ceph/cache.h
@@ -28,7 +28,7 @@ void ceph_fscache_invalidate(struct inode *inode, bool dio_write);
 
 static inline struct fscache_cookie *ceph_fscache_cookie(struct ceph_inode_info *ci)
 {
-	return netfs_i_cookie(&ci->netfs.inode);
+	return netfs_i_cookie(&ci->netfs);
 }
 
 static inline void ceph_fscache_resize(struct inode *inode, loff_t to)
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 650746b3ba99..56c53ab3618e 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -460,7 +460,7 @@ struct inode *ceph_alloc_inode(struct super_block *sb)
 	dout("alloc_inode %p\n", &ci->netfs.inode);
 
 	/* Set parameters for the netfs library */
-	netfs_inode_init(&ci->netfs.inode, &ceph_netfs_ops);
+	netfs_inode_init(&ci->netfs, &ceph_netfs_ops);
 
 	spin_lock_init(&ci->i_ceph_lock);
 
diff --git a/fs/cifs/fscache.h b/fs/cifs/fscache.h
index ab9a51d0125c..aa3b941a5555 100644
--- a/fs/cifs/fscache.h
+++ b/fs/cifs/fscache.h
@@ -61,7 +61,7 @@ void cifs_fscache_fill_coherency(struct inode *inode,
 
 static inline struct fscache_cookie *cifs_inode_cookie(struct inode *inode)
 {
-	return netfs_i_cookie(inode);
+	return netfs_i_cookie(&CIFS_I(inode)->netfs);
 }
 
 static inline void cifs_invalidate_cache(struct inode *inode, unsigned int flags)
diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
index d37e012386f3..2accba6ee9a7 100644
--- a/fs/netfs/buffered_read.c
+++ b/fs/netfs/buffered_read.c
@@ -326,12 +326,12 @@ static bool netfs_skip_folio_read(struct folio *folio, loff_t pos, size_t len,
  *
  * This is usable whether or not caching is enabled.
  */
-int netfs_write_begin(struct file *file, struct address_space *mapping,
+int netfs_write_begin(struct netfs_inode *ctx,
+		      struct file *file, struct address_space *mapping,
 		      loff_t pos, unsigned int len, struct folio **_folio,
 		      void **_fsdata)
 {
 	struct netfs_io_request *rreq;
-	struct netfs_inode *ctx = netfs_inode(file_inode(file ));
 	struct folio *folio;
 	unsigned int fgp_flags = FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE;
 	pgoff_t index = pos >> PAGE_SHIFT;
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index 6dbb4c9ce50d..d0e60b2ab5bc 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -277,7 +277,8 @@ struct netfs_cache_ops {
 struct readahead_control;
 extern void netfs_readahead(struct readahead_control *);
 int netfs_read_folio(struct file *, struct folio *);
-extern int netfs_write_begin(struct file *, struct address_space *,
+extern int netfs_write_begin(struct netfs_inode *,
+			     struct file *, struct address_space *,
 			     loff_t, unsigned int, struct folio **,
 			     void **);
 
@@ -308,13 +309,11 @@ static inline struct netfs_inode *netfs_inode(struct inode *inode)
  * Initialise the netfs library context struct.  This is expected to follow on
  * directly from the VFS inode struct.
  */
-static inline void netfs_inode_init(struct inode *inode,
+static inline void netfs_inode_init(struct netfs_inode *ctx,
 				    const struct netfs_request_ops *ops)
 {
-	struct netfs_inode *ctx = netfs_inode(inode);
-
 	ctx->ops = ops;
-	ctx->remote_i_size = i_size_read(inode);
+	ctx->remote_i_size = i_size_read(&ctx->inode);
 #if IS_ENABLED(CONFIG_FSCACHE)
 	ctx->cache = NULL;
 #endif
@@ -327,10 +326,8 @@ static inline void netfs_inode_init(struct inode *inode,
  *
  * Inform the netfs lib that a file got resized so that it can adjust its state.
  */
-static inline void netfs_resize_file(struct inode *inode, loff_t new_i_size)
+static inline void netfs_resize_file(struct netfs_inode *ctx, loff_t new_i_size)
 {
-	struct netfs_inode *ctx = netfs_inode(inode);
-
 	ctx->remote_i_size = new_i_size;
 }
 
@@ -340,10 +337,9 @@ static inline void netfs_resize_file(struct inode *inode, loff_t new_i_size)
  *
  * Get the caching cookie (if enabled) from the network filesystem's inode.
  */
-static inline struct fscache_cookie *netfs_i_cookie(struct inode *inode)
+static inline struct fscache_cookie *netfs_i_cookie(struct netfs_inode *ctx)
 {
 #if IS_ENABLED(CONFIG_FSCACHE)
-	struct netfs_inode *ctx = netfs_inode(inode);
 	return ctx->cache;
 #else
 	return NULL;