diff mbox

[15/70] NFSd: Add locking to the nfs4_file->fi_fds[] array

Message ID 1397846704-14567-16-git-send-email-trond.myklebust@primarydata.com (mailing list archive)
State New, archived
Headers show

Commit Message

Trond Myklebust April 18, 2014, 6:44 p.m. UTC 
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfsd/nfs4state.c | 107 ++++++++++++++++++++++++++++++++++++++++++++--------
 fs/nfsd/state.h     |  26 -------------
 2 files changed, 91 insertions(+), 42 deletions(-)

Comments

Christoph Hellwig April 19, 2014, 2:35 p.m. UTC | #1 
> +/* XXX: for first cut may fall back on returning file that doesn't work
> + * at all? */

I think moving this code around might be a good opportunity to remove
this confusing comment.

> +static struct file *find_writeable_file(struct nfs4_file *f)
> +{
> +	struct file *ret;
> +
> +	spin_lock(&f->fi_lock);
> +	ret = __nfs4_get_fd(f, O_WRONLY);
> +	if (!ret)
> +		ret = __nfs4_get_fd(f, O_RDWR);
> +	spin_unlock(&f->fi_lock);
> +	return ret;
> +}
> +
> +static struct file *find_readable_file(struct nfs4_file *f)
> +{
> +	struct file *ret;
> +
> +	spin_lock(&f->fi_lock);
> +	ret = __nfs4_get_fd(f, O_RDONLY);
> +	if (!ret)
> +		ret = __nfs4_get_fd(f, O_RDWR);
> +	spin_unlock(&f->fi_lock);
> +	return ret;

Seems like these two functions could be easily consolidated by passing
a single flags argument.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust April 21, 2014, 1:01 p.m. UTC | #2 
Hi Christoph,

Thanks for reviewing these!

On Sat, 2014-04-19 at 07:35 -0700, Christoph Hellwig wrote:
> > +/* XXX: for first cut may fall back on returning file that doesn't work
> > + * at all? */
> 
> I think moving this code around might be a good opportunity to remove
> this confusing comment.

Will do.

> > +static struct file *find_writeable_file(struct nfs4_file *f)
> > +{
> > +	struct file *ret;
> > +
> > +	spin_lock(&f->fi_lock);
> > +	ret = __nfs4_get_fd(f, O_WRONLY);
> > +	if (!ret)
> > +		ret = __nfs4_get_fd(f, O_RDWR);
> > +	spin_unlock(&f->fi_lock);
> > +	return ret;
> > +}
> > +
> > +static struct file *find_readable_file(struct nfs4_file *f)
> > +{
> > +	struct file *ret;
> > +
> > +	spin_lock(&f->fi_lock);
> > +	ret = __nfs4_get_fd(f, O_RDONLY);
> > +	if (!ret)
> > +		ret = __nfs4_get_fd(f, O_RDWR);
> > +	spin_unlock(&f->fi_lock);
> > +	return ret;
> 
> Seems like these two functions could be easily consolidated by passing
> a single flags argument.

Yes, but we'd have to invent a new set of flags for just this function
and so I'm not sure that the end result would be more readable.
We can't reuse O_RDONLY/O_WRONLY/O_RDWR since they can't be bitwise ORed
to produce the 'read or read/write', 'write or read/write' combinations
that we need.

Cheers
  Trond
Christoph Hellwig April 21, 2014, 1:14 p.m. UTC | #3 
On Mon, Apr 21, 2014 at 09:01:37AM -0400, Trond Myklebust wrote:
> > > +static struct file *find_readable_file(struct nfs4_file *f)
> > > +{
> > > +	struct file *ret;
> > > +
> > > +	spin_lock(&f->fi_lock);
> > > +	ret = __nfs4_get_fd(f, O_RDONLY);
> > > +	if (!ret)
> > > +		ret = __nfs4_get_fd(f, O_RDWR);
> > > +	spin_unlock(&f->fi_lock);
> > > +	return ret;
> > 
> > Seems like these two functions could be easily consolidated by passing
> > a single flags argument.
> 
> Yes, but we'd have to invent a new set of flags for just this function
> and so I'm not sure that the end result would be more readable.
> We can't reuse O_RDONLY/O_WRONLY/O_RDWR since they can't be bitwise ORed
> to produce the 'read or read/write', 'write or read/write' combinations
> that we need.

One option would be:

static struct file *__nfs4_get_fd(struct nfs4_file *f, int oflag,
		int type)
{
	if ((oflag & type) && f->fi_fds[type])
		return get_file(f->fi_fds[type]);
	return NULL;
}

struct file *nfsd4_find_file(struct nfs4_file *f, unsigned int oflag)
{
	struct file *ret;

	BUG_ON(oflag & ~(O_RDONLY|O_WRONLY|O_RDWR);

	spin_lock(&f->fi_lock);
	ret = __nfs4_get_fd(f, oflag, O_RDONLY);
	if (!ret) {
		ret = __nfs4_get_fd(f, oflag, O_WRONLY);
		if (!ret) 
			ret = __nfs4_get_fd(f, oflag, O_RDWR);
	}
	spin_unlock(&f->fi_lock);
	return ret;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig April 21, 2014, 1:16 p.m. UTC | #4 
On Mon, Apr 21, 2014 at 06:14:12AM -0700, Christoph Hellwig wrote:
> One option would be:

Doesn't work because O_RDONLY is defined as 0.  But if we use
FMODE_READ and FMODE_WRITE as flag it should work.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ad7d21e06c36..73e7da308d37 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -249,6 +249,54 @@  get_nfs4_file(struct nfs4_file *fi)
 	atomic_inc(&fi->fi_ref);
 }
 
+static struct file *__nfs4_get_fd(struct nfs4_file *f, int oflag)
+{
+	if (f->fi_fds[oflag])
+		return get_file(f->fi_fds[oflag]);
+	return NULL;
+}
+
+/* XXX: for first cut may fall back on returning file that doesn't work
+ * at all? */
+static struct file *find_writeable_file(struct nfs4_file *f)
+{
+	struct file *ret;
+
+	spin_lock(&f->fi_lock);
+	ret = __nfs4_get_fd(f, O_WRONLY);
+	if (!ret)
+		ret = __nfs4_get_fd(f, O_RDWR);
+	spin_unlock(&f->fi_lock);
+	return ret;
+}
+
+static struct file *find_readable_file(struct nfs4_file *f)
+{
+	struct file *ret;
+
+	spin_lock(&f->fi_lock);
+	ret = __nfs4_get_fd(f, O_RDONLY);
+	if (!ret)
+		ret = __nfs4_get_fd(f, O_RDWR);
+	spin_unlock(&f->fi_lock);
+	return ret;
+}
+
+static struct file *find_any_file(struct nfs4_file *f)
+{
+	struct file *ret;
+
+	spin_lock(&f->fi_lock);
+	ret = __nfs4_get_fd(f, O_RDWR);
+	if (!ret) {
+		ret = __nfs4_get_fd(f, O_WRONLY);
+		if (!ret)
+			ret = __nfs4_get_fd(f, O_RDONLY);
+	}
+	spin_unlock(&f->fi_lock);
+	return ret;
+}
+
 static int num_delegations;
 unsigned long max_delegations;
 
@@ -297,20 +345,29 @@  static void nfs4_file_get_access(struct nfs4_file *fp, int oflag)
 		__nfs4_file_get_access(fp, oflag);
 }
 
-static void nfs4_file_put_fd(struct nfs4_file *fp, int oflag)
+static struct file *nfs4_file_put_fd(struct nfs4_file *fp, int oflag)
 {
-	if (fp->fi_fds[oflag]) {
-		fput(fp->fi_fds[oflag]);
-		fp->fi_fds[oflag] = NULL;
-	}
+	struct file *filp;
+
+	filp = fp->fi_fds[oflag];
+	fp->fi_fds[oflag] = NULL;
+	return filp;
 }
 
 static void __nfs4_file_put_access(struct nfs4_file *fp, int oflag)
 {
-	if (atomic_dec_and_test(&fp->fi_access[oflag])) {
-		nfs4_file_put_fd(fp, oflag);
+	if (atomic_dec_and_lock(&fp->fi_access[oflag], &fp->fi_lock)) {
+		struct file *f1 = NULL;
+		struct file *f2 = NULL;
+
+		f1 = nfs4_file_put_fd(fp, oflag);
 		if (atomic_read(&fp->fi_access[1 - oflag]) == 0)
-			nfs4_file_put_fd(fp, O_RDWR);
+			f2 = nfs4_file_put_fd(fp, O_RDWR);
+		spin_unlock(&fp->fi_lock);
+		if (f1)
+			fput(f1);
+		if (f2)
+			fput(f2);
 	}
 }
 
@@ -651,8 +708,10 @@  static void release_lock_stateid(struct nfs4_ol_stateid *stp)
 	unhash_generic_stateid(stp);
 	unhash_stid(&stp->st_stid);
 	file = find_any_file(stp->st_file);
-	if (file)
+	if (file) {
 		locks_remove_posix(file, (fl_owner_t)lockowner(stp->st_stateowner));
+		fput(file);
+	}
 	close_generic_stateid(stp);
 	free_generic_stateid(stp);
 }
@@ -2985,17 +3044,27 @@  static inline int nfs4_access_to_access(u32 nfs4_access)
 static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
 		struct svc_fh *cur_fh, struct nfsd4_open *open)
 {
+	struct file *filp = NULL;
 	__be32 status;
 	int oflag = nfs4_access_to_omode(open->op_share_access);
 	int access = nfs4_access_to_access(open->op_share_access);
 
+	spin_lock(&fp->fi_lock);
 	if (!fp->fi_fds[oflag]) {
-		status = nfsd_open(rqstp, cur_fh, S_IFREG, access,
-			&fp->fi_fds[oflag]);
+		spin_unlock(&fp->fi_lock);
+		status = nfsd_open(rqstp, cur_fh, S_IFREG, access, &filp);
 		if (status)
 			return status;
+		spin_lock(&fp->fi_lock);
+		if (!fp->fi_fds[oflag]) {
+			fp->fi_fds[oflag] = filp;
+			filp = NULL;
+		}
 	}
 	nfs4_file_get_access(fp, oflag);
+	spin_unlock(&fp->fi_lock);
+	if (filp)
+		fput(filp);
 
 	return nfs_ok;
 }
@@ -3094,13 +3163,15 @@  static int nfs4_setlease(struct nfs4_delegation *dp)
 	if (status)
 		goto out_free;
 	fp->fi_lease = fl;
-	fp->fi_deleg_file = get_file(fl->fl_file);
+	fp->fi_deleg_file = fl->fl_file;
 	atomic_set(&fp->fi_delegees, 1);
 	spin_lock(&state_lock);
 	hash_delegation_locked(dp, fp);
 	spin_unlock(&state_lock);
 	return 0;
 out_free:
+	if (fl->fl_file)
+		fput(fl->fl_file);
 	locks_free_lock(fl);
 	return status;
 }
@@ -3734,6 +3805,7 @@  nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
 				status = nfserr_serverfault;
 				goto out;
 			}
