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

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2022-03-09 at 19:23 +0000, David Howells wrote:
> Jeff Layton <jlayton@kernel.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? ;-)
> 

Sort of, I guess.  The natural way to enforce the requirement that the
inode and context be ordered and adjacent like that is to make a struct
that embeds them both.

My thinking was that someone at some point will try to move things
around if they're just adjacent like this rather than an encapsulated
"object".

If we go this route, then please leave some comments in each filesystem
warning people off from breaking them up.

> > 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.
> 

Good point. Looks like around 200 or so places that would need to change
in the affected filesystems.

> > > +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.
> 

Ok.

-- 
Jeff Layton <jlayton@kernel.org>