On Thu, 10 Jul 2014 05:05:22 -0700
Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> On Thu, Jul 10, 2014 at 07:49:52AM -0400, Jeff Layton wrote:
> >
> > AFAIU, the fi_access counters tell us when we're able to zero out the
> > fi_fds slot. fput returns void so we don't have a way to know whether
> > we're putting the last reference to a file or not. Without that,
> > find_*_file become quite problematic -- how would we know whether the
> > pointers they return are still good?
>
> Oh right, that's defintively a good reason to keep the counters.
>
> > > Note that the comment explaining fi_access also seems wrong, as it
> > > suggest contributing 0-4 to the counts, but at best we increment each
> > > one by 1 in a single operation.
> > >
> >
> > Right, we only increment by one for each operation, but open stateids
> > can be upgraded and downgraded. If we do:
> >
> > OPEN for NFS4_SHARE_ACCESS_READ
> > OPEN for NFS4_SHARE_ACCESS_WRITE
> > OPEN for NFS4_SHARE_ACCESS_BOTH
> >
> > ...then we will have incremented the fi_access counts by a total of 4.
> >
> > The big pain in the ass with this code is explained in the comment over
> > bmap_to_share_mode. We have to keep track of what share bits have been
> > previously used in order to comply with the RFC, so you can easily end
> > up with "extra" references.
>
> I've read both the comment, and the section in the RFC, but I still
> don't understand how they explain this convoluted code.
>
> Assume we'd have three members in fi_access instead, indexed by
> (oflags & (O_RDONLY|O_WRONLY|O_RDWR):
>
> static void nfs4_file_get_access(struct nfs4_file *fp, u32 access)
> {
> int oflags = nfs4_access_to_omode(access);
>
> WARN_ON_ONCE(fp->fi_fds[oflag] == NULL);
> atomic_inc(&fp->fi_access[oflag]);
> }
>
> static void nfs4_file_put_access(struct nfs4_file *fp, u32 access)
> {
> int oflags = nfs4_access_to_omode(access);
>
> if (atomic_dec_and_test(&fp->fi_access[oflag]))
> nfs4_file_put_fd(fp, oflag);
> }
>
> nfsd4_open_downgrade would become a little more complicated because it
> would have to check if an FD for the downgraded access is around before
> dropping the reference to the r/w one it currently holds. And as far as
> I can tell O_RDWR to O_RDONLY or O_WRONLY is the only possible case for
> it, feel free to correct me.
>
> Or we could simply not bother with downgrading the underlying file here
> at all, just like it's done for the case where only a O_RDWR file is
> open in the current code, which probably is a fairly common case.
That sounds like it would work, but will it really improve this code?
I'm not convinced.
Having to check whether you actually have the fd before dropping its
refcount sounds "fiddly". You'd need to do the check and put under the
fi_lock in order to not open up new races.
So, we'd either lose the benefit of using atomics for fi_access, or
have to have a whole separate nfs4_file_put_access_locked codepath that
can be done while already holding the lock.
That doesn't sound to me like it's really an improvement on this code.
--
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
