Skip to contentSkip to conversation

nfsd: deny mode handling overhaul

PATCH In review Jeff Layton 6 participants 27 messages Jul 10, 2014 – Jul 15, 2014

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Jeff Layton

author
#0 lore raw

cover nfsd: deny mode handling overhaul

Here is the deny mode handling overhaul broken out into a separate series, as requested by Christoph. I've tried to address most of his review comments, but I may have missed some. Let me know if if I did.

Part of the client_mutex removal series involves cleaning up the handling of deny modes in the server in order to setting and enforcement atomic.

This does add a new per-nfs4_file spinlock, but in principle it should never be contended while the client_mutex is still wrapped around all of this code.

Jeff Layton (8):
nfsd: refactor nfs4_file_get_access and nfs4_file_put_access
nfsd: remove nfs4_file_put_fd
nfsd: shrink st_access_bmap and st_deny_bmap
nfsd: set stateid access and deny bits in nfs4_get_vfs_file
nfsd: clean up reset_union_bmap_deny
nfsd: always hold the fi_lock when bumping fi_access refcounts
nfsd: make deny mode enforcement more efficient and close races in it
nfsd: cleanup and rename nfs4_check_open

Trond Myklebust (3):
nfsd: Add fine grained protection for the nfs4_file->fi_stateids list
nfsd: Add locking to the nfs4_file->fi_fds[] array
nfsd: clean up helper __release_lock_stateid

 fs/nfsd/nfs4state.c | 437 ++++++++++++++++++++++++++++++++++++++--------------
 fs/nfsd/state.h     |  32 +---
 2 files changed, 325 insertions(+), 144 deletions(-)
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH 1/11 nfsd: Add fine grained protection for the nfs4_file->fi_stateids list

From: Trond Myklebust <[email protected]>

Access to this list is currently serialized by the client_mutex. Add finer grained locking around this list in preparation for its removal.

Signed-off-by Trond Myklebust <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 21 ++++++++++++++++++---
 fs/nfsd/state.h     |  1 +
 2 files changed, 19 insertions(+), 3 deletions(-)
fs/nfsd/nfs4state.c +18 -3
index 3704789ca4b7..cfb10d060c83 100644
@@ -708,7 +708,11 @@ release_all_access(struct nfs4_ol_stateid *stp)
 
 static void unhash_generic_stateid(struct nfs4_ol_stateid *stp)
 {
+	struct nfs4_file *fp = stp->st_file;
+
+	spin_lock(&fp->fi_lock);
 	list_del(&stp->st_perfile);
+	spin_unlock(&fp->fi_lock);
 	list_del(&stp->st_perstateowner);
 }
 

@@ -2676,6 +2680,7 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
 	lockdep_assert_held(&state_lock);
 
 	atomic_set(&fp->fi_ref, 1);
+	spin_lock_init(&fp->fi_lock);
 	INIT_LIST_HEAD(&fp->fi_stateids);
 	INIT_LIST_HEAD(&fp->fi_delegations);
 	ihold(ino);

@@ -2799,7 +2804,6 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
 	stp->st_stid.sc_type = NFS4_OPEN_STID;
 	INIT_LIST_HEAD(&stp->st_locks);
 	list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
-	list_add(&stp->st_perfile, &fp->fi_stateids);
 	stp->st_stateowner = &oo->oo_owner;
 	get_nfs4_file(fp);
 	stp->st_file = fp;

@@ -2808,6 +2812,9 @@ static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
 	set_access(open->op_share_access, stp);
 	set_deny(open->op_share_deny, stp);
 	stp->st_openstp = NULL;
+	spin_lock(&fp->fi_lock);
+	list_add(&stp->st_perfile, &fp->fi_stateids);
+	spin_unlock(&fp->fi_lock);
 }
 
 static void

@@ -2915,6 +2922,7 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type)
 		return nfs_ok;
 	ret = nfserr_locked;
 	/* Search for conflicting share reservations */
+	spin_lock(&fp->fi_lock);
 	list_for_each_entry(stp, &fp->fi_stateids, st_perfile) {
 		if (test_deny(deny_type, stp) ||
 		    test_deny(NFS4_SHARE_DENY_BOTH, stp))

@@ -2922,6 +2930,7 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type)
 	}
 	ret = nfs_ok;
 out:
+	spin_unlock(&fp->fi_lock);
 	put_nfs4_file(fp);
 	return ret;
 }

@@ -3150,6 +3159,7 @@ nfs4_check_open(struct nfs4_file *fp, struct nfsd4_open *open, struct nfs4_ol_st
 	struct nfs4_ol_stateid *local;
 	struct nfs4_openowner *oo = open->op_openowner;
 
+	spin_lock(&fp->fi_lock);
 	list_for_each_entry(local, &fp->fi_stateids, st_perfile) {
 		/* ignore lock owners */
 		if (local->st_stateowner->so_is_open_owner == 0)

@@ -3158,9 +3168,12 @@ nfs4_check_open(struct nfs4_file *fp, struct nfsd4_open *open, struct nfs4_ol_st
 		if (local->st_stateowner == &oo->oo_owner)
 			*stpp = local;
 		/* check for conflicting share reservations */
-		if (!test_share(local, open))
+		if (!test_share(local, open)) {
+			spin_unlock(&fp->fi_lock);
 			return nfserr_share_denied;
+		}
 	}
+	spin_unlock(&fp->fi_lock);
 	return nfs_ok;
 }
 

@@ -4408,7 +4421,6 @@ alloc_init_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp, struct
 	if (stp == NULL)
 		return NULL;
 	stp->st_stid.sc_type = NFS4_LOCK_STID;
-	list_add(&stp->st_perfile, &fp->fi_stateids);
 	list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
 	stp->st_stateowner = &lo->lo_owner;
 	get_nfs4_file(fp);

@@ -4417,6 +4429,9 @@ alloc_init_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp, struct
 	stp->st_deny_bmap = open_stp->st_deny_bmap;
 	stp->st_openstp = open_stp;
 	list_add(&stp->st_locks, &open_stp->st_locks);
+	spin_lock(&fp->fi_lock);
+	list_add(&stp->st_perfile, &fp->fi_stateids);
+	spin_unlock(&fp->fi_lock);
 	return stp;
 }
fs/nfsd/state.h +1 -0
index 06d1a908a58e..04737b3ed363 100644
@@ -377,6 +377,7 @@ static inline struct nfs4_lockowner * lockowner(struct nfs4_stateowner *so)
 /* nfs4_file: a file opened by some number of (open) nfs4_stateowners. */
 struct nfs4_file {
 	atomic_t		fi_ref;
+	spinlock_t		fi_lock;
 	struct hlist_node       fi_hash;    /* hash by "struct inode *" */
 	struct list_head        fi_stateids;
 	struct list_head	fi_delegations;
Signature · 1 line
1.9.3
author
#2 lore raw

PATCH 2/11 nfsd: Add locking to the nfs4_file->fi_fds[] array

In reply to Jeff Layton

From: Trond Myklebust <[email protected]>

Preparation for removal of the client_mutex, which currently protects this array. While we don't actually need the find_*_file_locked variants just yet, a later patch will. So go ahead and add them now to reduce future churn in this code.

Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 134 +++++++++++++++++++++++++++++++++++++++++++++-------
 fs/nfsd/state.h     |  26 ----------
 2 files changed, 118 insertions(+), 42 deletions(-)
fs/nfsd/nfs4state.c +118 -16
index cfb10d060c83..314dc8061461 100644
@@ -268,6 +268,79 @@ 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;
+}
+
+static struct file *
+find_writeable_file_locked(struct nfs4_file *f)
+{
+	struct file *ret;
+
+	lockdep_assert_held(&f->fi_lock);
+
+	ret = __nfs4_get_fd(f, O_WRONLY);
+	if (!ret)
+		ret = __nfs4_get_fd(f, O_RDWR);
+	return ret;
+}
+
+static struct file *
+find_writeable_file(struct nfs4_file *f)
+{
+	struct file *ret;
+
+	spin_lock(&f->fi_lock);
+	ret = find_writeable_file_locked(f);
+	spin_unlock(&f->fi_lock);
+
+	return ret;
+}
+
+static struct file *find_readable_file_locked(struct nfs4_file *f)
+{
+	struct file *ret;
+
+	lockdep_assert_held(&f->fi_lock);
+
+	ret = __nfs4_get_fd(f, O_RDONLY);
+	if (!ret)
+		ret = __nfs4_get_fd(f, O_RDWR);
+	return ret;
+}
+
+static struct file *
+find_readable_file(struct nfs4_file *f)
+{
+	struct file *ret;
+
+	spin_lock(&f->fi_lock);
+	ret = find_readable_file_locked(f);
+	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;
 

@@ -316,20 +389,31 @@ 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);
+	might_lock(&fp->fi_lock);
+
+	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);
 	}
 }
 

@@ -737,8 +821,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);
 }

@@ -3206,17 +3292,27 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
 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)
 			goto out;
+		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);
 
 	status = nfsd4_truncate(rqstp, cur_fh, open);
 	if (status)

@@ -3301,13 +3397,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;
 }

@@ -3905,6 +4003,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:

@@ -3932,7 +4031,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;

@@ -4653,6 +4752,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		break;
 	}
 out:
+	if (filp)
+		fput(filp);
 	if (status && new_state)
 		release_lock_stateid(lock_stp);
 	nfsd4_bump_seqid(cstate, status);

@@ -4793,7 +4894,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;

@@ -4815,7 +4916,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);
 	if (!cstate->replay_owner)

@@ -4826,7 +4928,7 @@ out:
 
 out_nfserr:
 	status = nfserrno(err);
