* delegation patches for 3.14
@ 2014-01-26 20:59 J. Bruce Fields
2014-01-26 20:59 ` [PATCH 1/3] nfsd4: minor nfs4_setlease cleanup J. Bruce Fields
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: J. Bruce Fields @ 2014-01-26 20:59 UTC (permalink / raw)
To: linux-nfs
These patches do a little minor cleanup and close a small race left
after the vfs delegation patches.
--b.
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/3] nfsd4: minor nfs4_setlease cleanup 2014-01-26 20:59 delegation patches for 3.14 J. Bruce Fields @ 2014-01-26 20:59 ` J. Bruce Fields 2014-01-26 20:59 ` [PATCH 2/3] nfsd4: delay setting current_fh in open J. Bruce Fields 2014-01-26 20:59 ` [PATCH 3/3] nfsd4: fix delegation-unlink/rename race J. Bruce Fields 2 siblings, 0 replies; 5+ messages in thread From: J. Bruce Fields @ 2014-01-26 20:59 UTC (permalink / raw) To: linux-nfs; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> As far as I can tell, this list is used only under the state lock, so we may as well do this in the simpler order. Acked-by: Jeff Layton <jlayton@redhat.com> Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 5795d5f..ed3085b 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3043,18 +3043,18 @@ static int nfs4_setlease(struct nfs4_delegation *dp) if (!fl) return -ENOMEM; fl->fl_file = find_readable_file(fp); - list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations); status = vfs_setlease(fl->fl_file, fl->fl_type, &fl); - if (status) { - list_del_init(&dp->dl_perclnt); - locks_free_lock(fl); - return status; - } + if (status) + goto out_free; + list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations); fp->fi_lease = fl; fp->fi_deleg_file = get_file(fl->fl_file); atomic_set(&fp->fi_delegees, 1); list_add(&dp->dl_perfile, &fp->fi_delegations); return 0; +out_free: + locks_free_lock(fl); + return status; } static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp) -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] nfsd4: delay setting current_fh in open 2014-01-26 20:59 delegation patches for 3.14 J. Bruce Fields 2014-01-26 20:59 ` [PATCH 1/3] nfsd4: minor nfs4_setlease cleanup J. Bruce Fields @ 2014-01-26 20:59 ` J. Bruce Fields 2014-01-26 20:59 ` [PATCH 3/3] nfsd4: fix delegation-unlink/rename race J. Bruce Fields 2 siblings, 0 replies; 5+ messages in thread From: J. Bruce Fields @ 2014-01-26 20:59 UTC (permalink / raw) To: linux-nfs; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> This is basically a no-op, to simplify a following patch. Acked-by: Jeff Layton <jlayton@redhat.com> Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4proc.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index dadff09..844813a 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -230,17 +230,16 @@ static void nfsd4_set_open_owner_reply_cache(struct nfsd4_compound_state *cstate } static __be32 -do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open) +do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh) { struct svc_fh *current_fh = &cstate->current_fh; - struct svc_fh *resfh; int accmode; __be32 status; - resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL); - if (!resfh) + *resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL); + if (!*resfh) return nfserr_jukebox; - fh_init(resfh, NFS4_FHSIZE); + fh_init(*resfh, NFS4_FHSIZE); open->op_truncate = 0; if (open->op_create) { @@ -265,12 +264,12 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru */ status = do_nfsd_create(rqstp, current_fh, open->op_fname.data, open->op_fname.len, &open->op_iattr, - resfh, open->op_createmode, + *resfh, open->op_createmode, (u32 *)open->op_verf.data, &open->op_truncate, &open->op_created); if (!status && open->op_label.len) - nfsd4_security_inode_setsecctx(resfh, &open->op_label, open->op_bmval); + nfsd4_security_inode_setsecctx(*resfh, &open->op_label, open->op_bmval); /* * Following rfc 3530 14.2.16, use the returned bitmask @@ -282,29 +281,26 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru FATTR4_WORD1_TIME_MODIFY); } else { status = nfsd_lookup(rqstp, current_fh, - open->op_fname.data, open->op_fname.len, resfh); + open->op_fname.data, open->op_fname.len, *resfh); fh_unlock(current_fh); } if (status) goto out; - status = nfsd_check_obj_isreg(resfh); + status = nfsd_check_obj_isreg(*resfh); if (status) goto out; if (is_create_with_attrs(open) && open->op_acl != NULL) - do_set_nfs4_acl(rqstp, resfh, open->op_acl, open->op_bmval); + do_set_nfs4_acl(rqstp, *resfh, open->op_acl, open->op_bmval); - nfsd4_set_open_owner_reply_cache(cstate, open, resfh); + nfsd4_set_open_owner_reply_cache(cstate, open, *resfh); accmode = NFSD_MAY_NOP; if (open->op_created || open->op_claim_type == NFS4_OPEN_CLAIM_DELEGATE_CUR) accmode |= NFSD_MAY_OWNER_OVERRIDE; - status = do_open_permission(rqstp, resfh, open, accmode); + status = do_open_permission(rqstp, *resfh, open, accmode); set_change_info(&open->op_cinfo, current_fh); - fh_dup2(current_fh, resfh); out: - fh_put(resfh); - kfree(resfh); return status; } @@ -357,6 +353,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open) { __be32 status; + struct svc_fh *resfh = NULL; struct nfsd4_compoundres *resp; struct net *net = SVC_NET(rqstp); struct nfsd_net *nn = net_generic(net, nfsd_net_id); @@ -423,7 +420,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, switch (open->op_claim_type) { case NFS4_OPEN_CLAIM_DELEGATE_CUR: case NFS4_OPEN_CLAIM_NULL: - status = do_open_lookup(rqstp, cstate, open); + status = do_open_lookup(rqstp, cstate, open, &resfh); if (status) goto out; break; @@ -439,6 +436,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, status = do_open_fhandle(rqstp, cstate, open); if (status) goto out; + resfh = &cstate->current_fh; break; case NFS4_OPEN_CLAIM_DELEG_PREV_FH: case NFS4_OPEN_CLAIM_DELEGATE_PREV: @@ -458,9 +456,14 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, * successful, it (1) truncates the file if open->op_truncate was * set, (2) sets open->op_stateid, (3) sets open->op_delegation. */ - status = nfsd4_process_open2(rqstp, &cstate->current_fh, open); + status = nfsd4_process_open2(rqstp, resfh, open); WARN_ON(status && open->op_created); out: + if (resfh && resfh != &cstate->current_fh) { + fh_dup2(&cstate->current_fh, resfh); + fh_put(resfh); + kfree(resfh); + } nfsd4_cleanup_open_state(open, status); if (open->op_openowner && !nfsd4_has_session(cstate)) cstate->replay_owner = &open->op_openowner->oo_owner; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] nfsd4: fix delegation-unlink/rename race 2014-01-26 20:59 delegation patches for 3.14 J. Bruce Fields 2014-01-26 20:59 ` [PATCH 1/3] nfsd4: minor nfs4_setlease cleanup J. Bruce Fields 2014-01-26 20:59 ` [PATCH 2/3] nfsd4: delay setting current_fh in open J. Bruce Fields @ 2014-01-26 20:59 ` J. Bruce Fields 2014-01-27 19:24 ` J. Bruce Fields 2 siblings, 1 reply; 5+ messages in thread From: J. Bruce Fields @ 2014-01-26 20:59 UTC (permalink / raw) To: linux-nfs; +Cc: J. Bruce Fields From: "J. Bruce Fields" <bfields@redhat.com> If a file is unlinked or renamed between the time when we do the local open and the time when we get the delegation, then we will return to the client indicating that it holds a delegation even though the file no longer exists under the name it was open under. But a client performing an open-by-name, when it is returned a delegation, must be able to assume that the file is still linked at the name it was opened under. So, hold the parent i_mutex for longer to prevent concurrent renames or unlinks. Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4proc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 844813a..ef76ba6 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -279,11 +279,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru if (open->op_createmode == NFS4_CREATE_EXCLUSIVE && status == 0) open->op_bmval[1] = (FATTR4_WORD1_TIME_ACCESS | FATTR4_WORD1_TIME_MODIFY); - } else { + } else + /* + * Note this may exit with the parent still locked. + * We will hold the lock until nfsd4_open's final + * lookup, to prevent renames or unlinks until we've had + * a chance to an acquire a delegation if appropriate. + */ status = nfsd_lookup(rqstp, current_fh, open->op_fname.data, open->op_fname.len, *resfh); - fh_unlock(current_fh); - } if (status) goto out; status = nfsd_check_obj_isreg(*resfh); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] nfsd4: fix delegation-unlink/rename race 2014-01-26 20:59 ` [PATCH 3/3] nfsd4: fix delegation-unlink/rename race J. Bruce Fields @ 2014-01-27 19:24 ` J. Bruce Fields 0 siblings, 0 replies; 5+ messages in thread From: J. Bruce Fields @ 2014-01-27 19:24 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On Sun, Jan 26, 2014 at 03:59:21PM -0500, J. Bruce Fields wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > If a file is unlinked or renamed between the time when we do the local > open and the time when we get the delegation, then we will return to the > client indicating that it holds a delegation even though the file no > longer exists under the name it was open under. > > But a client performing an open-by-name, when it is returned a > delegation, must be able to assume that the file is still linked at the > name it was opened under. > > So, hold the parent i_mutex for longer to prevent concurrent renames or > unlinks. > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/nfsd/nfs4proc.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index 844813a..ef76ba6 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -279,11 +279,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru > if (open->op_createmode == NFS4_CREATE_EXCLUSIVE && status == 0) > open->op_bmval[1] = (FATTR4_WORD1_TIME_ACCESS | > FATTR4_WORD1_TIME_MODIFY); > - } else { > + } else > + /* > + * Note this may exit with the parent still locked. > + * We will hold the lock until nfsd4_open's final > + * lookup, to prevent renames or unlinks until we've had > + * a chance to an acquire a delegation if appropriate. > + */ > status = nfsd_lookup(rqstp, current_fh, > open->op_fname.data, open->op_fname.len, *resfh); > - fh_unlock(current_fh); > - } > if (status) > goto out; > status = nfsd_check_obj_isreg(*resfh); One last-minute fix: we can now end up taking two i_mutexes. The locking's still correct but we need the following annotation on the parent directory. (I haven't actually seen any lockdep warning trigger here.) --b. diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index e85b463..a41302a 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -207,7 +207,12 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp, goto out_nfserr; } } else { - fh_lock(fhp); + /* + * In the nfsd4_open() case, this may be held across + * subsequent open and delegation acquisition which may + * need to take the child's i_mutex: + */ + fh_lock_nested(fhp, I_MUTEX_PARENT); dentry = lookup_one_len(name, dparent, len); host_err = PTR_ERR(dentry); if (IS_ERR(dentry)) ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-27 19:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-26 20:59 delegation patches for 3.14 J. Bruce Fields 2014-01-26 20:59 ` [PATCH 1/3] nfsd4: minor nfs4_setlease cleanup J. Bruce Fields 2014-01-26 20:59 ` [PATCH 2/3] nfsd4: delay setting current_fh in open J. Bruce Fields 2014-01-26 20:59 ` [PATCH 3/3] nfsd4: fix delegation-unlink/rename race J. Bruce Fields 2014-01-27 19:24 ` J. Bruce Fields
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox