On Thu, 10 Jul 2014 00:59:20 -0700
Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> > +static void nfs4_file_get_access(struct nfs4_file *fp, u32 access)
> > {
> > + int oflag = nfs4_access_to_omode(access);
> > +
> > + /* Note: relies on NFS4_SHARE_ACCESS_BOTH == READ|WRITE */
> > + access &= NFS4_SHARE_ACCESS_BOTH;
> > + if (access == 0)
> > + return;
> > +
> > if (oflag == O_RDWR) {
>
> This fragment looks odd to me in several ways.
>
> For one NFS4_SHARE_ACCESS_BOTH isn't == READ|WRITE, although reading it
> again I suspect this supposed to mean
> NFS4_SHARE_ACCESS_READ|NFS4_SHARE_ACCESS_WRITE.
Yeah, that's correct. I probably shouldn't abbreviate those, sorry...
> Second why to the &=
> on access if it's not used except for the test, or for that matter
> why don't we do the check on the oflag?
>
That &= made more sense in Trond's original patch, but it probably can
be removed now.
> I can see two sensible ways to do this:
>
> a)
>
> static void nfs4_file_get_access(struct nfs4_file *fp, u32 access)
> {
> int oflag = nfs4_access_to_omode(access);
>
> if (oflag == O_RDWR) {
> __nfs4_file_get_access(fp, O_RDONLY);
> __nfs4_file_get_access(fp, O_WRONLY);
> } else if (oflag == O_RDONLY || oflag == O_RDONLY)
> __nfs4_file_get_access(fp, oflag);
> }
> }
>
> Or even better just avoid the nfs4_file_get_access call altogether:
>
> static void nfs4_file_get_access(struct nfs4_file *fp, u32 access)
> {
> WARN_ON_ONCE(access & ~NFS4_SHARE_ACCESS_BOTH);
>
> if (access & NFS4_SHARE_ACCESS_WRITE)
> __nfs4_file_get_access(fp, O_WRONLY);
> if (access & NFS4_SHARE_ACCESS_READ)
> __nfs4_file_get_access(fp, O_RDONLY);
> }
>
> Same for the put side.
>
Yeah, that second one looks fine. I'll change the patch to do that
here, but I'm not sure if I'll need to morph that a bit in the later
patches.
>
> Btw, what is the story about the third fd in fi_fds? Seems like
> nfs4_get_vfs_file can put a file pointer in there, but
> nfs4_file_get_access never grabs a reference to it.
This code is just plain odd altogether, but it does make a perverse
sort of sense. Here's the (patched) __nfs4_file_put_access:
if (atomic_dec_and_lock(&fp->fi_access[oflag], &fp->fi_lock)) {
struct file *f1 = NULL;
struct file *f2 = NULL;
swap(f1, fp->fi_fds[oflag]);
if (atomic_read(&fp->fi_access[1 - oflag]) == 0)
swap(f2, fp->fi_fds[O_RDWR]);
spin_unlock(&fp->fi_lock);
So, we decrement the fi_access counter on the O_*ONLY flag that we want
to put. If it goes to zero, then we check the other O_*ONLY counter and
if it's also zero, we fput the O_RDWR file.
So, you're correct that we never take an fi_access reference for O_RDWR,
which is why the array doesn't have a slot for it:
atomic_t fi_access[2];
...it's tracked by the union of the O_RDONLY and O_WRONLY counters.
--
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
