Messages in this thread | | | From | Linus Torvalds <> | Date | Thu, 9 Jun 2022 18:18:27 -0700 | Subject | Re: [PATCH] s390: disable -Warray-bounds |
| |
On Thu, Jun 9, 2022 at 4:59 PM Dave Chinner <dchinner@redhat.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
| |