| Message ID | 1403189450-18729-6-git-send-email-jlayton@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 19, 2014 at 10:49:11AM -0400, Jeff Layton wrote: > From: Trond Myklebust <trond.myklebust@primarydata.com> > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> Looks good, but should have a patch description that better tells why this is done (looks like preparing for nfs_lock_state() removal again, not fixing some existing race) Reviewed-by: Christoph Hellwig <hch@lst.de> -- 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
On Mon, 23 Jun 2014 09:04:07 -0700 Christoph Hellwig <hch@infradead.org> wrote: > On Thu, Jun 19, 2014 at 10:49:11AM -0400, Jeff Layton wrote: > > From: Trond Myklebust <trond.myklebust@primarydata.com> > > > > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> > > Looks good, but should have a patch description that better tells > why this is done (looks like preparing for nfs_lock_state() removal > again, not fixing some existing race) > > Reviewed-by: Christoph Hellwig <hch@lst.de> Correct. I'll add that into the desciption if you think it's warranted, but again that's the case with many of the patches in this series.
On Mon, Jun 23, 2014 at 12:20:39PM -0400, Jeff Layton wrote: > Correct. I'll add that into the desciption if you think it's warranted, > but again that's the case with many of the patches in this series. It might be fairly obvious in the context of the series, it's not if someone goes back in history with a git-blame. And it's not a 100% obvious with the series either, at least earlier versions also fixes pre-existing races, although all of that might be upstream now. So as far as I am concerned a simple one-line blurb mentioning what the new locking is for would be useful to be added to all patches just changing the locking. -- 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
On Mon, 23 Jun 2014 09:23:32 -0700 Christoph Hellwig <hch@infradead.org> wrote: > On Mon, Jun 23, 2014 at 12:20:39PM -0400, Jeff Layton wrote: > > Correct. I'll add that into the desciption if you think it's warranted, > > but again that's the case with many of the patches in this series. > > It might be fairly obvious in the context of the series, it's not > if someone goes back in history with a git-blame. > > And it's not a 100% obvious with the series either, at least earlier > versions also fixes pre-existing races, although all of that might be > upstream now. > > So as far as I am concerned a simple one-line blurb mentioning what > the new locking is for would be useful to be added to all patches > just changing the locking. Fair enough. I'll do that. Thanks,
On Mon, Jun 23, 2014 at 12:24:58PM -0400, Jeff Layton wrote: > On Mon, 23 Jun 2014 09:23:32 -0700 > Christoph Hellwig <hch@infradead.org> wrote: > > > On Mon, Jun 23, 2014 at 12:20:39PM -0400, Jeff Layton wrote: > > > Correct. I'll add that into the desciption if you think it's warranted, > > > but again that's the case with many of the patches in this series. > > > > It might be fairly obvious in the context of the series, it's not > > if someone goes back in history with a git-blame. > > > > And it's not a 100% obvious with the series either, at least earlier > > versions also fixes pre-existing races, although all of that might be > > upstream now. > > > > So as far as I am concerned a simple one-line blurb mentioning what > > the new locking is for would be useful to be added to all patches > > just changing the locking. > > Fair enough. I'll do that. I appreciate that it's tedious but yes, thanks, that would be helpful. --b. -- 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 --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 8dc1289a7b74..d1b975b29334 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -625,7 +625,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); } @@ -2672,7 +2676,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_lockowners); 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; @@ -2681,6 +2684,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 @@ -2788,6 +2794,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)) @@ -2795,6 +2802,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; } @@ -2993,6 +3001,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) @@ -3001,9 +3010,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; } @@ -4308,7 +4320,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); @@ -4316,6 +4327,9 @@ alloc_init_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp, struct stp->st_access_bmap = 0; stp->st_deny_bmap = open_stp->st_deny_bmap; stp->st_openstp = open_stp; + spin_lock(&fp->fi_lock); + list_add(&stp->st_perfile, &fp->fi_stateids); + spin_unlock(&fp->fi_lock); return stp; }