linux-kernel - Re: [PATCH v2 12/19] netfs: Add a netfs inode context

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1790300.1646853782@warthog.procyon.org.uk>
Date:   Wed, 09 Mar 2022 19:23:02 +0000
From:   David Howells <dhowells@...hat.com>
To:     Jeff Layton <jlayton@...nel.org>
Cc:     dhowells@...hat.com, linux-cachefs@...hat.com,
        Anna Schumaker <anna.schumaker@...app.com>,
        Steve French <sfrench@...ba.org>,
        Dominique Martinet <asmadeus@...ewreck.org>,
        David Wysochanski <dwysocha@...hat.com>,
        Ilya Dryomov <idryomov@...il.com>,
        Jeffle Xu <jefflexu@...ux.alibaba.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-afs@...ts.infradead.org, linux-nfs@...r.kernel.org,
        linux-cifs@...r.kernel.org, ceph-devel@...r.kernel.org,
        v9fs-developer@...ts.sourceforge.net,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 12/19] netfs: Add a netfs inode context

Jeff Layton <jlayton@...nel.org> wrote:

> > Add a netfs_i_context struct that should be included in the network
> > filesystem's own inode struct wrapper, directly after the VFS's inode
> > struct, e.g.:
> > 
> > 	struct my_inode {
> > 		struct {
> > 			struct inode		vfs_inode;
> > 			struct netfs_i_context	netfs_ctx;
> > 		};
> 
> This seems a bit klunky.
>
> I think it'd be better encapsulation to give this struct a name (e.g.
> netfs_inode) and then have the filesystems replace the embedded
> vfs_inode with a netfs_inode.

I think what you really want is:

	struct my_inode : netfs_inode {
	};

right? ;-)

> That way it's still just pointer math to get to the context from the
> inode and vice versa, but the replacement seems a bit cleaner.
> 
> It might mean a bit more churn in the filesystems themselves as you
> convert them, but most of them use macros or inline functions as
> accessors so it shouldn't be _too_ bad.

That's a lot of churn - and will definitely cause conflicts with other
patches aimed at those filesystems.  I'd prefer to avoid that if I can.

> > +static int ceph_init_request(struct netfs_io_request *rreq, struct file *file)
> > +{
> > ...
> > +}
> > +
> 
> ^^^
> The above change seems like it should be in its own patch. Wasn't it at
> one point? Converting this to use init_request doesn't seem to rely on
> the new embedded context.

Well, I wrote it as a separate patch on the end for convenience, but I
intended to merge it here otherwise ceph wouldn't be able to do readahead for
a few patches.

I was thinking that it would require the context change to work and certainly
it requires the error-return-from-init_request patch to work, but actually it
probably doesn't require the former so I could probably separate that bit out
and put it between 11 and 12.

David