+			get_file(file);
 		}
 		break;
 	case NFS4_OPEN_STID:
@@ -3761,7 +3833,7 @@  nfs4_preprocess_stateid_op(struct net *net, struct nfsd4_compound_state *cstate,
 	}
 	status = nfs_ok;
 	if (file)
-		*filpp = get_file(file);
+		*filpp = file;
 out:
 	nfs4_unlock_state();
 	return status;
@@ -4489,6 +4561,8 @@  nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		break;
 	}
 out:
+	if (filp)
+		fput(filp);
 	if (status && new_state)
 		release_lockowner(lock_sop);
 	nfsd4_bump_seqid(cstate, status);
@@ -4630,7 +4704,7 @@  nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (!file_lock) {
 		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
 		status = nfserr_jukebox;
-		goto out;
+		goto fput;
 	}
 	locks_init_lock(file_lock);
 	file_lock->fl_type = F_UNLCK;
@@ -4652,7 +4726,8 @@  nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	}
 	update_stateid(&stp->st_stid.sc_stateid);
 	memcpy(&locku->lu_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
-
+fput:
+	fput(filp);
 out:
 	nfsd4_bump_seqid(cstate, status);
 	nfs4_unlock_state();
@@ -4662,7 +4737,7 @@  out:
 
 out_nfserr:
 	status = nfserrno(err);
-	goto out;
+	goto fput;
 }
 
 /*
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 80789aa5b5b4..004733885cca 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -405,32 +405,6 @@  struct nfs4_file {
 	bool			fi_had_conflict;
 };
 
-/* XXX: for first cut may fall back on returning file that doesn't work
- * at all? */
-static inline struct file *find_writeable_file(struct nfs4_file *f)
-{
-	if (f->fi_fds[O_WRONLY])
-		return f->fi_fds[O_WRONLY];
-	return f->fi_fds[O_RDWR];
-}
-
-static inline struct file *find_readable_file(struct nfs4_file *f)
-{
-	if (f->fi_fds[O_RDONLY])
-		return f->fi_fds[O_RDONLY];
-	return f->fi_fds[O_RDWR];
-}
-
-static inline struct file *find_any_file(struct nfs4_file *f)
-{
-	if (f->fi_fds[O_RDWR])
-		return f->fi_fds[O_RDWR];
-	else if (f->fi_fds[O_WRONLY])
-		return f->fi_fds[O_WRONLY];
-	else
-		return f->fi_fds[O_RDONLY];
-}
-
 /* "ol" stands for "Open or Lock".  Better suggestions welcome. */
 struct nfs4_ol_stateid {
 	struct nfs4_stid    st_stid; /* must be first field */