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