-	goto out;
+	goto fput;
 }
 
 /*
fs/nfsd/state.h +0 -26
index 04737b3ed363..9f1159d5de56 100644
@@ -398,32 +398,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 */
Signature · 1 line
1.9.3
author
#3 lore raw

PATCH 3/11 nfsd: clean up helper __release_lock_stateid

In reply to Jeff Layton

From: Trond Myklebust <[email protected]>

Use filp_close instead of open coding. filp_close does a bit more than just release the locks and put the filp. It also calls ->flush and dnotify_flush, both of which should be done here anyway.

Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
fs/nfsd/nfs4state.c +2 -4
index 314dc8061461..4ab567e7db0f 100644
@@ -821,10 +821,8 @@ 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) {
-		locks_remove_posix(file, (fl_owner_t)lockowner(stp->st_stateowner));
-		fput(file);
-	}
+	if (file)
+		filp_close(file, (fl_owner_t)lockowner(stp->st_stateowner));
 	close_generic_stateid(stp);
 	free_generic_stateid(stp);
 }
Signature · 1 line
1.9.3
author
#4 lore raw

PATCH 4/11 nfsd: refactor nfs4_file_get_access and nfs4_file_put_access

In reply to Jeff Layton

Have them take NFS4_SHARE_ACCESS_* flags instead of an open mode. This spares the callers from having to convert it themselves.

This also allows us to simplify these functions as we no longer need to do the access_to_omode conversion in either one.

Note too that this patch eliminates the WARN_ON in __nfs4_file_get_access. It's valid for now, but in a later patch we'll be bumping the refcounts prior to opening the file in order to close some races, at which point we'll need to remove it anyway.

Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)
fs/nfsd/nfs4state.c +27 -21
index 4ab567e7db0f..a19257f91f25 100644
@@ -374,19 +374,24 @@ static unsigned int file_hashval(struct inode *ino)
 
 static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
 
-static void __nfs4_file_get_access(struct nfs4_file *fp, int oflag)
+static void
+__nfs4_file_get_access(struct nfs4_file *fp, u32 access)
 {
-	WARN_ON_ONCE(!(fp->fi_fds[oflag] || fp->fi_fds[O_RDWR]));
-	atomic_inc(&fp->fi_access[oflag]);
+	if (access & NFS4_SHARE_ACCESS_WRITE)
+		atomic_inc(&fp->fi_access[O_WRONLY]);
+	if (access & NFS4_SHARE_ACCESS_READ)
+		atomic_inc(&fp->fi_access[O_RDONLY]);
 }
 
-static void nfs4_file_get_access(struct nfs4_file *fp, int oflag)
+static __be32
+nfs4_file_get_access(struct nfs4_file *fp, u32 access)
 {
-	if (oflag == O_RDWR) {
-		__nfs4_file_get_access(fp, O_RDONLY);
-		__nfs4_file_get_access(fp, O_WRONLY);
-	} else
-		__nfs4_file_get_access(fp, oflag);
+	/* Does this access mode make sense? */
+	if (access & ~NFS4_SHARE_ACCESS_BOTH)
+		return nfserr_inval;
+
+	__nfs4_file_get_access(fp, access);
+	return nfs_ok;
 }
 
 static struct file *nfs4_file_put_fd(struct nfs4_file *fp, int oflag)

@@ -417,13 +422,14 @@ static void __nfs4_file_put_access(struct nfs4_file *fp, int oflag)
 	}
 }
 
-static void nfs4_file_put_access(struct nfs4_file *fp, int oflag)
+static void nfs4_file_put_access(struct nfs4_file *fp, u32 access)
 {
-	if (oflag == O_RDWR) {
-		__nfs4_file_put_access(fp, O_RDONLY);
+	WARN_ON_ONCE(access & ~NFS4_SHARE_ACCESS_BOTH);
+
+	if (access & NFS4_SHARE_ACCESS_WRITE)
 		__nfs4_file_put_access(fp, O_WRONLY);
-	} else
-		__nfs4_file_put_access(fp, oflag);
+	if (access & NFS4_SHARE_ACCESS_READ)
+		__nfs4_file_put_access(fp, O_RDONLY);
 }
 
 static struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct

@@ -784,8 +790,7 @@ release_all_access(struct nfs4_ol_stateid *stp)
 
 	for (i = 1; i < 4; i++) {
 		if (test_access(i, stp))
-			nfs4_file_put_access(stp->st_file,
-					     nfs4_access_to_omode(i));
+			nfs4_file_put_access(stp->st_file, i);
 		clear_access(i, stp);
 	}
 }

@@ -3307,10 +3312,12 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
 			filp = NULL;
 		}
 	}
-	nfs4_file_get_access(fp, oflag);
+	status = nfs4_file_get_access(fp, open->op_share_access);
 	spin_unlock(&fp->fi_lock);
 	if (filp)
 		fput(filp);
+	if (status)
+		goto out_put_access;
 
 	status = nfsd4_truncate(rqstp, cur_fh, open);
 	if (status)

@@ -3319,7 +3326,7 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
 	return nfs_ok;
 
 out_put_access:
-	nfs4_file_put_access(fp, oflag);
+	nfs4_file_put_access(fp, open->op_share_access);
 out:
 	return status;
 }

@@ -4228,7 +4235,7 @@ static inline void nfs4_stateid_downgrade_bit(struct nfs4_ol_stateid *stp, u32 a
 {
 	if (!test_access(access, stp))
 		return;
-	nfs4_file_put_access(stp->st_file, nfs4_access_to_omode(access));
+	nfs4_file_put_access(stp->st_file, access);
 	clear_access(access, stp);
 }
 

@@ -4555,11 +4562,10 @@ check_lock_length(u64 offset, u64 length)
 static void get_lock_access(struct nfs4_ol_stateid *lock_stp, u32 access)
 {
 	struct nfs4_file *fp = lock_stp->st_file;
-	int oflag = nfs4_access_to_omode(access);
 
 	if (test_access(access, lock_stp))
 		return;
-	nfs4_file_get_access(fp, oflag);
+	__nfs4_file_get_access(fp, access);
 	set_access(access, lock_stp);
 }
Signature · 1 line
1.9.3
author
#5 lore raw

PATCH 5/11 nfsd: remove nfs4_file_put_fd

In reply to Jeff Layton

...and replace it with a simple swap call.

Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)
fs/nfsd/nfs4state.c +2 -11
index a19257f91f25..c02bad6d7e90 100644
@@ -394,15 +394,6 @@ nfs4_file_get_access(struct nfs4_file *fp, u32 access)
 	return nfs_ok;
 }
 
