[lkml]   [2022]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2 12/19] netfs: Add a netfs inode context
Jeff Layton <> 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.


 \ /
  Last update: 2022-03-09 20:25    [W:0.106 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site