Message-ID: <YqKJXf6dif2emugf@rh>
Date:   Fri, 10 Jun 2022 09:59:25 +1000
From:   Dave Chinner <dchinner@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     David Howells <dhowells@...hat.com>,
        Kees Cook <keescook@...omium.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] s390: disable -Warray-bounds

On Thu, Jun 09, 2022 at 11:20:02AM -0700, Linus Torvalds wrote:
> On Thu, Jun 9, 2022 at 7:14 AM David Howells <dhowells@...hat.com> wrote:
> >
> > Note that Dave Chinner would rather I converted code like:
> >
> >         struct myfs_inode *myfsinode = xyz;
> >         myfsinode->netfs.inode.i_ino = 123;
> >
> > to something like:
> >
> >         struct myfs_inode *myfsinode = xyz;
> >         struct inode *inode = VFS_I(myfsinode);
> >         inode->i_ino = 123;
> >
> > where the translation is wrapped inside a VFS_I() macro in every filesystem
> > and wants this across all filesystems.
> 
> What? No. That's absolutely disgusting.
> 
> Maybe I'm mis-undestanding.

Perhaps, because I think what I said looks very different when taken
out of context.

I saw a heap of different implementations of the same thing with no
consistency across them (i.e. inode container definitions) and a
mess of a patch to convert them without solving the problem that
there's no consistent convention for doing filesystem inode -> VFS
inode container conversion

> The usual way filesystems should handle this is that they have their
> own inode information that contains a 'struct inode', and then they
> have an inline function to go from that generic VFS inode to their one
> using "container_of()".
> 
> And yeah, maybe they call that container_of() thing MYINODE() or
> something, although I think an inline function without the ugly
> all-uppercase is right.

Right, BTRFS_I(), EXT4_I(), F2FS_I(), AFS_FS_I(), P9FS_I(), etc.

It's a convention, it dates back to macro days (hence upper case
even though most are static inlines these days), and it obvious no
matter what filesystem code I read that when I see this XXX_I(inode)
convention I know the code is accessing the filesystem inode in the
container, not the VFS indoe.

> But the way they go the other way is literally to just dereference the
> inode that they have, ie they just use a
> 
>         if (S_ISREG(inode->vfs_inode.i_mode)) ..

The problem with this is that we have very similar names in both the
VFS inode and the filesysetm inodes (e.g. i_flags), and without a
clear demarcation of which inode is being referenced it can lead to
confusion and bugs.

> kind pattern. There's no reason or excuse to try to "wrap" that, and
> it would be a big step backwards to introduce some kind of VFS_I()
> macro.

If the result of adding a helper convention is that every reverse
inode container resolution looks identical across all filesystems,
then we no longer have to know the details of the fs specific
container to get the conversion right. All the code across all the
filesystems would look the same, even though the wrapper would be
different.

We do helper conversions like this all the time to make the code
easier to read, understand and maintain, so I really don't see why
this would be considered a step backwards....

> There's also no reason to make that generic. At no point should you
> ever go from "random filesystem inode" to "actual generic VFS inode"
> in some uncontrolled manner.

We never do any conversions in an uncontrolled manner. We often need
to go from fs inode to vfs inode because we are deep in filesystem
implementation code passing around filesystem inodes, but the piece
of information we need to access is stored in the VFS inode (e.g.
uid, gid, etc). That's what this netfs inode container was requiring
in the patchset...

> But maybe Dave is talking about something else, and I'm missing the point.

Perhaps - my comment was not about the VFS_I() name or implementation;
I used it simply because I can point at code that uses it as an
example of having a symmetric, easily recognisable convention.

My point was that the fs inode to vfs inode conversion is a common
operation performed across all filesystems that lacks any
consistency in implementation. Some filesystems use a symmetric API
for these container conversions and so I was simply suggesting that
converting them all to use a common symmetric convention would
simplify the maintenance of filesystem code in future and make it
easier for other people to understand...

Cheers,

Dave.
-- 
Dave Chinner
dchinner@...hat.com

Powered by blists - more mailing lists