-static struct file *nfs4_file_put_fd(struct nfs4_file *fp, int oflag)
-{
-	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)
 {
 	might_lock(&fp->fi_lock);

@@ -411,9 +402,9 @@ static void __nfs4_file_put_access(struct nfs4_file *fp, int oflag)
 		struct file *f1 = NULL;
 		struct file *f2 = NULL;
 
-		f1 = nfs4_file_put_fd(fp, oflag);
+		swap(f1, fp->fi_fds[oflag]);
 		if (atomic_read(&fp->fi_access[1 - oflag]) == 0)
-			f2 = nfs4_file_put_fd(fp, O_RDWR);
+			swap(f2, fp->fi_fds[O_RDWR]);
 		spin_unlock(&fp->fi_lock);
 		if (f1)
 			fput(f1);
Signature · 1 line
1.9.3
author
#6 lore raw

PATCH 6/11 nfsd: shrink st_access_bmap and st_deny_bmap

In reply to Jeff Layton

We never use anything above bit #3, so an unsigned long for each is wasteful. Shrink them to a char each, and add some WARN_ON_ONCE calls if we try to set or clear bits that would go outside those sizes.

Note too that because atomic bitops work on unsigned longs, we have to abandon their use here. That shouldn't be a problem though since we don't really care about the atomicity in this code anyway. Using them was just a convenient way to flip bits.

Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 38 +++++++++++++++++++++++++++-----------
 fs/nfsd/state.h     |  4 ++--
 2 files changed, 29 insertions(+), 13 deletions(-)
fs/nfsd/nfs4state.c +27 -11
index c02bad6d7e90..f7f11631c26c 100644
@@ -721,42 +721,58 @@ test_share(struct nfs4_ol_stateid *stp, struct nfsd4_open *open) {
 static inline void
 set_access(u32 access, struct nfs4_ol_stateid *stp)
 {
-	__set_bit(access, &stp->st_access_bmap);
+	unsigned char mask = 1 << access;
+
+	WARN_ON_ONCE(access > NFS4_SHARE_ACCESS_BOTH);
+	stp->st_access_bmap |= mask;
 }
 
 /* clear share access for a given stateid */
 static inline void
 clear_access(u32 access, struct nfs4_ol_stateid *stp)
 {
-	__clear_bit(access, &stp->st_access_bmap);
+	unsigned char mask = 1 << access;
+
+	WARN_ON_ONCE(access > NFS4_SHARE_ACCESS_BOTH);
+	stp->st_access_bmap &= ~mask;
 }
 
 /* test whether a given stateid has access */
 static inline bool
 test_access(u32 access, struct nfs4_ol_stateid *stp)
 {
-	return test_bit(access, &stp->st_access_bmap);
+	unsigned char mask = 1 << access;
+
+	return (bool)(stp->st_access_bmap & mask);
 }
 
 /* set share deny for a given stateid */
 static inline void
-set_deny(u32 access, struct nfs4_ol_stateid *stp)
+set_deny(u32 deny, struct nfs4_ol_stateid *stp)
 {
-	__set_bit(access, &stp->st_deny_bmap);
+	unsigned char mask = 1 << deny;
+
+	WARN_ON_ONCE(deny > NFS4_SHARE_DENY_BOTH);
+	stp->st_deny_bmap |= mask;
 }
 
 /* clear share deny for a given stateid */
 static inline void
-clear_deny(u32 access, struct nfs4_ol_stateid *stp)
+clear_deny(u32 deny, struct nfs4_ol_stateid *stp)
 {
-	__clear_bit(access, &stp->st_deny_bmap);
+	unsigned char mask = 1 << deny;
+
+	WARN_ON_ONCE(deny > NFS4_SHARE_DENY_BOTH);
+	stp->st_deny_bmap &= ~mask;
 }
 
 /* test whether a given stateid is denying specific access */
 static inline bool
-test_deny(u32 access, struct nfs4_ol_stateid *stp)
+test_deny(u32 deny, struct nfs4_ol_stateid *stp)
 {
-	return test_bit(access, &stp->st_deny_bmap);
+	unsigned char mask = 1 << deny;
+
+	return (bool)(stp->st_deny_bmap & mask);
 }
 
 static int nfs4_access_to_omode(u32 access)

@@ -4282,12 +4298,12 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
 		goto out; 
 	status = nfserr_inval;
 	if (!test_access(od->od_share_access, stp)) {
-		dprintk("NFSD: access not a subset current bitmap: 0x%lx, input access=%08x\n",
+		dprintk("NFSD: access not a subset of current bitmap: 0x%hhx, input access=%08x\n",
 			stp->st_access_bmap, od->od_share_access);
 		goto out;
 	}
 	if (!test_deny(od->od_share_deny, stp)) {
-		dprintk("NFSD:deny not a subset current bitmap: 0x%lx, input deny=%08x\n",
+		dprintk("NFSD: deny not a subset of current bitmap: 0x%hhx, input deny=%08x\n",
 			stp->st_deny_bmap, od->od_share_deny);
 		goto out;
 	}
fs/nfsd/state.h +2 -2
index 9f1159d5de56..72aee4b4f1ae 100644
@@ -406,8 +406,8 @@ struct nfs4_ol_stateid {
 	struct list_head              st_locks;
 	struct nfs4_stateowner      * st_stateowner;
 	struct nfs4_file            * st_file;
-	unsigned long                 st_access_bmap;
-	unsigned long                 st_deny_bmap;
+	unsigned char                 st_access_bmap;
+	unsigned char                 st_deny_bmap;
 	struct nfs4_ol_stateid         * st_openstp;
 };
Signature · 1 line
1.9.3
author
#7 lore raw

PATCH 7/11 nfsd: set stateid access and deny bits in nfs4_get_vfs_file

In reply to Jeff Layton

Cleanup -- ensure that the stateid bits are set at the same time that the file access refcounts are incremented. Keeping them coherent like this makes it easier to ensure that we account for all of the references.

Since the initialization of the st_*_bmap fields is done when it's hashed, we go ahead and hash the stateid before getting access to the file and unhash it if that function returns error. This will be necessary anyway in a follow-on patch that will overhaul deny mode handling.

Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
fs/nfsd/nfs4state.c +12 -11
index f7f11631c26c..0a54fc956463 100644
@@ -3300,7 +3300,8 @@ nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
 }
 
 static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
-		struct svc_fh *cur_fh, struct nfsd4_open *open)
+		struct svc_fh *cur_fh, struct nfs4_ol_stateid *stp,
+		struct nfsd4_open *open)
 {
 	struct file *filp = NULL;
 	__be32 status;

@@ -3330,6 +3331,9 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
 	if (status)
 		goto out_put_access;
 
+	/* Set access and deny bits in stateid */
+	set_access(open->op_share_access, stp);
+	set_deny(open->op_share_deny, stp);
 	return nfs_ok;
 
 out_put_access:

@@ -3341,20 +3345,15 @@ out:
 static __be32
 nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *cur_fh, struct nfs4_ol_stateid *stp, struct nfsd4_open *open)
 {
-	u32 op_share_access = open->op_share_access;
 	__be32 status;
 
-	if (!test_access(op_share_access, stp))
-		status = nfs4_get_vfs_file(rqstp, fp, cur_fh, open);
+	if (!test_access(open->op_share_access, stp))
+		status = nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open);
 	else
 		status = nfsd4_truncate(rqstp, cur_fh, open);
 
 	if (status)
 		return status;
-
-	/* remember the open */
-	set_access(op_share_access, stp);
-	set_deny(open->op_share_deny, stp);
 	return nfs_ok;
 }
 

