Message-ID: <CAHk-=whdVzniqKj160Gn5ngPLwMhvvPSEg8VE4mndbq4uj-kog@mail.gmail.com>
Date:   Thu, 9 Jun 2022 18:18:27 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Dave Chinner <dchinner@...hat.com>
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 9, 2022 at 4:59 PM Dave Chinner <dchinner@...hat.com> wrote:
>
> 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

Sure. But the thing is, they aren't actually all the same.

We do have a pattern - embed the generic vfs inode inside the
filesystem-specific one - but the exact details of how you do it isn't
fixed in stone.

And this netfs thing is actually an example of why it *shouldn't* be
fixed in stone, exactly because a netfs user doesn't want to just
"embed the vfs inode" - it wants to embed something *else* that in
turn embeds the vfs inode.

So yes, most filesystems do similar things, but they aren't exactly
the same. And they *could* be more different than they actually are
(there's nothing that says you *have* to embed the generic VFS inode
in the filesystem-specific one, it's just that we make it easy and
it's a pattern that has been copied because it works really well)

And yes, we could just enforce naming, and force everybody do use

   #define VFS_I(myino) (&(myino)->vfs_inode)

but then we really would have been in trouble with this whole netfs
embedded struct.

And no, it wouldn't be some kind of insurmountable issue, using an
unnamed union (so that "vfs_inode" would be the inode, and
"netfs_inode" would be the bigger netfs inode+context) would have made
it all work out.

But I really don't see the point of trying to just force everybody to
use the same name, and force people to use a common macro that doesn't
really *buy* you anything.

I think just writing 'inode->vfs_inode.i_mode' is very clear, and is
particularly obvious in that there's no costly translation.
'VFS_I(inode)->i_mode' might be shorter to write, but that's mainly
because of the ugly shortened macro name. If you want ugly short
names, you could have called the inode member just 'vfs_i' in the
first place.

And yes, we could go even further, and just make the rule be that
everybody should actually put the generic VFS inode struct at the
beginning of the filesystem inode. I think people do that already in
practice.

Then we could maybe use some language tricks to make the filesystems
get their own inode pointer directly as arguments, instead of getting
a 'struct inode *" and having to do that

        struct ext4_inode_info *ei = EXT4_I(inode);

at all.

I suspect we'd have to use a macro with a cast at the op assignment
time, which would be really ugly, though, but maybe there's some gcc
language extension that allows that kind of thing.

Anyway, my point is that yes, we could enforce tighter rules here, and
make everybody match some particular pattern.

But I don't think we'd actually benefit from it, and I think it would
have just caused more pain in this situation, for example.

                 Linus

Powered by blists - more mailing lists