nfsd: bugfixes and preliminary patches for client_mutex removal
Subsystems (2)
cover nfsd: bugfixes and preliminary patches for client_mutex removal
Hi Bruce!
This patchset contains some bugfixes and preliminary patches for the client_mutex removal work. I'm sending these out first as they either fix bugs or don't add extra locking that's only nested inside the client_mutex anyway.
These are based on your nfsd-next tree. Patch #3 fixes a (possibly minor?) bug in the fi_delegations list handling, and might be suitable for stable based on that. I haven't cc'ed stable on it as I can't quite convince myself it's a real problem, but it might be reasonable to do so anyway.
Benny Halevy (3):
nfsd4: use recall_lock for delegation hashing
nfsd4: rename recall_lock to state_lock
nfsd4: hash deleg stateid only on successful nfs4_set_delegation
Jeff Layton (4):
nfsd: make nfsd4_encode_fattr static
nfsd: fix laundromat next-run-time calculation
nfsd: fix setting of NFS4_OO_CONFIRMED in nfsd4_open
nfsd: remove unneeded zeroing of fields in nfsd4_proc_compound
Trond Myklebust (2):
NFSd: Protect addition to the file_hashtbl
NFSd: protect delegation setup with the i_lock
fs/nfsd/nfs4callback.c | 18 +++++- fs/nfsd/nfs4proc.c | 6 +- fs/nfsd/nfs4state.c | 168 ++++++++++++++++++++++++++++++------------------- fs/nfsd/nfs4xdr.c | 2 +- fs/nfsd/state.h | 1 + 5 files changed, 124 insertions(+), 71 deletions(-)
Signature · 1 line
1.9.3
PATCH 1/9 nfsd: make nfsd4_encode_fattr static
sparse says:
CHECK fs/nfsd/nfs4xdr.c
fs/nfsd/nfs4xdr.c:2043:1: warning: symbol 'nfsd4_encode_fattr' was not declared. Should it be static?
--- fs/nfsd/nfs4xdr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/nfsd/nfs4xdr.c +1 -1
@@ -2039,7 +2039,7 @@ static int get_parent_attributes(struct svc_export *exp, struct kstat *stat)* Note: @fhp can be NULL; in this case, we might have to compose the filehandle * ourselves. */ -__be32 +static __be32 nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp, struct svc_export *exp, struct dentry *dentry, u32 *bmval,
Signature · 1 line
1.9.3
PATCH 2/9 nfsd: fix laundromat next-run-time calculation
In reply to Jeff LaytonThe laundromat uses two variables to calculate when it should next run, but one is completely ignored at the end of the run. Merge the two and rename the variable to be more descriptive of what it does.
--- fs/nfsd/nfs4state.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)
fs/nfsd/nfs4state.c +8 -14
@@ -3385,8 +3385,7 @@ nfs4_laundromat(struct nfsd_net *nn)struct nfs4_delegation *dp;struct list_head *pos, *next, reaplist;time_t cutoff =get_seconds () - nn->nfsd4_lease; -time_t t, clientid_val = nn->nfsd4_lease; -time_t u, test_val = nn->nfsd4_lease; +time_t t, new_timeo = nn->nfsd4_lease;nfs4_lock_state ();
@@ -3398,8 +3397,7 @@ nfs4_laundromat(struct nfsd_net *nn) clp = list_entry(pos, struct nfs4_client, cl_lru);if (time_after ((unsigned long )clp->cl_time, (unsigned long )cutoff)) { t = clp->cl_time - cutoff; -if (clientid_val > t) - clientid_val = t; + new_timeo =min (new_timeo, t);break ; }if (mark_client_expired_locked (clp)) {
@@ -3422,9 +3420,8 @@ nfs4_laundromat(struct nfsd_net *nn)if (net_generic (dp->dl_stid.sc_client->net, nfsd_net_id) != nn)continue ;if (time_after ((unsigned long )dp->dl_time, (unsigned long )cutoff)) { - u = dp->dl_time - cutoff; -if (test_val > u) - test_val = u; + t = dp->dl_time - cutoff; + new_timeo =min (new_timeo, t);break ; }list_move (&dp->dl_recall_lru, &reaplist);
@@ -3434,21 +3431,18 @@ nfs4_laundromat(struct nfsd_net *nn) dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);revoke_delegation (dp); } - test_val = nn->nfsd4_lease;list_for_each_safe (pos, next, &nn->close_lru) { oo = container_of(pos, struct nfs4_openowner, oo_close_lru);if (time_after ((unsigned long )oo->oo_time, (unsigned long )cutoff)) { - u = oo->oo_time - cutoff; -if (test_val > u) - test_val = u; + t = oo->oo_time - cutoff; + new_timeo =min (new_timeo, t);break ; }release_openowner (oo); } -if (clientid_val <NFSD_LAUNDROMAT_MINTIMEOUT ) - clientid_val =NFSD_LAUNDROMAT_MINTIMEOUT ; + new_timeo =max_t (time_t, new_timeo,NFSD_LAUNDROMAT_MINTIMEOUT );nfs4_unlock_state (); -return clientid_val; +return new_timeo; }static struct workqueue_struct *laundry_wq;
Signature · 1 line
1.9.3
PATCH 3/9 nfsd4: use recall_lock for delegation hashing
In reply to Jeff LaytonFrom: Benny Halevy <[email protected]>
This fixes a bug in the handling of the fi_delegations list.
nfs4_setlease does not hold the recall_lock when adding to it. The client_mutex is held, which prevents against concurrent list changes, but nfsd_break_deleg_cb does not hold while walking it. New delegations could theoretically creep onto the list while we're walking it there.
--- fs/nfsd/nfs4state.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
fs/nfsd/nfs4state.c +15 -5
@@ -433,12 +433,21 @@ static void unhash_stid(struct nfs4_stid *s) s->sc_type =0 ; } +static void +hash_delegation_locked (struct nfs4_delegation *dp,struct nfs4_file *fp) +{ +lockdep_assert_held (&recall_lock); + +list_add (&dp->dl_perfile, &fp->fi_delegations); +list_add (&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations); +} +/* Called under the state lock. */ static void unhash_delegation (struct nfs4_delegation *dp) { -list_del_init (&dp->dl_perclnt);spin_lock (&recall_lock); +list_del_init (&dp->dl_perclnt);list_del_init (&dp->dl_perfile);list_del_init (&dp->dl_recall_lru);spin_unlock (&recall_lock);
@@ -3053,11 +3062,12 @@ static int nfs4_setlease(struct nfs4_delegation *dp) status =vfs_setlease (fl->fl_file, fl->fl_type, &fl);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); +spin_lock (&recall_lock); +hash_delegation_locked (dp, fp); +spin_unlock (&recall_lock);return 0 ; out_free:locks_free_lock (fl);
@@ -3078,9 +3088,8 @@ static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)spin_unlock (&recall_lock);return -EAGAIN ; } -list_add (&dp->dl_perfile, &fp->fi_delegations); +hash_delegation_locked (dp, fp);spin_unlock (&recall_lock); -list_add (&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);return 0 ; }
@@ -4891,6 +4900,7 @@ static u64 nfsd_find_all_delegations(struct nfs4_client *clp, u64 max,struct nfs4_delegation *dp, *next;u64 count =0 ; +lockdep_assert_held (&recall_lock);list_for_each_entry_safe (dp, next, &clp->cl_delegations, dl_perclnt) {if (victims)list_move (&dp->dl_recall_lru, victims);
Signature · 1 line
1.9.3
PATCH 4/9 nfsd: fix setting of NFS4_OO_CONFIRMED in nfsd4_open
In reply to Jeff LaytonIn the NFS4_OPEN_CLAIM_PREVIOUS case, we should only mark it confirmed if the nfs4_check_open_reclaim check succeeds.
In the NFS4_OPEN_CLAIM_DELEG_PREV_FH and NFS4_OPEN_CLAIM_DELEGATE_PREV cases, I see no point in declaring the openowner confirmed when the operation is going to fail anyway, and doing so might allow the client to game things such that it wouldn't need to confirm a subsequent open with the same owner.
--- fs/nfsd/nfs4proc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
fs/nfsd/nfs4proc.c +1 -2
@@ -430,12 +430,12 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, goto out;break ;case NFS4_OPEN_CLAIM_PREVIOUS : - open->op_openowner->oo_flags |=NFS4_OO_CONFIRMED ; status =nfs4_check_open_reclaim (&open->op_clientid, cstate->minorversion, nn);if (status) goto out; + open->op_openowner->oo_flags |=NFS4_OO_CONFIRMED ;case NFS4_OPEN_CLAIM_FH :case NFS4_OPEN_CLAIM_DELEG_CUR_FH : status =do_open_fhandle (rqstp, cstate, open);
@@ -445,7 +445,6 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,break ;case NFS4_OPEN_CLAIM_DELEG_PREV_FH :case NFS4_OPEN_CLAIM_DELEGATE_PREV : - open->op_openowner->oo_flags |=NFS4_OO_CONFIRMED ;dprintk ("NFSD: unsupported OPEN claim type %d\n" , open->op_claim_type); status = nfserr_notsupp;
Signature · 1 line
1.9.3
PATCH 5/9 nfsd: remove unneeded zeroing of fields in nfsd4_proc_compound
In reply to Jeff LaytonThe memset of resp in svc_process_common should ensure that these are already zeroed by the time they get here.
--- fs/nfsd/nfs4proc.c | 3 --- 1 file changed, 3 deletions(-)
fs/nfsd/nfs4proc.c +0 -3
@@ -1292,11 +1292,8 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,xdr_reserve_space (&resp->xdr,8 + args->taglen); resp->taglen = args->taglen; resp->tag = args->tag; - resp->opcnt =0 ; resp->rqstp = rqstp; cstate->minorversion = args->minorversion; - cstate->replay_owner =NULL ; - cstate->session =NULL ;fh_init (current_fh,NFS4_FHSIZE );fh_init (save_fh,NFS4_FHSIZE );/*
Signature · 1 line
1.9.3
PATCH 6/9 nfsd4: rename recall_lock to state_lock
In reply to Jeff LaytonFrom: Benny Halevy <[email protected]>
...as the name is a bit more descriptive and we've started using it for other purposes.
--- fs/nfsd/nfs4state.c | 63 +++++++++++++++++++++++++++-------------------------- 1 file changed, 32 insertions(+), 31 deletions(-)
fs/nfsd/nfs4state.c +32 -31
@@ -81,7 +81,7 @@ static DEFINE_MUTEX(client_mutex);* effort to decrease the scope of the client_mutex, this spinlock may * eventually cover more: */ -static DEFINE_SPINLOCK (recall_lock); +static DEFINE_SPINLOCK (state_lock);static struct kmem_cache *openowner_slab;static struct kmem_cache *lockowner_slab;
@@ -235,9 +235,9 @@ static void nfsd4_free_file(struct nfs4_file *f)static inline void put_nfs4_file (struct nfs4_file *fi) { -if (atomic_dec_and_lock (&fi->fi_ref, &recall_lock)) { +if (atomic_dec_and_lock (&fi->fi_ref, &state_lock)) {hlist_del (&fi->fi_hash); -spin_unlock (&recall_lock); +spin_unlock (&state_lock);iput (fi->fi_inode);nfsd4_free_file (fi); }
@@ -436,7 +436,7 @@ static void unhash_stid(struct nfs4_stid *s)static void hash_delegation_locked (struct nfs4_delegation *dp,struct nfs4_file *fp) { -lockdep_assert_held (&recall_lock); +lockdep_assert_held (&state_lock);list_add (&dp->dl_perfile, &fp->fi_delegations);list_add (&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
@@ -446,11 +446,11 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)static void unhash_delegation (struct nfs4_delegation *dp) { -spin_lock (&recall_lock); +spin_lock (&state_lock);list_del_init (&dp->dl_perclnt);list_del_init (&dp->dl_perfile);list_del_init (&dp->dl_recall_lru); -spin_unlock (&recall_lock); +spin_unlock (&state_lock);if (dp->dl_file) {nfs4_put_deleg_lease (dp->dl_file);put_nfs4_file (dp->dl_file);
@@ -1161,13 +1161,13 @@ destroy_client(struct nfs4_client *clp)struct nfsd_net *nn =net_generic (clp->net, nfsd_net_id);INIT_LIST_HEAD (&reaplist); -spin_lock (&recall_lock); +spin_lock (&state_lock);while (!list_empty (&clp->cl_delegations)) { dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);list_del_init (&dp->dl_perclnt);list_move (&dp->dl_recall_lru, &reaplist); } -spin_unlock (&recall_lock); +spin_unlock (&state_lock);while (!list_empty (&reaplist)) { dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);destroy_delegation (dp);
@@ -2527,9 +2527,9 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) fp->fi_lease =NULL ;memset (fp->fi_fds,0 ,sizeof (fp->fi_fds));memset (fp->fi_access,0 ,sizeof (fp->fi_access)); -spin_lock (&recall_lock); +spin_lock (&state_lock);hlist_add_head (&fp->fi_hash, &file_hashtbl[hashval]); -spin_unlock (&recall_lock); +spin_unlock (&state_lock); }void
@@ -2700,15 +2700,15 @@ find_file(struct inode *ino)unsigned int hashval =file_hashval (ino);struct nfs4_file *fp; -spin_lock (&recall_lock); +spin_lock (&state_lock);hlist_for_each_entry (fp, &file_hashtbl[hashval], fi_hash) {if (fp->fi_inode == ino) {get_nfs4_file (fp); -spin_unlock (&recall_lock); +spin_unlock (&state_lock);return fp; } } -spin_unlock (&recall_lock); +spin_unlock (&state_lock);return NULL ; }
@@ -2745,6 +2745,7 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)struct nfs4_client *clp = dp->dl_stid.sc_client;struct nfsd_net *nn =net_generic (clp->net, nfsd_net_id); +lockdep_assert_held (&state_lock);/* We're assuming the state code never drops its reference * without first removing the lease. Since we're in this lease * callback (and since the lease code is serialized by the kernel
@@ -2781,11 +2782,11 @@ static void nfsd_break_deleg_cb(struct file_lock *fl)*/ fl->fl_break_time =0 ; -spin_lock (&recall_lock); +spin_lock (&state_lock); fp->fi_had_conflict = true;list_for_each_entry (dp, &fp->fi_delegations, dl_perfile)nfsd_break_one_deleg (dp); -spin_unlock (&recall_lock); +spin_unlock (&state_lock); } static
@@ -3065,9 +3066,9 @@ static int nfs4_setlease(struct nfs4_delegation *dp) fp->fi_lease = fl; fp->fi_deleg_file =get_file (fl->fl_file);atomic_set (&fp->fi_delegees,1 ); -spin_lock (&recall_lock); +spin_lock (&state_lock);hash_delegation_locked (dp, fp); -spin_unlock (&recall_lock); +spin_unlock (&state_lock);return 0 ; out_free:locks_free_lock (fl);
@@ -3082,14 +3083,14 @@ static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp) dp->dl_file = fp;if (!fp->fi_lease)return nfs4_setlease (dp); -spin_lock (&recall_lock); +spin_lock (&state_lock);atomic_inc (&fp->fi_delegees);if (fp->fi_had_conflict) { -spin_unlock (&recall_lock); +spin_unlock (&state_lock);return -EAGAIN ; }hash_delegation_locked (dp, fp); -spin_unlock (&recall_lock); +spin_unlock (&state_lock);return 0 ; }
@@ -3423,7 +3424,7 @@ nfs4_laundromat(struct nfsd_net *nn) clp->cl_clientid.cl_id);expire_client (clp); } -spin_lock (&recall_lock); +spin_lock (&state_lock);list_for_each_safe (pos, next, &nn->del_recall_lru) { dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);if (net_generic (dp->dl_stid.sc_client->net, nfsd_net_id) != nn)
@@ -3435,7 +3436,7 @@ nfs4_laundromat(struct nfsd_net *nn) }list_move (&dp->dl_recall_lru, &reaplist); } -spin_unlock (&recall_lock); +spin_unlock (&state_lock);list_for_each_safe (pos, next, &reaplist) { dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);revoke_delegation (dp);
@@ -4900,7 +4901,7 @@ static u64 nfsd_find_all_delegations(struct nfs4_client *clp, u64 max,struct nfs4_delegation *dp, *next;u64 count =0 ; -lockdep_assert_held (&recall_lock); +lockdep_assert_held (&state_lock);list_for_each_entry_safe (dp, next, &clp->cl_delegations, dl_perclnt) {if (victims)list_move (&dp->dl_recall_lru, victims);
@@ -4916,9 +4917,9 @@ u64 nfsd_forget_client_delegations(struct nfs4_client *clp, u64 max)LIST_HEAD (victims);u64 count; -spin_lock (&recall_lock); +spin_lock (&state_lock); count =nfsd_find_all_delegations (clp, max, &victims); -spin_unlock (&recall_lock); +spin_unlock (&state_lock);list_for_each_entry_safe (dp, next, &victims, dl_recall_lru)revoke_delegation (dp);
@@ -4932,11 +4933,11 @@ u64 nfsd_recall_client_delegations(struct nfs4_client *clp, u64 max)LIST_HEAD (victims);u64 count; -spin_lock (&recall_lock); +spin_lock (&state_lock); count =nfsd_find_all_delegations (clp, max, &victims);list_for_each_entry_safe (dp, next, &victims, dl_recall_lru)nfsd_break_one_deleg (dp); -spin_unlock (&recall_lock); +spin_unlock (&state_lock);return count; }
@@ -4945,9 +4946,9 @@ u64 nfsd_print_client_delegations(struct nfs4_client *clp, u64 max) {u64 count =0 ; -spin_lock (&recall_lock); +spin_lock (&state_lock); count =nfsd_find_all_delegations (clp, max,NULL ); -spin_unlock (&recall_lock); +spin_unlock (&state_lock);nfsd_print_count (clp, count,"delegations" );return count;
@@ -5158,12 +5159,12 @@ nfs4_state_shutdown_net(struct net *net)nfs4_lock_state ();INIT_LIST_HEAD (&reaplist); -spin_lock (&recall_lock); +spin_lock (&state_lock);list_for_each_safe (pos, next, &nn->del_recall_lru) { dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);list_move (&dp->dl_recall_lru, &reaplist); } -spin_unlock (&recall_lock); +spin_unlock (&state_lock);list_for_each_safe (pos, next, &reaplist) { dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);destroy_delegation (dp);
Signature · 1 line
1.9.3
PATCH 7/9 nfsd4: hash deleg stateid only on successful nfs4_set_delegation
In reply to Jeff LaytonFrom: Benny Halevy <[email protected]>
We don't want the stateid to be found in the hash table before the delegation is granted.
Currently this is protected by the client_mutex, but we want to break that up and this is a necessary step toward that goal.
--- fs/nfsd/nfs4state.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/nfsd/nfs4state.c +1 -1
@@ -375,7 +375,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv dp =delegstateid (nfs4_alloc_stid (clp, deleg_slab));if (dp ==NULL )return dp; - dp->dl_stid.sc_type =NFS4_DELEG_STID ;/* * delegation seqid's are never incremented. The 4.1 special * meaning of seqid 0 isn't meaningful, really, but let's avoid
@@ -438,6 +437,7 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp) {lockdep_assert_held (&state_lock); + dp->dl_stid.sc_type =NFS4_DELEG_STID ;list_add (&dp->dl_perfile, &fp->fi_delegations);list_add (&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations); }
Signature · 1 line
1.9.3
PATCH 8/9 NFSd: Protect addition to the file_hashtbl
In reply to Jeff LaytonFrom: Trond Myklebust <[email protected]>
Ensure that we only can have a single struct nfs4_file per inode in the file_hashtbl and make addition atomic with respect to lookup.
--- fs/nfsd/nfs4state.c | 46 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 11 deletions(-)
fs/nfsd/nfs4state.c +35 -11
@@ -2519,6 +2519,8 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) {unsigned int hashval =file_hashval (ino); +lockdep_assert_held (&state_lock); +atomic_set (&fp->fi_ref,1 );INIT_LIST_HEAD (&fp->fi_stateids);INIT_LIST_HEAD (&fp->fi_delegations);
@@ -2527,9 +2529,7 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) fp->fi_lease =NULL ;memset (fp->fi_fds,0 ,sizeof (fp->fi_fds));memset (fp->fi_access,0 ,sizeof (fp->fi_access)); -spin_lock (&state_lock);hlist_add_head (&fp->fi_hash, &file_hashtbl[hashval]); -spin_unlock (&state_lock); }void
@@ -2695,23 +2695,49 @@ find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,/* search file_hashtbl[] for file */ static struct nfs4_file * -find_file (struct inode *ino) +find_file_locked (struct inode *ino) {unsigned int hashval =file_hashval (ino);struct nfs4_file *fp; -spin_lock (&state_lock); +lockdep_assert_held (&state_lock); +hlist_for_each_entry (fp, &file_hashtbl[hashval], fi_hash) {if (fp->fi_inode == ino) {get_nfs4_file (fp); -spin_unlock (&state_lock);return fp; } } -spin_unlock (&state_lock);return NULL ; } +static struct nfs4_file * +find_file (struct inode *ino) +{ +struct nfs4_file *fp; + +spin_lock (&state_lock); + fp =find_file_locked (ino); +spin_unlock (&state_lock); +return fp; +} + +static struct nfs4_file * +find_or_add_file (struct inode *ino,struct nfs4_file *new) +{ +struct nfs4_file *fp; + +spin_lock (&state_lock); + fp =find_file_locked (ino); +if (fp ==NULL ) { +nfsd4_init_file (new, ino); + fp = new; + } +spin_unlock (&state_lock); + +return fp; +} +/* * Called to check deny when READ with all zero stateid or * WRITE with all zero or all one stateid
@@ -3230,21 +3256,19 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf* and check for delegations in the process of being recalled. * If not found, create the nfs4_file struct */ - fp =find_file (ino); -if (fp) { + fp =find_or_add_file (ino, open->op_file); +if (fp != open->op_file) {if ((status =nfs4_check_open (fp, open, &stp))) goto out; status =nfs4_check_deleg (cl, open, &dp);if (status) goto out; }else { + open->op_file =NULL ; status = nfserr_bad_stateid;if (nfsd4_is_deleg_cur (open)) goto out; status = nfserr_jukebox; - fp = open->op_file; - open->op_file =NULL ; -nfsd4_init_file (fp, ino); }/*
Signature · 1 line
1.9.3
Re: [PATCH 8/9] NFSd: Protect addition to the file_hashtbl
Quoted 15 lines · Jeff Layton
From: Trond Myklebust <[email protected]>
Ensure that we only can have a single struct
nfs4_fileper inode in thefile_hashtbland make addition atomic with respect to lookup.Signed-off-by Trond Myklebust <[email protected]>--- fs/nfsd/nfs4state.c | 46 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 11 deletions(-)fs/nfsd/nfs4state.c +0
-0
fs/nfsd/nfs4state.c static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) · line 2523
Quoted 5 lines
fs/nfsd/nfs4state.c +2
-0@@ -2519,6 +2519,8 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) {unsigned int hashval =file_hashval (ino); +lockdep_assert_held (&state_lock); +
Oops, lockdep points out we overlooked a deadlock here: this function also calls igrab(), which takes the i_lock, the reverse ordering from what we take in the delegation-break case.
Dropping this patch for now.
--b.
Quoted 95 lines
fs/nfsd/nfs4state.c +33
-11atomic_set (&fp->fi_ref,1 );INIT_LIST_HEAD (&fp->fi_stateids);INIT_LIST_HEAD (&fp->fi_delegations);@@ -2527,9 +2529,7 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) fp->fi_lease =NULL ;memset (fp->fi_fds,0 ,sizeof (fp->fi_fds));memset (fp->fi_access,0 ,sizeof (fp->fi_access)); -spin_lock (&state_lock);hlist_add_head (&fp->fi_hash, &file_hashtbl[hashval]); -spin_unlock (&state_lock); }void @@ -2695,23 +2695,49 @@ find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,/* search file_hashtbl[] for file */ static struct nfs4_file * -find_file (struct inode *ino) +find_file_locked (struct inode *ino) {unsigned int hashval =file_hashval (ino);struct nfs4_file *fp; -spin_lock (&state_lock); +lockdep_assert_held (&state_lock); +hlist_for_each_entry (fp, &file_hashtbl[hashval], fi_hash) {if (fp->fi_inode == ino) {get_nfs4_file (fp); -spin_unlock (&state_lock);return fp; } } -spin_unlock (&state_lock);return NULL ; } +static struct nfs4_file * +find_file (struct inode *ino) +{ +struct nfs4_file *fp; + +spin_lock (&state_lock); + fp =find_file_locked (ino); +spin_unlock (&state_lock); +return fp; +} + +static struct nfs4_file * +find_or_add_file (struct inode *ino,struct nfs4_file *new) +{ +struct nfs4_file *fp; + +spin_lock (&state_lock); + fp =find_file_locked (ino); +if (fp ==NULL ) { +nfsd4_init_file (new, ino); + fp = new; + } +spin_unlock (&state_lock); + +return fp; +} +/* * Called to check deny when READ with all zero stateid or * WRITE with all zero or all one stateid @@ -3230,21 +3256,19 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf* and check for delegations in the process of being recalled. * If not found, create the nfs4_file struct */ - fp =find_file (ino); -if (fp) { + fp =find_or_add_file (ino, open->op_file); +if (fp != open->op_file) {if ((status =nfs4_check_open (fp, open, &stp))) goto out; status =nfs4_check_deleg (cl, open, &dp);if (status) goto out; }else { + open->op_file =NULL ; status = nfserr_bad_stateid;if (nfsd4_is_deleg_cur (open)) goto out; status = nfserr_jukebox; - fp = open->op_file; - open->op_file =NULL ; -nfsd4_init_file (fp, ino); }/* Signature · 1 line
1.9.3
Re: [PATCH 8/9] NFSd: Protect addition to the file_hashtbl
Quoted 19 lines · J. Bruce Fields
Quoted 18 lines · Jeff Layton
From: Trond Myklebust <[email protected]>
Ensure that we only can have a single struct
nfs4_fileper inode in thefile_hashtbland make addition atomic with respect to lookup.Signed-off-by Trond Myklebust <[email protected]>--- fs/nfsd/nfs4state.c | 46 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 11 deletions(-)fs/nfsd/nfs4state.c +0
-0@@ -2519,6 +2519,8 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) {unsigned int hashval =file_hashval (ino);
Quoted 9 lines
fs/nfsd/nfs4state.c static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) · line 2523
Quoted 2 lines
fs/nfsd/nfs4state.c +2
-0+lockdep_assert_held (&state_lock); +Oops, lockdep points out we overlooked a deadlock here: this function also calls
igrab(), which takes thei_lock, the reverse ordering from what we take in the delegation-break case.Dropping this patch for now.
This was the reason for the delegation recall locking changes which are also part of the series.
That said, why do we need igrab here as opposed to just ihold()?
Signature · 5 lines
Trond Myklebust Linux NFS client maintainer, PrimaryData [email protected]
Re: [PATCH 8/9] NFSd: Protect addition to the file_hashtbl
Quoted 27 lines · On Thu, 5 Jun 2014 12:18:09 -0400 Trond Myklebust…
Quoted 26 lines · J. Bruce Fields
Quoted 15 lines · Jeff Layton
From: Trond Myklebust <[email protected]>
Ensure that we only can have a single struct
nfs4_fileper inode in thefile_hashtbland make addition atomic with respect to lookup.Signed-off-by Trond Myklebust <[email protected]>--- fs/nfsd/nfs4state.c | 46 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 11 deletions(-)fs/nfsd/nfs4state.c +0
-0fs/nfsd/nfs4state.c static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) · line 2523
Quoted 5 lines
fs/nfsd/nfs4state.c +2
-0@@ -2519,6 +2519,8 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino) {unsigned int hashval =file_hashval (ino); +lockdep_assert_held (&state_lock); +Oops, lockdep points out we overlooked a deadlock here: this function also calls
igrab(), which takes thei_lock, the reverse ordering from what we take in the delegation-break case.
Quoted 8 lines
Quoted 2 lines
Dropping this patch for now.
This was the reason for the delegation recall locking changes which are also part of the series.
That said, why do we need igrab here as opposed to just
ihold()?
Yeah, that would be a lot simpler. We certainly already have a reference by virtue of the dentry in the filehandle so ihold is reasonable. I'll change it to use that.
Signature · 1 line
Jeff Layton <[email protected]>
PATCH 9/9 NFSd: protect delegation setup with the i_lock
In reply to Jeff LaytonFrom: Trond Myklebust <[email protected]>
state_lock is a heavily contended global lock. We don't want to grab that while simultaneously holding the inode->i_lock. Avoid doing that in the delegation break callback by ensuring that we add/remove the dl_perfile under the inode->i_lock.
This however, can cause an ABBA deadlock between the state_lock and the i_lock. Work around that by doing the list manipulation in the delegation recall from the workqueue context prior to starting the rpc call.
--- fs/nfsd/nfs4callback.c | 18 ++++++++++++++++-- fs/nfsd/nfs4state.c | 41 +++++++++++++++++++++++++++-------------- fs/nfsd/state.h | 1 + 3 files changed, 44 insertions(+), 16 deletions(-)
fs/nfsd/nfs4callback.c +16 -2
@@ -1011,9 +1011,8 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)run_nfsd4_cb (cb); } -static void nfsd4_do_callback_rpc (struct work_struct *w) +static void nfsd4_run_callback_rpc (struct nfsd4_callback *cb) { - struct nfsd4_callback *cb = container_of(w, struct nfsd4_callback, cb_work);struct nfs4_client *clp = cb->cb_clp;struct rpc_clnt *clnt;
@@ -1031,11 +1030,25 @@ static void nfsd4_do_callback_rpc(struct work_struct *w) cb->cb_ops, cb); } +static void nfsd4_do_callback_rpc (struct work_struct *w) +{ + struct nfsd4_callback *cb = container_of(w, struct nfsd4_callback, cb_work); +nfsd4_run_callback_rpc (cb); +} +void nfsd4_init_callback (struct nfsd4_callback *cb) {INIT_WORK (&cb->cb_work, nfsd4_do_callback_rpc); } +static void nfsd4_do_cb_recall (struct work_struct *w) +{ + struct nfsd4_callback *cb = container_of(w, struct nfsd4_callback, cb_work); + +nfsd4_prepare_cb_recall (cb->cb_op); +nfsd4_run_callback_rpc (cb); +} +void nfsd4_cb_recall (struct nfs4_delegation *dp) {struct nfsd4_callback *cb = &dp->dl_recall;
@@ -1052,6 +1065,7 @@ void nfsd4_cb_recall(struct nfs4_delegation *dp)INIT_LIST_HEAD (&cb->cb_per_client); cb->cb_done = true; +INIT_WORK (&cb->cb_work, nfsd4_do_cb_recall);run_nfsd4_cb (&dp->dl_recall); }
fs/nfsd/nfs4state.c +27 -14
83 lines not on this page · raw message
fs/nfsd/state.h +1 -0
@@ -472,6 +472,7 @@ extern void nfsd4_cb_recall(struct nfs4_delegation *dp);extern int nfsd4_create_callback_queue (void );extern void nfsd4_destroy_callback_queue (void );extern void nfsd4_shutdown_callback (struct nfs4_client *); +extern void nfsd4_prepare_cb_recall (struct nfs4_delegation *dp);extern void nfs4_put_delegation (struct nfs4_delegation *dp);extern struct nfs4_client_reclaim *nfs4_client_to_reclaim (const char *name,struct nfsd_net *nn);
Signature · 1 line
1.9.3
Re: [PATCH 9/9] NFSd: protect delegation setup with the i_lock
Quoted 6 lines · Jeff Layton
From: Trond Myklebust <[email protected]>
state_lockis a heavily contended global lock. We don't want to grab that while simultaneously holding theinode->i_lock. Avoid doing that in the delegation break callback by ensuring that we add/remove thedl_perfileunder theinode->i_lock.
The current code never takes the state lock (or recall lock) under i_lock. Non-trivial use of i_lock in nfsd is only added in this patch.
Quoted 4 lines
This however, can cause an ABBA deadlock between the
state_lockand thei_lock. Work around that by doing the list manipulation in the delegation recall from the workqueue context prior to starting the rpc call.
I very much dislike the patch for multiple reasons. For one thing nfsd currently stays away from i_lock which is a VFS (and sometimes abused by the fs) lock except for a single case in the file locking code which really should be factored into common code. I'd rather have locks in nfsd than starting to use i_lock in nfsd and making auditing it's use and it's lock hierchies more complex by introducing usage outside of the VFS.
Second I really don't like pushing the delegation setup into the callback workqueue. I have a patchset refactoring the callback code to allow adding more callbacks without lots of copy & paste, and except for this new addition the code in the workqueue already is almost perfectly generic for arbitrary callbacks.
Please add a lock in struct nfs4_file to protects it's own members instead.
Re: [PATCH 9/9] NFSd: protect delegation setup with the i_lock
Quoted 16 lines · On Mon, 2 Jun 2014 01:46:16 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 6 lines · Jeff Layton
From: Trond Myklebust <[email protected]>
state_lockis a heavily contended global lock. We don't want to grab that while simultaneously holding theinode->i_lock. Avoid doing that in the delegation break callback by ensuring that we add/remove thedl_perfileunder theinode->i_lock.The current code never takes the state lock (or recall lock) under
i_lock. Non-trivial use ofi_lockin nfsd is only added in this patch.Quoted 4 lines
This however, can cause an ABBA deadlock between the
state_lockand thei_lock. Work around that by doing the list manipulation in the delegation recall from the workqueue context prior to starting the rpc call.
Quoted 8 lines
I very much dislike the patch for multiple reasons. For one thing nfsd currently stays away from
i_lockwhich is a VFS (and sometimes abused by the fs) lock except for a single case in the file locking code which really should be factored into common code. I'd rather have locks in nfsd than starting to usei_lockin nfsd and making auditing it's use and it's lock hierchies more complex by introducing usage outside of the VFS.
Ok, fair enough. A later patch in the series adds a per nfs4_file lock (the fi_lock) that we can use for this purpose in lieu of the i_lock. Nesting that within the i_lock shouldn't be too painful.
Quoted 9 lines
Second I really don't like pushing the delegation setup into the callback workqueue. I have a patchset refactoring the callback code to allow adding more callbacks without lots of copy & paste, and except for this new addition the code in the workqueue already is almost perfectly generic for arbitrary callbacks.
Please add a lock in struct
nfs4_fileto protects it's own members instead.
This is a problem however. The del_recall_lru list is a per-namespace list, so we need a lock that is either global or per-namespace to add things onto it. The point of this patch is to get that extra locking out of the codepath where the i_lock is already held.
We could do that within the rpc_call_prepare operation, but the main problem there is that the delegation could leak if the rpc task allocation fails. Would you be amenable to adding a "cb_prepare" operation or something that we could use to run things from the workqueue before the actual callback runs?
Signature · 1 line
Jeff Layton <[email protected]>
Re: [PATCH 9/9] NFSd: protect delegation setup with the i_lock
Quoted 3 lines · Jeff Layton
Ok, fair enough. A later patch in the series adds a per
nfs4_filelock (thefi_lock) that we can use for this purpose in lieu of thei_lock. Nesting that within thei_lockshouldn't be too painful.
i_lock is part of some very complex locking hierachies in the inode and dentry caches. Long term I'd prefer to get the file locking code detangled from that, but let's keep it simple for now and nest the nfs4_file lock inside for now.
Quoted 5 lines
We could do that within the
rpc_call_prepareoperation, but the main problem there is that the delegation could leak if the rpc task allocation fails. Would you be amenable to adding a "cb_prepare" operation or something that we could use to run things from the workqueue before the actual callback runs?
I guess we'll have to do it then. I'll see if it can be done nicely.
I suspect the root problem of all this is that the file locking code calls the lock manager ops with i_lock held, which isn't very nice.
Re: [PATCH 9/9] NFSd: protect delegation setup with the i_lock
Quoted 10 lines · On Mon, 2 Jun 2014 11:20:26 -0700 Christoph Hellwig <[email protected]> wrote:
Quoted 3 lines · Jeff Layton
Ok, fair enough. A later patch in the series adds a per
nfs4_filelock (thefi_lock) that we can use for this purpose in lieu of thei_lock. Nesting that within thei_lockshouldn't be too painful.
i_lockis part of some very complex locking hierachies in the inode and dentry caches. Long term I'd prefer to get the file locking code detangled from that, but let's keep it simple for now and nest thenfs4_filelock inside for now.
Quoted 8 lines
Quoted 5 lines
We could do that within the
rpc_call_prepareoperation, but the main problem there is that the delegation could leak if the rpc task allocation fails. Would you be amenable to adding a "cb_prepare" operation or something that we could use to run things from the workqueue before the actual callback runs?I guess we'll have to do it then. I'll see if it can be done nicely.
There is another option, but it's sort of ugly...
We could try to queue the thing in the rpc_call_prepare op, but if that fails to occur then we could do it in the rpc_release callback. The tricky part is figuring out whether we'll need to queue it in rpc_release.
Perhaps we can try to use the dl_time field to indicate that as well so we won't need to add a separate flag for it.
Quoted 3 lines
I suspect the root problem of all this is that the file locking code calls the lock manager ops with
i_lockheld, which isn't very nice.
Yeah, it would be nice if it didn't, but it's better than it was. It used to use a global spinlock for that instead of the i_lock, but that was also a clear scalability problem...
Signature · 1 line
Jeff Layton <[email protected]>
[PATCH 10/9] nfsd: remove initial assignment of "p" in nfsd4_encode_security_label
In reply to Jeff Layton"pp" is no longer passed to this function, and "p" is clobbered just afterward anyway.
--- fs/nfsd/nfs4xdr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/nfsd/nfs4xdr.c +1 -1
@@ -1981,7 +1981,7 @@ static inline __be32 nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp, void *context, int len) { - __be32 *p = *pp; + __be32 *p; p =xdr_reserve_space (xdr, len +4 +4 +4 );if (!p)
Signature · 1 line
1.9.3
Quoted 7 lines · Jeff Layton
Hi Bruce!
This patchset contains some bugfixes and preliminary patches for the
client_mutexremoval work. I'm sending these out first as they either fix bugs or don't add extra locking that's only nested inside theclient_mutexanyway.
Quoted 5 lines
These are based on your nfsd-next tree. Patch #3 fixes a (possibly minor?) bug in the
fi_delegationslist handling, and might be suitable for stable based on that. I haven't cc'ed stable on it as I can't quite convince myself it's a real problem, but it might be reasonable to do so anyway.
It looks innocent enough, so I'm adding the stable cc:.
I'm dropping patch 9 ("NFSd: protect delegation setup with the i_lock") and applying the others.
--b.
Quoted 26 lines
Benny Halevy (3):
nfsd4: userecall_lockfor delegation hashing
nfsd4: renamerecall_locktostate_lock
nfsd4: hash deleg stateid only on successfulnfs4_set_delegationJeff Layton (4):
nfsd: makenfsd4_encode_fattrstatic
nfsd: fix laundromat next-run-time calculation
nfsd: fix setting ofNFS4_OO_CONFIRMEDinnfsd4_open
nfsd: remove unneeded zeroing of fields innfsd4_proc_compoundTrond Myklebust (2):
NFSd: Protect addition to thefile_hashtbl
NFSd: protect delegation setup with thei_lockfs/nfsd/nfs4callback.c | 18 +++++- fs/nfsd/nfs4proc.c | 6 +- fs/nfsd/nfs4state.c | 168 ++++++++++++++++++++++++++++++------------------- fs/nfsd/nfs4xdr.c | 2 +- fs/nfsd/state.h | 1 + 5 files changed, 124 insertions(+), 71 deletions(-)Signature · 1 line
1.9.3