@@ -3602,12 +3601,14 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 		if (status)
 			goto out;
 	} else {
-		status = nfs4_get_vfs_file(rqstp, fp, current_fh, open);
-		if (status)
-			goto out;
 		stp = open->op_stp;
 		open->op_stp = NULL;
 		init_open_stateid(stp, fp, open);
+		status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
+		if (status) {
+			release_open_stateid(stp);
+			goto out;
+		}
 	}
 	update_stateid(&stp->st_stid.sc_stateid);
 	memcpy(&open->op_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
Signature · 1 line
1.9.3
author
#8 lore raw

PATCH 8/11 nfsd: clean up reset_union_bmap_deny

In reply to Jeff Layton

Fix the "deny" argument type, and start the loop at 1. The 0 iteration is always a noop.

Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/nfsd/nfs4state.c +3 -2
index 0a54fc956463..5f7294712ad4 100644
@@ -4266,10 +4266,11 @@ static inline void nfs4_stateid_downgrade(struct nfs4_ol_stateid *stp, u32 to_ac
 }
 
 static void
-reset_union_bmap_deny(unsigned long deny, struct nfs4_ol_stateid *stp)
+reset_union_bmap_deny(u32 deny, struct nfs4_ol_stateid *stp)
 {
 	int i;
-	for (i = 0; i < 4; i++) {
+
+	for (i = 1; i < 4; i++) {
 		if ((i & deny) != i)
 			clear_deny(i, stp);
 	}
Signature · 1 line
1.9.3
author
#9 lore raw

PATCH 9/11 nfsd: always hold the fi_lock when bumping fi_access refcounts

In reply to Jeff Layton

Once we remove the client_mutex, there's an unlikely but possible race that could occur. It will be possible for nfs4_file_put_access to race with nfs4_file_get_access. The refcount will go to zero (briefly) and then bumped back to one. If that happens we set ourselves up for a use-after-free and the potential for a lock to race onto the i_flock list as a filp is being torn down.

Ensure that we can safely bump the refcount on the file by holding the fi_lock whenever that's done. The only place it currently isn't is in get_lock_access.

In order to ensure atomicity with finding the file, use the find_*_file_locked variants and then call get_lock_access to get new access references on the nfs4_file under the same lock.

Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
fs/nfsd/nfs4state.c +14 -2
index 5f7294712ad4..8f320f2f8b84 100644
@@ -377,6 +377,8 @@ static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
 static void
 __nfs4_file_get_access(struct nfs4_file *fp, u32 access)
 {
+	lockdep_assert_held(&fp->fi_lock);
+
 	if (access & NFS4_SHARE_ACCESS_WRITE)
 		atomic_inc(&fp->fi_access[O_WRONLY]);
 	if (access & NFS4_SHARE_ACCESS_READ)

@@ -386,6 +388,8 @@ __nfs4_file_get_access(struct nfs4_file *fp, u32 access)
 static __be32
 nfs4_file_get_access(struct nfs4_file *fp, u32 access)
 {
+	lockdep_assert_held(&fp->fi_lock);
+
 	/* Does this access mode make sense? */
 	if (access & ~NFS4_SHARE_ACCESS_BOTH)
 		return nfserr_inval;

@@ -4572,6 +4576,8 @@ static void get_lock_access(struct nfs4_ol_stateid *lock_stp, u32 access)
 {
 	struct nfs4_file *fp = lock_stp->st_file;
 
+	lockdep_assert_held(&fp->fi_lock);
+
 	if (test_access(access, lock_stp))
 		return;
 	__nfs4_file_get_access(fp, access);

@@ -4623,6 +4629,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	struct nfs4_openowner *open_sop = NULL;
 	struct nfs4_lockowner *lock_sop = NULL;
 	struct nfs4_ol_stateid *lock_stp;
+	struct nfs4_file *fp;
 	struct file *filp = NULL;
 	struct file_lock *file_lock = NULL;
 	struct file_lock *conflock = NULL;

@@ -4703,20 +4710,25 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
+	fp = lock_stp->st_file;
 	locks_init_lock(file_lock);
 	switch (lock->lk_type) {
 		case NFS4_READ_LT:
 		case NFS4_READW_LT:
-			filp = find_readable_file(lock_stp->st_file);
+			spin_lock(&fp->fi_lock);
+			filp = find_readable_file_locked(fp);
 			if (filp)
 				get_lock_access(lock_stp, NFS4_SHARE_ACCESS_READ);
+			spin_unlock(&fp->fi_lock);
 			file_lock->fl_type = F_RDLCK;
 			break;
 		case NFS4_WRITE_LT:
 		case NFS4_WRITEW_LT:
-			filp = find_writeable_file(lock_stp->st_file);
+			spin_lock(&fp->fi_lock);
+			filp = find_writeable_file_locked(fp);
 			if (filp)
 				get_lock_access(lock_stp, NFS4_SHARE_ACCESS_WRITE);
+			spin_unlock(&fp->fi_lock);
 			file_lock->fl_type = F_WRLCK;
 			break;
 		default:
Signature · 1 line
1.9.3
author
#10 lore raw

PATCH 10/11 nfsd: make deny mode enforcement more efficient and close races in it

In reply to Jeff Layton

The current enforcement of deny modes is both inefficient and scattered across several places, which makes it hard to guarantee atomicity. The inefficiency is a problem now, and the lack of atomicity will mean races once the client_mutex is removed.

First, we address the inefficiency. We have to track deny modes on a per-stateid basis to ensure that open downgrades are sane, but when the server goes to enforce them it has to walk the entire list of stateids and check against each one.

Instead of doing that, maintain a per-nfs4_file deny mode. When a file is opened, we simply set any deny bits in that mode that were specified in the OPEN call. We can then use that unified deny mode to do a simple check to see whether there are any conflicts without needing to walk the entire stateid list.

The only time we'll need to walk the entire list of stateids is when a stateid that has a deny mode on it is being released, or one is having its deny mode downgraded. In that case, we must walk the entire list and recalculate the fi_share_deny field. Since deny modes are pretty rare today, this should be very rare under normal workloads.

To address the potential for races once the client_mutex is removed, protect fi_share_deny with the fi_lock. In nfs4_get_vfs_file, check to make sure that any deny mode we want to apply won't conflict with existing access. If that's ok, then have nfs4_file_get_access check that new access to the file won't conflict with existing deny modes.

If that also passes, then get file access references, set the correct access and deny bits in the stateid, and update the fi_share_deny field. If opening the file or truncating it fails, then unwind the whole mess and return the appropriate error.

Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 182 +++++++++++++++++++++++++++++++++++-----------------
 fs/nfsd/state.h     |   1 +
 2 files changed, 125 insertions(+), 58 deletions(-)
fs/nfsd/nfs4state.c +124 -58
index 8f320f2f8b84..da88b31c0afe 100644
@@ -394,10 +394,33 @@ nfs4_file_get_access(struct nfs4_file *fp, u32 access)
 	if (access & ~NFS4_SHARE_ACCESS_BOTH)
 		return nfserr_inval;
 
+	/* Does it conflict with a deny mode already set? */
+	if ((access & fp->fi_share_deny) != 0)
+		return nfserr_share_denied;
+
 	__nfs4_file_get_access(fp, access);
 	return nfs_ok;
 }
 
+static __be32 nfs4_file_check_deny(struct nfs4_file *fp, u32 deny)
+{
+	/* Common case is that there is no deny mode. */
+	if (deny) {
+		/* Does this deny mode make sense? */
+		if (deny & ~NFS4_SHARE_DENY_BOTH)
+			return nfserr_inval;
+
+		if ((deny & NFS4_SHARE_DENY_READ) &&
+		    atomic_read(&fp->fi_access[O_RDONLY]))
+			return nfserr_share_denied;
+
+		if ((deny & NFS4_SHARE_DENY_WRITE) &&
+		    atomic_read(&fp->fi_access[O_WRONLY]))
+			return nfserr_share_denied;
+	}
+	return nfs_ok;
+}
+
 static void __nfs4_file_put_access(struct nfs4_file *fp, int oflag)
 {
 	might_lock(&fp->fi_lock);

@@ -710,17 +733,6 @@ bmap_to_share_mode(unsigned long bmap) {
 	return access;
 }
 
-static bool
-test_share(struct nfs4_ol_stateid *stp, struct nfsd4_open *open) {
-	unsigned int access, deny;
-
-	access = bmap_to_share_mode(stp->st_access_bmap);
-	deny = bmap_to_share_mode(stp->st_deny_bmap);
-	if ((access & open->op_share_deny) || (deny & open->op_share_access))
-		return false;
-	return true;
-}
-
 /* set share access for a given stateid */
 static inline void
 set_access(u32 access, struct nfs4_ol_stateid *stp)

@@ -793,11 +805,49 @@ static int nfs4_access_to_omode(u32 access)
 	return O_RDONLY;
 }
 
+/*
+ * A stateid that had a deny mode associated with it is being released
+ * or downgraded. Recalculate the deny mode on the file.
+ */
+static void
+recalculate_deny_mode(struct nfs4_file *fp)
+{
+	struct nfs4_ol_stateid *stp;
+
+	spin_lock(&fp->fi_lock);
+	fp->fi_share_deny = 0;
+	list_for_each_entry(stp, &fp->fi_stateids, st_perfile)
+		fp->fi_share_deny |= bmap_to_share_mode(stp->st_deny_bmap);
+	spin_unlock(&fp->fi_lock);
+}
+
+static void
+reset_union_bmap_deny(u32 deny, struct nfs4_ol_stateid *stp)
+{
+	int i;
+	bool change = false;
+
+	for (i = 1; i < 4; i++) {
+		if ((i & deny) != i) {
+			change = true;
+			clear_deny(i, stp);
+		}
+	}
+
+	/* Recalculate per-file deny mode if there was a change */
+	if (change)
+		recalculate_deny_mode(stp->st_file);
+}
+
 /* release all access and file references for a given stateid */
 static void
 release_all_access(struct nfs4_ol_stateid *stp)
 {
 	int i;
+	struct nfs4_file *fp = stp->st_file;
+
+	if (fp && stp->st_deny_bmap != 0)
+		recalculate_deny_mode(fp);
 
 	for (i = 1; i < 4; i++) {
 		if (test_access(i, stp))

@@ -2787,6 +2837,7 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
 	fp->fi_inode = ino;
 	fp->fi_had_conflict = false;
 	fp->fi_lease = NULL;
+	fp->fi_share_deny = 0;
 	memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
 	memset(fp->fi_access, 0, sizeof(fp->fi_access));
 	hlist_add_head(&fp->fi_hash, &file_hashtbl[hashval]);

@@ -3014,22 +3065,15 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type)
 {
 	struct inode *ino = current_fh->fh_dentry->d_inode;
 	struct nfs4_file *fp;
-	struct nfs4_ol_stateid *stp;
-	__be32 ret;
+	__be32 ret = nfs_ok;
 
 	fp = find_file(ino);
 	if (!fp)
-		return nfs_ok;
-	ret = nfserr_locked;
-	/* Search for conflicting share reservations */
+		return ret;
+	/* Check for conflicting share reservations */
 	spin_lock(&fp->fi_lock);
-	list_for_each_entry(stp, &fp->fi_stateids, st_perfile) {
-		if (test_deny(deny_type, stp) ||
-		    test_deny(NFS4_SHARE_DENY_BOTH, stp))
-			goto out;
-	}
-	ret = nfs_ok;
-out:
+	if (fp->fi_share_deny & deny_type)
+		ret = nfserr_locked;
 	spin_unlock(&fp->fi_lock);
 	put_nfs4_file(fp);
 	return ret;

@@ -3265,12 +3309,9 @@ nfs4_check_open(struct nfs4_file *fp, struct nfsd4_open *open, struct nfs4_ol_st
 		if (local->st_stateowner->so_is_open_owner == 0)
 			continue;
 		/* remember if we have seen this open owner */
-		if (local->st_stateowner == &oo->oo_owner)
+		if (local->st_stateowner == &oo->oo_owner) {
 			*stpp = local;
-		/* check for conflicting share reservations */
-		if (!test_share(local, open)) {
-			spin_unlock(&fp->fi_lock);
-			return nfserr_share_denied;
+			break;
 		}
 	}
 	spin_unlock(&fp->fi_lock);

@@ -3311,56 +3352,91 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
 	__be32 status;
 	int oflag = nfs4_access_to_omode(open->op_share_access);
 	int access = nfs4_access_to_access(open->op_share_access);
+	unsigned char old_access_bmap, old_deny_bmap;
 
 	spin_lock(&fp->fi_lock);
+
+	/*
+	 * Are we trying to set a deny mode that would conflict with
+	 * current access?
+	 */
+	status = nfs4_file_check_deny(fp, open->op_share_deny);
+	if (status != nfs_ok) {
+		spin_unlock(&fp->fi_lock);
+		goto out;
+	}
+
+	/* set access to the file */
+	status = nfs4_file_get_access(fp, open->op_share_access);
+	if (status != nfs_ok) {
+		spin_unlock(&fp->fi_lock);
+		goto out;
+	}
+
+	/* Set access bits in stateid */
+	old_access_bmap = stp->st_access_bmap;
+	set_access(open->op_share_access, stp);
+
+	/* Set new deny mask */
+	old_deny_bmap = stp->st_deny_bmap;
+	set_deny(open->op_share_deny, stp);
+	fp->fi_share_deny |= (open->op_share_deny & NFS4_SHARE_DENY_BOTH);
+
 	if (!fp->fi_fds[oflag]) {
 		spin_unlock(&fp->fi_lock);
 		status = nfsd_open(rqstp, cur_fh, S_IFREG, access, &filp);
 		if (status)
-			goto out;
+			goto out_put_access;
 		spin_lock(&fp->fi_lock);
 		if (!fp->fi_fds[oflag]) {
 			fp->fi_fds[oflag] = filp;
 			filp = NULL;
 		}
 	}
-	status = nfs4_file_get_access(fp, open->op_share_access);
 	spin_unlock(&fp->fi_lock);
 	if (filp)
 		fput(filp);
-	if (status)
-		goto out_put_access;
 
 	status = nfsd4_truncate(rqstp, cur_fh, open);
 	if (status)
 		goto out_put_access;
-
-	/* Set access and deny bits in stateid */
-	set_access(open->op_share_access, stp);
-	set_deny(open->op_share_deny, stp);
-	return nfs_ok;
-
-out_put_access:
-	nfs4_file_put_access(fp, open->op_share_access);
 out:
 	return status;
+out_put_access:
+	stp->st_access_bmap = old_access_bmap;
+	nfs4_file_put_access(fp, open->op_share_access);
+	reset_union_bmap_deny(bmap_to_share_mode(old_deny_bmap), stp);
+	goto out;
 }
 
 static __be32
 nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *cur_fh, struct nfs4_ol_stateid *stp, struct nfsd4_open *open)
 {
 	__be32 status;
+	unsigned char old_deny_bmap;
 
 	if (!test_access(open->op_share_access, stp))
-		status = nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open);
-	else
-		status = nfsd4_truncate(rqstp, cur_fh, open);
+		return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open);
 
-	if (status)
+	/* test and set deny mode */
+	spin_lock(&fp->fi_lock);
+	status = nfs4_file_check_deny(fp, open->op_share_deny);
+	if (status == nfs_ok) {
+		old_deny_bmap = stp->st_deny_bmap;
+		set_deny(open->op_share_deny, stp);
+		fp->fi_share_deny |=
+				(open->op_share_deny & NFS4_SHARE_DENY_BOTH);
+	}
+	spin_unlock(&fp->fi_lock);
+
+	if (status != nfs_ok)
 		return status;
-	return nfs_ok;
-}
 
+	status = nfsd4_truncate(rqstp, cur_fh, open);
+	if (status != nfs_ok)
+		reset_union_bmap_deny(old_deny_bmap, stp);
+	return status;
+}
 
 static void
 nfs4_set_claim_prev(struct nfsd4_open *open, bool has_session)

@@ -3582,7 +3658,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 */
 	fp = find_or_add_file(ino, open->op_file);
 	if (fp != open->op_file) {
-		if ((status = nfs4_check_open(fp, open, &stp)))
+		status = nfs4_check_open(fp, open, &stp);
+		if (status)
 			goto out;
 		status = nfs4_check_deleg(cl, open, &dp);
 		if (status)

@@ -4269,17 +4346,6 @@ static inline void nfs4_stateid_downgrade(struct nfs4_ol_stateid *stp, u32 to_ac
 	}
 }
 
-static void
-reset_union_bmap_deny(u32 deny, struct nfs4_ol_stateid *stp)
-{
-	int i;
-
-	for (i = 1; i < 4; i++) {
-		if ((i & deny) != i)
-			clear_deny(i, stp);
-	}
-}
-
 __be32
 nfsd4_open_downgrade(struct svc_rqst *rqstp,
 		     struct nfsd4_compound_state *cstate,
fs/nfsd/state.h +1 -0
index 72aee4b4f1ae..015b972da8ba 100644
@@ -391,6 +391,7 @@ struct nfs4_file {
 	 *   + 1 to both of the above if NFS4_SHARE_ACCESS_BOTH is set.
 	 */
 	atomic_t		fi_access[2];
+	u32			fi_share_deny;
 	struct file		*fi_deleg_file;
 	struct file_lock	*fi_lease;
 	atomic_t		fi_delegees;
Signature · 1 line
1.9.3
#11 lore raw

Re: [PATCH 10/11] nfsd: make deny mode enforcement more efficient and close races in it

Quoted 16 lines · Jeff Layton

The current enforcement of deny modes is both inefficient and scattered across several places, which makes it hard to guarantee atomicity. The inefficiency is a problem now, and the lack of atomicity will mean races once the client_mutex is removed.

First, we address the inefficiency. We have to track deny modes on a per-stateid basis to ensure that open downgrades are sane, but when the server goes to enforce them it has to walk the entire list of stateids and check against each one.

Instead of doing that, maintain a per-nfs4_file deny mode. When a file is opened, we simply set any deny bits in that mode that were specified in the OPEN call. We can then use that unified deny mode to do a simple check to see whether there are any conflicts without needing to walk the entire stateid list.

Quoted 5 lines

The only time we'll need to walk the entire list of stateids is when a stateid that has a deny mode on it is being released, or one is having its deny mode downgraded. In that case, we must walk the entire list and recalculate the fi_share_deny field. Since deny modes are pretty rare today, this should be very rare under normal workloads.

Yeah, we only care about the bare minimum correctness for deny modes. Looks good!

--b.

Quoted 328 lines

To address the potential for races once the client_mutex is removed, protect fi_share_deny with the fi_lock. In nfs4_get_vfs_file, check to make sure that any deny mode we want to apply won't conflict with existing access. If that's ok, then have nfs4_file_get_access check that new access to the file won't conflict with existing deny modes.

If that also passes, then get file access references, set the correct access and deny bits in the stateid, and update the fi_share_deny field. If opening the file or truncating it fails, then unwind the whole mess and return the appropriate error.

Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 182 +++++++++++++++++++++++++++++++++++-----------------
 fs/nfsd/state.h     |   1 +
 2 files changed, 125 insertions(+), 58 deletions(-)
fs/nfsd/nfs4state.c +124 -58
index 8f320f2f8b84..da88b31c0afe 100644
@@ -394,10 +394,33 @@ nfs4_file_get_access(struct nfs4_file *fp, u32 access)
 	if (access & ~NFS4_SHARE_ACCESS_BOTH)
 		return nfserr_inval;
 
+	/* Does it conflict with a deny mode already set? */
+	if ((access & fp->fi_share_deny) != 0)
+		return nfserr_share_denied;
+
 	__nfs4_file_get_access(fp, access);
 	return nfs_ok;
 }
 
+static __be32 nfs4_file_check_deny(struct nfs4_file *fp, u32 deny)
+{
+	/* Common case is that there is no deny mode. */
+	if (deny) {
+		/* Does this deny mode make sense? */
+		if (deny & ~NFS4_SHARE_DENY_BOTH)
+			return nfserr_inval;
+
+		if ((deny & NFS4_SHARE_DENY_READ) &&
+		    atomic_read(&fp->fi_access[O_RDONLY]))
+			return nfserr_share_denied;
+
+		if ((deny & NFS4_SHARE_DENY_WRITE) &&
+		    atomic_read(&fp->fi_access[O_WRONLY]))
+			return nfserr_share_denied;
+	}
+	return nfs_ok;
+}
+
 static void __nfs4_file_put_access(struct nfs4_file *fp, int oflag)
 {
 	might_lock(&fp->fi_lock);

@@ -710,17 +733,6 @@ bmap_to_share_mode(unsigned long bmap) {
 	return access;
 }
 
-static bool
-test_share(struct nfs4_ol_stateid *stp, struct nfsd4_open *open) {
-	unsigned int access, deny;
-
-	access = bmap_to_share_mode(stp->st_access_bmap);
-	deny = bmap_to_share_mode(stp->st_deny_bmap);
-	if ((access & open->op_share_deny) || (deny & open->op_share_access))
-		return false;
-	return true;
-}
-
 /* set share access for a given stateid */
 static inline void
 set_access(u32 access, struct nfs4_ol_stateid *stp)

@@ -793,11 +805,49 @@ static int nfs4_access_to_omode(u32 access)
 	return O_RDONLY;
 }
 
+/*
+ * A stateid that had a deny mode associated with it is being released
+ * or downgraded. Recalculate the deny mode on the file.
+ */
+static void
+recalculate_deny_mode(struct nfs4_file *fp)
+{
+	struct nfs4_ol_stateid *stp;
+
+	spin_lock(&fp->fi_lock);
+	fp->fi_share_deny = 0;
+	list_for_each_entry(stp, &fp->fi_stateids, st_perfile)
+		fp->fi_share_deny |= bmap_to_share_mode(stp->st_deny_bmap);
+	spin_unlock(&fp->fi_lock);
+}
+
+static void
+reset_union_bmap_deny(u32 deny, struct nfs4_ol_stateid *stp)
+{
+	int i;
+	bool change = false;
+
+	for (i = 1; i < 4; i++) {
+		if ((i & deny) != i) {
+			change = true;
+			clear_deny(i, stp);
+		}
+	}
+
+	/* Recalculate per-file deny mode if there was a change */
+	if (change)
+		recalculate_deny_mode(stp->st_file);
+}
+
 /* release all access and file references for a given stateid */
 static void
 release_all_access(struct nfs4_ol_stateid *stp)
 {
 	int i;
+	struct nfs4_file *fp = stp->st_file;
+
+	if (fp && stp->st_deny_bmap != 0)
+		recalculate_deny_mode(fp);
 
 	for (i = 1; i < 4; i++) {
 		if (test_access(i, stp))

@@ -2787,6 +2837,7 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
 	fp->fi_inode = ino;
 	fp->fi_had_conflict = false;
 	fp->fi_lease = NULL;
+	fp->fi_share_deny = 0;
 	memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
 	memset(fp->fi_access, 0, sizeof(fp->fi_access));
 	hlist_add_head(&fp->fi_hash, &file_hashtbl[hashval]);

@@ -3014,22 +3065,15 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type)
 {
 	struct inode *ino = current_fh->fh_dentry->d_inode;
 	struct nfs4_file *fp;
-	struct nfs4_ol_stateid *stp;
-	__be32 ret;
+	__be32 ret = nfs_ok;
 
 	fp = find_file(ino);
 	if (!fp)
-		return nfs_ok;
-	ret = nfserr_locked;
-	/* Search for conflicting share reservations */
+		return ret;
+	/* Check for conflicting share reservations */
 	spin_lock(&fp->fi_lock);
-	list_for_each_entry(stp, &fp->fi_stateids, st_perfile) {
-		if (test_deny(deny_type, stp) ||
-		    test_deny(NFS4_SHARE_DENY_BOTH, stp))
-			goto out;
-	}
-	ret = nfs_ok;
-out:
+	if (fp->fi_share_deny & deny_type)
+		ret = nfserr_locked;
 	spin_unlock(&fp->fi_lock);
 	put_nfs4_file(fp);
 	return ret;

@@ -3265,12 +3309,9 @@ nfs4_check_open(struct nfs4_file *fp, struct nfsd4_open *open, struct nfs4_ol_st
 		if (local->st_stateowner->so_is_open_owner == 0)
 			continue;
 		/* remember if we have seen this open owner */
-		if (local->st_stateowner == &oo->oo_owner)
+		if (local->st_stateowner == &oo->oo_owner) {
 			*stpp = local;
-		/* check for conflicting share reservations */
-		if (!test_share(local, open)) {
-			spin_unlock(&fp->fi_lock);
-			return nfserr_share_denied;
+			break;
 		}
 	}
 	spin_unlock(&fp->fi_lock);

@@ -3311,56 +3352,91 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
 	__be32 status;
 	int oflag = nfs4_access_to_omode(open->op_share_access);
 	int access = nfs4_access_to_access(open->op_share_access);
+	unsigned char old_access_bmap, old_deny_bmap;
 
 	spin_lock(&fp->fi_lock);
+
+	/*
+	 * Are we trying to set a deny mode that would conflict with
+	 * current access?
+	 */
+	status = nfs4_file_check_deny(fp, open->op_share_deny);
+	if (status != nfs_ok) {
+		spin_unlock(&fp->fi_lock);
+		goto out;
+	}
+
+	/* set access to the file */
+	status = nfs4_file_get_access(fp, open->op_share_access);
+	if (status != nfs_ok) {
+		spin_unlock(&fp->fi_lock);
+		goto out;
+	}
+
+	/* Set access bits in stateid */
+	old_access_bmap = stp->st_access_bmap;
+	set_access(open->op_share_access, stp);
+
+	/* Set new deny mask */
+	old_deny_bmap = stp->st_deny_bmap;
+	set_deny(open->op_share_deny, stp);
+	fp->fi_share_deny |= (open->op_share_deny & NFS4_SHARE_DENY_BOTH);
+
 	if (!fp->fi_fds[oflag]) {
 		spin_unlock(&fp->fi_lock);
 		status = nfsd_open(rqstp, cur_fh, S_IFREG, access, &filp);
 		if (status)
-			goto out;
+			goto out_put_access;
 		spin_lock(&fp->fi_lock);
 		if (!fp->fi_fds[oflag]) {
 			fp->fi_fds[oflag] = filp;
 			filp = NULL;
 		}
 	}
-	status = nfs4_file_get_access(fp, open->op_share_access);
 	spin_unlock(&fp->fi_lock);
 	if (filp)
 		fput(filp);
-	if (status)
-		goto out_put_access;
 
 	status = nfsd4_truncate(rqstp, cur_fh, open);
 	if (status)
 		goto out_put_access;
-
-	/* Set access and deny bits in stateid */
-	set_access(open->op_share_access, stp);
-	set_deny(open->op_share_deny, stp);
-	return nfs_ok;
-
-out_put_access:
-	nfs4_file_put_access(fp, open->op_share_access);
 out:
 	return status;
+out_put_access:
+	stp->st_access_bmap = old_access_bmap;
+	nfs4_file_put_access(fp, open->op_share_access);
+	reset_union_bmap_deny(bmap_to_share_mode(old_deny_bmap), stp);
+	goto out;
 }
 
 static __be32
 nfs4_upgrade_open(struct svc_rqst *rqstp, struct nfs4_file *fp, struct svc_fh *cur_fh, struct nfs4_ol_stateid *stp, struct nfsd4_open *open)
 {
 	__be32 status;
+	unsigned char old_deny_bmap;
 
 	if (!test_access(open->op_share_access, stp))
-		status = nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open);
-	else
-		status = nfsd4_truncate(rqstp, cur_fh, open);
+		return nfs4_get_vfs_file(rqstp, fp, cur_fh, stp, open);
 
-	if (status)
+	/* test and set deny mode */
+	spin_lock(&fp->fi_lock);
+	status = nfs4_file_check_deny(fp, open->op_share_deny);
+	if (status == nfs_ok) {
+		old_deny_bmap = stp->st_deny_bmap;
+		set_deny(open->op_share_deny, stp);
+		fp->fi_share_deny |=
+				(open->op_share_deny & NFS4_SHARE_DENY_BOTH);
+	}
+	spin_unlock(&fp->fi_lock);
+
+	if (status != nfs_ok)
 		return status;
-	return nfs_ok;
-}
 
+	status = nfsd4_truncate(rqstp, cur_fh, open);
+	if (status != nfs_ok)
+		reset_union_bmap_deny(old_deny_bmap, stp);
+	return status;
+}
 
 static void
 nfs4_set_claim_prev(struct nfsd4_open *open, bool has_session)

@@ -3582,7 +3658,8 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 */
 	fp = find_or_add_file(ino, open->op_file);
 	if (fp != open->op_file) {
-		if ((status = nfs4_check_open(fp, open, &stp)))
+		status = nfs4_check_open(fp, open, &stp);
+		if (status)
 			goto out;
 		status = nfs4_check_deleg(cl, open, &dp);
 		if (status)

@@ -4269,17 +4346,6 @@ static inline void nfs4_stateid_downgrade(struct nfs4_ol_stateid *stp, u32 to_ac
 	}
 }
 
-static void
-reset_union_bmap_deny(u32 deny, struct nfs4_ol_stateid *stp)
-{
-	int i;
-
-	for (i = 1; i < 4; i++) {
-		if ((i & deny) != i)
-			clear_deny(i, stp);
-	}
-}
-
 __be32
 nfsd4_open_downgrade(struct svc_rqst *rqstp,
 		     struct nfsd4_compound_state *cstate,
fs/nfsd/state.h +1 -0
index 72aee4b4f1ae..015b972da8ba 100644
@@ -391,6 +391,7 @@ struct nfs4_file {
 	 *   + 1 to both of the above if NFS4_SHARE_ACCESS_BOTH is set.
 	 */
 	atomic_t		fi_access[2];
+	u32			fi_share_deny;
 	struct file		*fi_deleg_file;
 	struct file_lock	*fi_lease;
 	atomic_t		fi_delegees;
Signature · 1 line
1.9.3
#12 lore raw

RE: [PATCH 10/11] nfsd: make deny mode enforcement more efficient and close races in it

In reply to Jeff Layton
Quoted 3 lines

The current enforcement of deny modes is both inefficient and scattered across several places, which makes it hard to guarantee atomicity. The inefficiency is a problem now, and the lack of atomicity will mean races

once

Quoted 9 lines

the client_mutex is removed.

First, we address the inefficiency. We have to track deny modes on a per- stateid basis to ensure that open downgrades are sane, but when the server goes to enforce them it has to walk the entire list of stateids and check against each one.

Instead of doing that, maintain a per-nfs4_file deny mode. When a file is opened, we simply set any deny bits in that mode that were specified in

the

Quoted 5 lines

OPEN call. We can then use that unified deny mode to do a simple check to see whether there are any conflicts without needing to walk the entire stateid list.

The only time we'll need to walk the entire list of stateids is when a

stateid

Quoted 1 line

that has a deny mode on it is being released, or one is having its deny

mode

Quoted 2 lines

downgraded. In that case, we must walk the entire list and recalculate the fi_share_deny field. Since deny modes are pretty rare today, this should

be

Quoted 1 line

very rare under normal workloads.

What we do in Ganesha to avoid walking the list of stateids on release is maintain the effective deny (and access) mode not at bits, but as a counter for each bit. Thus, to remove a SHARE_ACCESS_READ | SHARE_DENY_WRITE, you decrement the counts for access_read and deny_write.

Frank

#13 lore raw

Re: [PATCH 10/11] nfsd: make deny mode enforcement more efficient and close races in it

Quoted 25 lines · On Fri, 11 Jul 2014 10:31:26 -0700 "Frank Filz" <[email protected]> wrote:
Quoted 3 lines

The current enforcement of deny modes is both inefficient and scattered across several places, which makes it hard to guarantee atomicity. The inefficiency is a problem now, and the lack of atomicity will mean races

once

Quoted 9 lines

the client_mutex is removed.

First, we address the inefficiency. We have to track deny modes on a per- stateid basis to ensure that open downgrades are sane, but when the server goes to enforce them it has to walk the entire list of stateids and check against each one.

Instead of doing that, maintain a per-nfs4_file deny mode. When a file is opened, we simply set any deny bits in that mode that were specified in

the

Quoted 5 lines

OPEN call. We can then use that unified deny mode to do a simple check to see whether there are any conflicts without needing to walk the entire stateid list.

The only time we'll need to walk the entire list of stateids is when a

stateid

Quoted 1 line

that has a deny mode on it is being released, or one is having its deny

mode

Quoted 2 lines

downgraded. In that case, we must walk the entire list and recalculate the fi_share_deny field. Since deny modes are pretty rare today, this should

be

Quoted 10 lines
Quoted 1 line

very rare under normal workloads.

What we do in Ganesha to avoid walking the list of stateids on release is maintain the effective deny (and access) mode not at bits, but as a counter for each bit. Thus, to remove a SHARE_ACCESS_READ | SHARE_DENY_WRITE, you decrement the counts for access_read and deny_write.

Frank

Sure, that's another possibility that I considered, but I didn't want to be bothered with having to add counters for deny modes. In practice there are *no* clients that use them (aside from pynfs and maybe the semi-mythical Windows v4.1 client).

With this scheme, deny mode enforcement is pretty darned efficient, particularly in the common case where there are no deny modes to enforce.

Any penalty for the use of deny modes is generally paid during the CLOSE or OPEN_DOWNGRADE on behalf of the client that's using them. Any RPC from a client that's not won't need to do any extra work (aside from maybe spinning on the fi_lock while another client is having to recalculate the fi_share_deny).

Signature · 1 line
Jeff Layton <[email protected]>
#14 lore raw

RE: [PATCH 10/11] nfsd: make deny mode enforcement more efficient and close races in it

Quoted 39 lines
Quoted 29 lines · On Fri, 11 Jul 2014 10:31:26 -0700 "Frank Filz" <[email protected]> wrote:
Quoted 4 lines

The current enforcement of deny modes is both inefficient and scattered across several places, which makes it hard to guarantee atomicity. The inefficiency is a problem now, and the lack of atomicity will mean races

once

Quoted 10 lines

the client_mutex is removed.

First, we address the inefficiency. We have to track deny modes on a per- stateid basis to ensure that open downgrades are sane, but when the server goes to enforce them it has to walk the entire list of stateids and check against each one.

Instead of doing that, maintain a per-nfs4_file deny mode. When a file is opened, we simply set any deny bits in that mode that were specified in

the

Quoted 6 lines

OPEN call. We can then use that unified deny mode to do a simple check to see whether there are any conflicts without needing to walk the entire stateid list.

The only time we'll need to walk the entire list of stateids is when a

stateid

Quoted 2 lines

that has a deny mode on it is being released, or one is having its deny

mode

Quoted 3 lines

downgraded. In that case, we must walk the entire list and recalculate the fi_share_deny field. Since deny modes are pretty rare today, this should

Quoted 7 lines

be

Quoted 1 line

very rare under normal workloads.

What we do in Ganesha to avoid walking the list of stateids on release is maintain the effective deny (and access) mode not at bits, but as a counter for each bit. Thus, to remove a SHARE_ACCESS_READ | SHARE_DENY_WRITE, you decrement the counts for access_read and

Quoted 7 lines

deny_write.

Quoted 4 lines

Frank

Sure, that's another possibility that I considered, but I didn't want to

be

Quoted 8 lines

bothered with having to add counters for deny modes. In practice there are *no* clients that use them (aside from pynfs and maybe the semi-mythical Windows v4.1 client).

With this scheme, deny mode enforcement is pretty darned efficient, particularly in the common case where there are no deny modes to enforce.

Any penalty for the use of deny modes is generally paid during the CLOSE

or

Quoted 2 lines

OPEN_DOWNGRADE on behalf of the client that's using them. Any RPC from a client that's not won't need to do any extra work (aside

from

Quoted 1 line

maybe spinning on the fi_lock while another client is having to

recalculate the

Quoted 1 line

fi_share_deny).

Good point.

Whatever happened to Pavel Shilovsky's O_DENY patch set? I was looking forward to that for allowing Ganesha and Samba share reservations to more fully interact with each other...

Frank

#15 lore raw

Re: [PATCH 10/11] nfsd: make deny mode enforcement more efficient and close races in it

Quoted 39 lines · Frank Filz
Quoted 39 lines
Quoted 36 lines · On Fri, 11 Jul 2014 10:31:26 -0700 "Frank Filz" <[email protected]> wrote:
Quoted 4 lines

The current enforcement of deny modes is both inefficient and scattered across several places, which makes it hard to guarantee atomicity. The inefficiency is a problem now, and the lack of atomicity will mean races

once

Quoted 10 lines

the client_mutex is removed.

First, we address the inefficiency. We have to track deny modes on a per- stateid basis to ensure that open downgrades are sane, but when the server goes to enforce them it has to walk the entire list of stateids and check against each one.

Instead of doing that, maintain a per-nfs4_file deny mode. When a file is opened, we simply set any deny bits in that mode that were specified in

the

Quoted 6 lines

OPEN call. We can then use that unified deny mode to do a simple check to see whether there are any conflicts without needing to walk the entire stateid list.

The only time we'll need to walk the entire list of stateids is when a

stateid

Quoted 2 lines

that has a deny mode on it is being released, or one is having its deny

mode

Quoted 3 lines

downgraded. In that case, we must walk the entire list and recalculate the fi_share_deny field. Since deny modes are pretty rare today, this should

be

Quoted 1 line

very rare under normal workloads.

What we do in Ganesha to avoid walking the list of stateids on release is maintain the effective deny (and access) mode not at bits, but as a counter for each bit. Thus, to remove a SHARE_ACCESS_READ | SHARE_DENY_WRITE, you decrement the counts for access_read and

Quoted 11 lines
Quoted 7 lines

deny_write.

Quoted 4 lines

Frank

Sure, that's another possibility that I considered, but I didn't want to

be

Quoted 3 lines

bothered with having to add counters for deny modes. In practice there are *no* clients that use them (aside from pynfs and maybe the semi-mythical Windows v4.1 client).

You don't need counters for deny modes. There can only be 1 of each, since any deny mode has to be part of an OPEN that has at least one non-zero share access mode. So a single bit for each should be fine.

Quoted 26 lines
Quoted 5 lines

With this scheme, deny mode enforcement is pretty darned efficient, particularly in the common case where there are no deny modes to enforce.

Any penalty for the use of deny modes is generally paid during the CLOSE

or

Quoted 2 lines

OPEN_DOWNGRADE on behalf of the client that's using them. Any RPC from a client that's not won't need to do any extra work (aside

from

Quoted 1 line

maybe spinning on the fi_lock while another client is having to

recalculate the

Quoted 1 line

fi_share_deny).

Good point.

Whatever happened to Pavel Shilovsky's O_DENY patch set? I was looking forward to that for allowing Ganesha and Samba share reservations to more fully interact with each other...

Frank

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

Signature · 5 lines
Trond Myklebust

Linux NFS client maintainer, PrimaryData

[email protected]
#16 lore raw

Re: [PATCH 10/11] nfsd: make deny mode enforcement more efficient and close races in it

Quoted 47 lines · On Fri, 11 Jul 2014 14:00:43 -0400 Trond Myklebust…
Quoted 46 lines · Frank Filz
Quoted 46 lines
Quoted 29 lines · On Fri, 11 Jul 2014 10:31:26 -0700 "Frank Filz" <[email protected]> wrote:
Quoted 4 lines

The current enforcement of deny modes is both inefficient and scattered across several places, which makes it hard to guarantee atomicity. The inefficiency is a problem now, and the lack of atomicity will mean races

once

Quoted 10 lines

the client_mutex is removed.

First, we address the inefficiency. We have to track deny modes on a per- stateid basis to ensure that open downgrades are sane, but when the server goes to enforce them it has to walk the entire list of stateids and check against each one.

Instead of doing that, maintain a per-nfs4_file deny mode. When a file is opened, we simply set any deny bits in that mode that were specified in

the

Quoted 6 lines

OPEN call. We can then use that unified deny mode to do a simple check to see whether there are any conflicts without needing to walk the entire stateid list.

The only time we'll need to walk the entire list of stateids is when a

stateid

Quoted 2 lines

that has a deny mode on it is being released, or one is having its deny

mode

Quoted 3 lines

downgraded. In that case, we must walk the entire list and recalculate the fi_share_deny field. Since deny modes are pretty rare today, this should

Quoted 7 lines

be

Quoted 1 line

very rare under normal workloads.

What we do in Ganesha to avoid walking the list of stateids on release is maintain the effective deny (and access) mode not at bits, but as a counter for each bit. Thus, to remove a SHARE_ACCESS_READ | SHARE_DENY_WRITE, you decrement the counts for access_read and

deny_write.

Quoted 4 lines

Frank

Sure, that's another possibility that I considered, but I didn't want to

Quoted 9 lines
Quoted 4 lines

be

Quoted 3 lines

bothered with having to add counters for deny modes. In practice there are *no* clients that use them (aside from pynfs and maybe the semi-mythical Windows v4.1 client).

You don't need counters for deny modes. There can only be 1 of each, since any deny mode has to be part of an OPEN that has at least one non-zero share access mode. So a single bit for each should be fine.

I'm not sure that's 100% true.

I see no reason that you can't have several clients open the same file with (e.g.) ACCESS_READ and DENY_WRITE. You wouldn't want to lift the DENY_WRITE on the file until all of those clients have closed the file (or downgraded to remove the DENY_WRITE access).

So, I think you do need a little more than just a single set of bits on the file. You either need to use per-file counters for them there (which it sounds like ganesha does) or track them on a per-stateid basis (like knfsd does).

Quoted 29 lines
Quoted 26 lines
Quoted 5 lines

With this scheme, deny mode enforcement is pretty darned efficient, particularly in the common case where there are no deny modes to enforce.

Any penalty for the use of deny modes is generally paid during the CLOSE

or

Quoted 2 lines

OPEN_DOWNGRADE on behalf of the client that's using them. Any RPC from a client that's not won't need to do any extra work (aside

from

Quoted 1 line

maybe spinning on the fi_lock while another client is having to

recalculate the

Quoted 1 line

fi_share_deny).

Good point.

Whatever happened to Pavel Shilovsky's O_DENY patch set? I was looking forward to that for allowing Ganesha and Samba share reservations to more fully interact with each other...

Frank

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

Signature · 1 line
Jeff Layton <[email protected]>
#17 lore raw

RE: [PATCH 10/11] nfsd: make deny mode enforcement more efficient and close races in it

In reply to Trond Myklebust
Quoted 52 lines · Frank Filz
Quoted 33 lines
Quoted 33 lines
Quoted 30 lines · On Fri, 11 Jul 2014 10:31:26 -0700 "Frank Filz" <[email protected]> wrote:
Quoted 4 lines

The current enforcement of deny modes is both inefficient and scattered across several places, which makes it hard to guarantee atomicity. The inefficiency is a problem now, and the lack of atomicity will mean races

once

Quoted 11 lines

the client_mutex is removed.

First, we address the inefficiency. We have to track deny modes on a
per- stateid basis to ensure that open downgrades are sane, but when the server goes to enforce them it has to walk the entire list of stateids and check against each one.

Instead of doing that, maintain a per-nfs4_file deny mode. When a file is opened, we simply set any deny bits in that mode that were specified in

the

Quoted 6 lines

OPEN call. We can then use that unified deny mode to do a simple check to see whether there are any conflicts without needing to walk the entire stateid list.

The only time we'll need to walk the entire list of stateids is when a

stateid

Quoted 2 lines

that has a deny mode on it is being released, or one is having its deny

mode

Quoted 3 lines

downgraded. In that case, we must walk the entire list and recalculate the fi_share_deny field. Since deny modes are pretty rare today, this should

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

be

Quoted 1 line

very rare under normal workloads.

What we do in Ganesha to avoid walking the list of stateids on release is maintain the effective deny (and access) mode not at bits, but as a counter for each bit. Thus, to remove a SHARE_ACCESS_READ | SHARE_DENY_WRITE, you decrement the

counts for

Quoted 10 lines
Quoted 9 lines
Quoted 1 line

access_read and

deny_write.

Quoted 4 lines

Frank

Sure, that's another possibility that I considered, but I didn't want to

be

Quoted 8 lines
Quoted 4 lines
Quoted 4 lines

bothered with having to add counters for deny modes. In practice there are
*no* clients that use them (aside from pynfs and maybe the semi-mythical Windows v4.1 client).

You don't need counters for deny modes. There can only be 1 of each, since any deny mode has to be part of an OPEN that has at least one non-zero share access mode. So a single bit for each should be fine.

You can have any number of:

OPEN SHARE_ACCESS_READ, SHARE_DENY_WRITE...

It's the share reservation equivalent of a read lock...

Frank

Quoted 39 lines
Quoted 3 lines
Quoted 3 lines

With this scheme, deny mode enforcement is pretty darned efficient, particularly in the common case where there are no deny modes to

enforce.

Quoted 18 lines
Quoted 3 lines

Any penalty for the use of deny modes is generally paid during the CLOSE

or

Quoted 3 lines

OPEN_DOWNGRADE on behalf of the client that's using them. Any RPC from a client that's not won't need to do any extra work (aside

from

Quoted 1 line

maybe spinning on the fi_lock while another client is having to

recalculate the

Quoted 1 line

fi_share_deny).

Good point.

Whatever happened to Pavel Shilovsky's O_DENY patch set? I was looking forward to that for allowing Ganesha and Samba share reservations to more fully interact with each other...

Quoted 6 lines

Frank

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More

majordomo

Quoted 1 line

info at http://vger.kernel.org/majordomo-info.html

--
Trond Myklebust

Linux NFS client maintainer, PrimaryData

[email protected]

author
#18 lore raw

PATCH 11/11 nfsd: cleanup and rename nfs4_check_open

In reply to Jeff Layton

Rename it to better describe what it does, and have it just return the stateid instead of a __be32 (which is now always nfs_ok). Also, do the search for an existing stateid after the delegation check, to reduce cleanup if the delegation check returns error.

Signed-off-by Jeff Layton <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 fs/nfsd/nfs4state.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
fs/nfsd/nfs4state.c +6 -9
index da88b31c0afe..225f98c7d00d 100644
@@ -3297,10 +3297,10 @@ out:
 	return nfs_ok;
 }
 
-static __be32
-nfs4_check_open(struct nfs4_file *fp, struct nfsd4_open *open, struct nfs4_ol_stateid **stpp)
+static struct nfs4_ol_stateid *
+nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
 {
-	struct nfs4_ol_stateid *local;
+	struct nfs4_ol_stateid *local, *ret = NULL;
 	struct nfs4_openowner *oo = open->op_openowner;
 
 	spin_lock(&fp->fi_lock);

@@ -3308,14 +3308,13 @@ nfs4_check_open(struct nfs4_file *fp, struct nfsd4_open *open, struct nfs4_ol_st
 		/* ignore lock owners */
 		if (local->st_stateowner->so_is_open_owner == 0)
 			continue;
-		/* remember if we have seen this open owner */
 		if (local->st_stateowner == &oo->oo_owner) {
-			*stpp = local;
+			ret = local;
 			break;
 		}
 	}
 	spin_unlock(&fp->fi_lock);
-	return nfs_ok;
+	return ret;
 }
 
 static inline int nfs4_access_to_access(u32 nfs4_access)

@@ -3658,12 +3657,10 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 */
 	fp = find_or_add_file(ino, open->op_file);
 	if (fp != open->op_file) {
-		status = nfs4_check_open(fp, open, &stp);
-		if (status)
-			goto out;
 		status = nfs4_check_deleg(cl, open, &dp);
 		if (status)
 			goto out;
+		stp = nfsd4_find_existing_open(fp, open);
 	} else {
 		open->op_file = NULL;
 		status = nfserr_bad_stateid;
Signature · 1 line
1.9.3
#19 lore raw
In reply to Jeff Layton
Quoted 4 lines · Jeff Layton

Here is the deny mode handling overhaul broken out into a separate series, as requested by Christoph. I've tried to address most of his review comments, but I may have missed some. Let me know if if I did.

They look fine to me; I'll merge them soon absent any objections from Christoph.

--b.

Quoted 31 lines

Part of the client_mutex removal series involves cleaning up the handling of deny modes in the server in order to setting and enforcement atomic.

This does add a new per-nfs4_file spinlock, but in principle it should never be contended while the client_mutex is still wrapped around all of this code.

Jeff Layton (8):
nfsd: refactor nfs4_file_get_access and nfs4_file_put_access
nfsd: remove nfs4_file_put_fd
nfsd: shrink st_access_bmap and st_deny_bmap
nfsd: set stateid access and deny bits in nfs4_get_vfs_file
nfsd: clean up reset_union_bmap_deny
nfsd: always hold the fi_lock when bumping fi_access refcounts
nfsd: make deny mode enforcement more efficient and close races in it
nfsd: cleanup and rename nfs4_check_open

Trond Myklebust (3):
nfsd: Add fine grained protection for the nfs4_file->fi_stateids list
nfsd: Add locking to the nfs4_file->fi_fds[] array
nfsd: clean up helper __release_lock_stateid

 fs/nfsd/nfs4state.c | 437 ++++++++++++++++++++++++++++++++++++++--------------
 fs/nfsd/state.h     |  32 +---
 2 files changed, 325 insertions(+), 144 deletions(-)
Signature · 1 line
1.9.3
Reviewed-by
#20 lore raw
Quoted 8 lines · J. Bruce Fields
Quoted 4 lines · Jeff Layton

Here is the deny mode handling overhaul broken out into a separate series, as requested by Christoph. I've tried to address most of his review comments, but I may have missed some. Let me know if if I did.

They look fine to me; I'll merge them soon absent any objections from Christoph.

The whole series looks good, although I've not tested it yet, just the original full series.

Reviewed-by Christoph Hellwig <[email protected]>
#21 lore raw
Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · J. Bruce Fields
Quoted 4 lines · Jeff Layton

Here is the deny mode handling overhaul broken out into a separate series, as requested by Christoph. I've tried to address most of his review comments, but I may have missed some. Let me know if if I did.

Quoted 7 lines
Quoted 2 lines

They look fine to me; I'll merge them soon absent any objections from Christoph.

The whole series looks good, although I've not tested it yet, just the original full series.

Reviewed-by Christoph Hellwig <[email protected]>

Thanks!

--b.

#22 lore raw
Quoted 11 lines · On Fri, 11 Jul 2014 10:31:28 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 10 lines · Christoph Hellwig
Quoted 8 lines · J. Bruce Fields
Quoted 4 lines · Jeff Layton

Here is the deny mode handling overhaul broken out into a separate series, as requested by Christoph. I've tried to address most of his review comments, but I may have missed some. Let me know if if I did.

They look fine to me; I'll merge them soon absent any objections from Christoph.

Quoted 7 lines
Quoted 4 lines

The whole series looks good, although I've not tested it yet, just the original full series.

Reviewed-by Christoph Hellwig <[email protected]>

Thanks!

Yes, thanks to both of you. The review and merging are much appreciated. Only 89 patches to go once these go in!

Signature · 1 line
Jeff Layton <[email protected]>
#23 lore raw
Quoted 2 lines · Jeff Layton

Yes, thanks to both of you. The review and merging are much appreciated. Only 89 patches to go once these go in!

Might help if we can get them into slighly smaller reviewable chunks, e.g. the next batch might be the stateid refcounting and everything around it, deferring the various client and session related bits and the fault injection changes for now.

#24 lore raw
Quoted 8 lines · On Sun, 13 Jul 2014 04:42:44 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 2 lines · Jeff Layton

Yes, thanks to both of you. The review and merging are much appreciated. Only 89 patches to go once these go in!

Might help if we can get them into slighly smaller reviewable chunks, e.g. the next batch might be the stateid refcounting and everything around it, deferring the various client and session related bits and the fault injection changes for now.

That should be doable as long as we're ok with adding in superfluous spinlocking until the client_mutex is removed. I'll work on breaking out the next set. It might be a few days as I'm still looking at whether it's feasible to not alter the sc_type like the code does today.

Signature · 1 line
Jeff Layton <[email protected]>
#25 lore raw
Quoted 7 lines · Jeff Layton
Quoted 4 lines · On Sun, 13 Jul 2014 04:42:44 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 2 lines · Jeff Layton

Yes, thanks to both of you. The review and merging are much appreciated. Only 89 patches to go once these go in!

Quoted 7 lines
Quoted 4 lines

Might help if we can get them into slighly smaller reviewable chunks, e.g. the next batch might be the stateid refcounting and everything around it, deferring the various client and session related bits and the fault injection changes for now.

That should be doable as long as we're ok with adding in superfluous spinlocking until the client_mutex is removed.

It means that an attempt to bisect any race introduced by the series will land on the one patch that drops the state lock.

But maybe that's the best we can do.

--b.

Quoted 3 lines

I'll work on breaking
out the next set. It might be a few days as I'm still looking at whether it's feasible to not alter the sc_type like the code does today.

#26 lore raw
Quoted 7 lines · J. Bruce Fields
Quoted 2 lines

That should be doable as long as we're ok with adding in superfluous spinlocking until the client_mutex is removed.

It means that an attempt to bisect any race introduced by the series will land on the one patch that drops the state lock.

But maybe that's the best we can do.

That's pretty much exactly how all the earlier series are structured as well.