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

[David Howells <dhowells@redhat.com>]

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? ;-)

> 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