On Tue, 15 Jul 2014 10:50:29 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Tue, Jul 15, 2014 at 08:13:34AM -0400, Jeff Layton wrote:
> > On Sun, 13 Jul 2014 05:19:19 -0700
> > Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> >
> > > On Sun, Jul 13, 2014 at 08:05:41AM -0400, Jeff Layton wrote:
> > > > It is weird, but I don't think it really matters as the filp is only
> > > > really used as a way to get to the inode -- it really doesn't matter
> > > > which struct file we use there. find_any_file will both take a
> > > > reference to and return the file, which is then eventually fput in
> > > > filp_close, so there should be no refcount leak or anything.
> > > >
> > > > The weirdness all comes from the vfs-layer file locking interfaces,
> > > > many of which take a struct file argument when they really would be
> > > > fine with a struct inode. Maybe one of these days we can get around to
> > > > cleaning that up.
> > >
> > > If filesystems get the file passed we should assume that they actually
> > > use it. In fact AFS does, but it's not NFS exportable at the moment,
> > > and ceph does in a debug printk. I'd be much happier to waste a pointer
> > > in the lock stateid to avoid this inconsistant interface. And it would
> > > allow to kill find_any_fileas well..
> > >
> >
> > I started looking at this this morning...
> >
> > FWIW, I don't see where AFS uses the struct file for anything
> > non-trivial in the filp_close codepaths. afs_do_unlk doesn't seem to do
> > much with it, and I don't see a ->flush operation for AFS. Am I missing
> > something there?
> >
> > Here's what I think this change would look like. It builds but is
> > otherwise untested, and the commit log is sort of half-assed right now.
> > It should get slotted on top of this patch in the series:
> >
> > nfsd: do filp_close in sc_free callback for lock stateids
> >
> > I'm not sure this really improves anything.
> >
> > For one thing, we can only store a single struct file, but there's no
> > guarantee that the locks represented by the lock stateid all used that
> > file.
>
> Right, so after this patch st_file means "the last file used by a lock
> operation using this lock stateid"?
>
Yes. If we did decide to use some variant of this, I might change the
name just to avoid confusion with the old st_file field.
> > Also, find_any_file is rather cheap, so I don't see this helping
> > performance much.
> >
> > Given that, is this really worth the extra memory? Thoughts?
>
> I'm not convinced it is. But I understand that we're imposing a weird
> assumption on the lock interface: "if you're exportable, you should use
> the struct file we passed you only to get the inode".
>
> It's out of scope for this work now, but I wonder whether my
> f9d7562fdb9d "nfsd4: share file descriptors between stateid's" was a
> mistake. It also seemed potentially surprising to filesystems that we
> could write or write-lock using a struct file originally opened for
> read, but maybe that wasn't really a problem.
>
Hmm...maybe.
I think though that you'd have had the same problem if you had tried to
use a different scheme. The VFS just doesn't do open upgrades today,
and that's the part that makes this so difficult.
Doing an upgrade means that you're going to change the struct file, and
that makes it difficult to guarantee that you'll always use the same one
for locking.
> > @@ -4866,8 +4849,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > case NFS4_READW_LT:
> > spin_lock(&fp->fi_lock);
> > filp = find_readable_file_locked(fp);
> > - if (filp)
> > + if (filp) {
> > + swap(lock_stp->st_file, filp);
> > get_lock_access(lock_stp, NFS4_SHARE_ACCESS_READ);
>
> I haven't checked carefully, but in the case of a new lock stateid, is
> st_file going to be NULL, in which case this is going to cause the
> openmode check following this case statement to fail?
>
> --b.
--
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
