Return-Path: linux-nfs-owner@vger.kernel.org Received: from fieldses.org ([174.143.236.118]:50813 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752795AbaGJUIO (ORCPT ); Thu, 10 Jul 2014 16:08:14 -0400 Date: Thu, 10 Jul 2014 16:08:09 -0400 From: "J. Bruce Fields" To: Jeff Layton Cc: hch@infradead.org, linux-nfs@vger.kernel.org Subject: Re: [PATCH 10/11] nfsd: make deny mode enforcement more efficient and close races in it Message-ID: <20140710200809.GE26561@fieldses.org> References: <1405015655-12469-1-git-send-email-jlayton@primarydata.com> <1405015655-12469-11-git-send-email-jlayton@primarydata.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1405015655-12469-11-git-send-email-jlayton@primarydata.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Thu, Jul 10, 2014 at 02:07:34PM -0400, Jeff Layton wrote: > 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. Yeah, we only care about the bare minimum correctness for deny modes. Looks good! --b. > > 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 > --- > fs/nfsd/nfs4state.c | 182 +++++++++++++++++++++++++++++++++++----------------- > fs/nfsd/state.h | 1 + > 2 files changed, 125 insertions(+), 58 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 8f320f2f8b84..da88b31c0afe 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -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, > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index 72aee4b4f1ae..015b972da8ba 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -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; > -- > 1.9.3 >