Jeff Layton <jlayton@xxxxxxxxxx> 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
