Fix a 2.6.38 delegation regression
Subsystems (2)
When I removed the mylease callback for the 2.6.38 merge window, I screwed up: I forgot that nfsd could hold multiple leases on the same file on behalf of multiple clients, and that all those leases would be associated with the same file descriptor. This could cause a problem when removing a lease, as the wrong lease could get removed.
There's no real reason why we should hold multiple leases; we can instead just keep one lease, and use a count of the number of delegations to decide when to remove it.
The following patches do that (after a little preperatory cleanup).
We could instead revert the mylease removal. But I think this is still a small enough change to justify after the merge window. So barring objections I intend to to submit this for 2.6.38 soon (along with a couple other bugfixes I have queued up.)
--b.
PATCH 1/6 nfsd4: split up nfsd_break_deleg_cb
We'll be adding some more code here soon.
--- fs/nfsd/nfs4state.c | 35 +++++++++++++++++------------------ 1 files changed, 17 insertions(+), 18 deletions(-)
fs/nfsd/nfs4state.c +17 -18
@@ -2329,23 +2329,8 @@ nfs4_file_downgrade(struct nfs4_file *fp, unsigned int share_access)nfs4_file_put_access (fp,O_RDONLY ); } -/* -* Spawn a thread to perform a recall on the delegation represented -* by the lease (file_lock) -* -* Called from break_lease() with lock_flocks() held. -* Note: we assume break_lease will only call this *once* for any given -* lease. -*/ -static -void nfsd_break_deleg_cb (struct file_lock *fl) +static void nfsd_break_one_deleg (struct nfs4_delegation *dp) { -struct nfs4_delegation *dp = (struct nfs4_delegation *)fl->fl_owner; - -dprintk ("NFSD nfsd_break_deleg_cb: dp %p fl %p\n" ,dp,fl); -if (!dp) -return ; -/* 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
@@ -2360,15 +2345,29 @@ void nfsd_break_deleg_cb(struct file_lock *fl)/* only place dl_time is set. protected by lock_flocks*/ dp->dl_time =get_seconds (); +nfsd4_cb_recall (dp); +} + +/* +* Called from break_lease() with lock_flocks() held. +* Note: we assume break_lease will only call this *once* for any given +* lease. +*/ +static +void nfsd_break_deleg_cb (struct file_lock *fl) +{ +struct nfs4_delegation *dp = (struct nfs4_delegation *)fl->fl_owner; + +BUG_ON (!dp);/* * We don't want the locks code to timeout the lease for us; * we'll remove it ourself if the delegation isn't returned -* in time. +* in time: */ fl->fl_break_time =0 ; +nfsd_break_one_deleg (dp); dp->dl_file->fi_had_conflict = true; -nfsd4_cb_recall (dp); } static
Signature · 1 line
1.7.1
Re: [PATCH 1/6] nfsd4: split up nfsd_break_deleg_cb
fs/nfsd/nfs4state.c void nfsd_break_deleg_cb(struct file_lock *fl) · line 2357
Quoted 2 lines
fs/nfsd/nfs4state.c +2
-0+static +void nfsd_break_deleg_cb (struct file_lock *fl)
Very weird formatting. Please make it either
static void nfsd_break_deleg_cb(struct file_lock *fl)
or
static voidnfsd_break_deleg_cb(struct file_lock *fl)
but never just put the static on a line of it's own.
Re: [PATCH 1/6] nfsd4: split up nfsd_break_deleg_cb
Quoted 5 lines · Christoph Hellwig
fs/nfsd/nfs4state.c void nfsd_break_deleg_cb(struct file_lock *fl) · line 2357
Quoted 2 lines
fs/nfsd/nfs4state.c +2
-0+static +void nfsd_break_deleg_cb (struct file_lock *fl)Very weird formatting. Please make it either
Quoted 8 lines
static void
nfsd_break_deleg_cb(struct file_lock *fl)or
static void
nfsd_break_deleg_cb(struct file_lock *fl)but never just put the static on a line of it's own.
Agreed. Those two lines were actually untouched by this patch, no matter what diff thinks--but I'm just as happy to fix it up while we're there; done.
--b.
Re: [PATCH 1/6] nfsd4: split up nfsd_break_deleg_cb
J. Bruce Fields wrote:
Agreed. Those two lines were actually untouched by this patch, no matter what diff thinks--but I'm just as happy to fix it up while we're there; done.
I agree too, but "indent -linux" likes "static" on a line by itself, and prefers it over "static void". This is not the first time I've disagreed with it, although I use it a lot.
Re: [PATCH 1/6] nfsd4: split up nfsd_break_deleg_cb
Quoted 9 lines · Jim Rees
J. Bruce Fields wrote:
Agreed. Those two lines were actually untouched by this patch, no matter what diff thinks--but I'm just as happy to fix it up while we're there; done.
I agree too, but "indent -linux" likes "static" on a line by itself, and prefers it over "static void". This is not the first time I've disagreed with it, although I use it a lot.
The linux styles of most indent-like programs are utter garbage. If you want Linux style use the scripts/Lindent script in the source tree. It still has a few quirks, like stupid ways to split overly long lines, but generally does a reasonable job.
Re: [PATCH 1/6] nfsd4: split up nfsd_break_deleg_cb
Christoph Hellwig wrote:
The linux styles of most indent-like programs are utter garbage. If you want Linux style use the scripts/Lindent script in the source tree. It still has a few quirks, like stupid ways to split overly long lines, but generally does a reasonable job.
I'll give that a try, thanks. Someone should fix "indent -linux", or at least its man page.
PATCH 2/6 nfsd4: add helper function for lease setup
In reply to J. Bruce Fields--- fs/nfsd/nfs4state.c | 33 +++++++++++++++++++++------------ 1 files changed, 21 insertions(+), 12 deletions(-)
fs/nfsd/nfs4state.c +21 -12
@@ -2640,6 +2640,26 @@ static bool nfsd4_cb_channel_good(struct nfs4_client *clp)return clp->cl_minorversion && clp->cl_cb_state ==NFSD4_CB_UNKNOWN ; } +static struct file_lock *nfs4_alloc_init_lease (struct nfs4_delegation *dp,int flag) +{ +struct file_lock *fl; + + fl =locks_alloc_lock (); +if (!fl) +return NULL ; +locks_init_lock (fl); + fl->fl_lmops = &nfsd_lease_mng_ops; + fl->fl_flags =FL_LEASE ; + fl->fl_type = flag ==NFS4_OPEN_DELEGATE_READ ?F_RDLCK :F_WRLCK ; + fl->fl_end =OFFSET_MAX ; + fl->fl_owner = (fl_owner_t )dp; + fl->fl_file = dp->dl_vfs_file; +BUG_ON (!fl->fl_file); + fl->fl_pid = current->tgid; + dp->dl_flock = fl; +return fl; +} +/* * Attempt to hand out a delegation. */
@@ -2685,20 +2705,9 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_sta goto out; } status = -ENOMEM ; - fl =locks_alloc_lock (); + fl =nfs4_alloc_init_lease (dp, flag);if (!fl) goto out; -locks_init_lock (fl); - fl->fl_lmops = &nfsd_lease_mng_ops; - fl->fl_flags =FL_LEASE ; - fl->fl_type = flag ==NFS4_OPEN_DELEGATE_READ ?F_RDLCK :F_WRLCK ; - fl->fl_end =OFFSET_MAX ; - fl->fl_owner = (fl_owner_t )dp; - fl->fl_file =find_readable_file (stp->st_file); -BUG_ON (!fl->fl_file); - fl->fl_pid = current->tgid; - dp->dl_flock = fl; -/* vfs_setlease checks to see if delegation should be handed out. * the lock_manager callback fl_change is used */
Signature · 1 line
1.7.1
PATCH 3/6 nfsd4: fix leak on allocation error
In reply to J. Bruce FieldsAlso share some common exit code.
--- fs/nfsd/nfs4state.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-)
fs/nfsd/nfs4state.c +10 -8
@@ -2700,14 +2700,12 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_sta } dp =alloc_init_deleg (sop->so_client, stp, fh, flag); -if (dp ==NULL ) { - flag =NFS4_OPEN_DELEGATE_NONE ; - goto out; - } +if (dp ==NULL ) + goto out_no_deleg; status = -ENOMEM ; fl =nfs4_alloc_init_lease (dp, flag);if (!fl) - goto out; + goto out_free;/* vfs_setlease checks to see if delegation should be handed out. * the lock_manager callback fl_change is used */
@@ -2715,9 +2713,7 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_stadprintk ("NFSD: setlease failed [%d], no delegation\n" , status); dp->dl_flock =NULL ;locks_free_lock (fl); -unhash_delegation (dp); - flag =NFS4_OPEN_DELEGATE_NONE ; - goto out; + goto out_free; }memcpy (&open->op_delegate_stateid, &dp->dl_stateid,sizeof (dp->dl_stateid));
@@ -2730,6 +2726,12 @@ out: && open->op_delegate_type != NFS4_OPEN_DELEGATE_NONE)dprintk ("NFSD: WARNING: refusing delegation reclaim\n" ); open->op_delegate_type = flag; +return ; +out_free: +unhash_delegation (dp); +out_no_deleg: + flag =NFS4_OPEN_DELEGATE_NONE ; + goto out; }/*
Signature · 1 line
1.7.1
PATCH 4/6 nfsd4: split lease setting into separate function
In reply to J. Bruce FieldsSplitting some code into a separate function which we'll be adding some more to.
--- fs/nfsd/nfs4state.c | 32 +++++++++++++++++++------------- 1 files changed, 19 insertions(+), 13 deletions(-)
fs/nfsd/nfs4state.c +19 -13
@@ -2660,6 +2660,23 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp, int freturn fl; } +static int nfs4_setlease (struct nfs4_delegation *dp,int flag) +{ +struct file_lock *fl; +int status; + + fl =nfs4_alloc_init_lease (dp, flag); +if (!fl) +return -ENOMEM ; + status =vfs_setlease (dp->dl_vfs_file, fl->fl_type, &fl); +if (status) { + dp->dl_flock =NULL ; +locks_free_lock (fl); +return -ENOMEM ; + } +return 0 ; +} +/* * Attempt to hand out a delegation. */
@@ -2669,7 +2686,6 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_stastruct nfs4_delegation *dp;struct nfs4_stateowner *sop = stp->st_stateowner;int cb_up; -struct file_lock *fl;int status, flag =0 ; cb_up =nfsd4_cb_channel_good (sop->so_client);
@@ -2702,19 +2718,9 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_sta dp =alloc_init_deleg (sop->so_client, stp, fh, flag);if (dp ==NULL ) goto out_no_deleg; - status = -ENOMEM ; - fl =nfs4_alloc_init_lease (dp, flag); -if (!fl) - goto out_free; -/* vfs_setlease checks to see if delegation should be handed out. -* the lock_manager callback fl_change is used -*/ -if ((status =vfs_setlease (fl->fl_file, fl->fl_type, &fl))) { -dprintk ("NFSD: setlease failed [%d], no delegation\n" , status); - dp->dl_flock =NULL ; -locks_free_lock (fl); + status =nfs4_setlease (dp, flag); +if (status) goto out_free; - }memcpy (&open->op_delegate_stateid, &dp->dl_stateid,sizeof (dp->dl_stateid));
Signature · 1 line
1.7.1
PATCH 5/6 nfsd4: remove unused deleg dprintk's.
In reply to J. Bruce FieldsThese aren't all that useful, and get in the way of the next steps.
--- fs/nfsd/nfs4state.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
fs/nfsd/nfs4state.c +0 -4
@@ -958,8 +958,6 @@ expire_client(struct nfs4_client *clp)spin_lock (&recall_lock);while (!list_empty (&clp->cl_delegations)) { dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt); -dprintk ("NFSD: expire client. dp %p, fp %p\n" , dp, - dp->dl_flock);list_del_init (&dp->dl_perclnt);list_move (&dp->dl_recall_lru, &reaplist); }
@@ -2932,8 +2930,6 @@ nfs4_laundromat(void) test_val = u;break ; } -dprintk ("NFSD: purging unused delegation dp %p, fp %p\n" , - dp, dp->dl_flock);list_move (&dp->dl_recall_lru, &reaplist); }spin_unlock (&recall_lock);
Signature · 1 line
1.7.1
PATCH 6/6 nfsd4: modify fi_delegations under recall_lock
In reply to J. Bruce FieldsModify fi_delegations only under the recall_lock, allowing us to use that list on lease breaks.
Also some trivial cleanup to simplify later changes.
--- fs/nfsd/nfs4state.c | 99 +++++++++++++++++++++++++++++--------------------- fs/nfsd/state.h | 5 ++- 2 files changed, 60 insertions(+), 44 deletions(-)
fs/nfsd/nfs4state.c +57 -42
@@ -230,9 +230,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_f dp->dl_client = clp;get_nfs4_file (fp); dp->dl_file = fp; - dp->dl_vfs_file =find_readable_file (fp); -get_file (dp->dl_vfs_file); - dp->dl_flock =NULL ; dp->dl_type = type; dp->dl_stateid.si_boot = boot_time; dp->dl_stateid.si_stateownerid = current_delegid++;
@@ -241,8 +238,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_ffh_copy_shallow (&dp->dl_fh, ¤t_fh->fh_handle); dp->dl_time =0 ;atomic_set (&dp->dl_count,1 ); -list_add (&dp->dl_perfile, &fp->fi_delegations); -list_add (&dp->dl_perclnt, &clp->cl_delegations);INIT_WORK (&dp->dl_recall.cb_work, nfsd4_do_callback_rpc);return dp; }
@@ -253,36 +248,30 @@ nfs4_put_delegation(struct nfs4_delegation *dp)if (atomic_dec_and_test (&dp->dl_count)) {dprintk ("NFSD: freeing dp %p\n" ,dp);put_nfs4_file (dp->dl_file); -fput (dp->dl_vfs_file);kmem_cache_free (deleg_slab, dp); num_delegations--; } } -/* Remove the associated file_lock first, then remove the delegation. -* lease_modify() is called to remove the FS_LEASE file_lock from -* the i_flock list, eventually calling nfsd's lock_manager -* fl_release_callback. -*/ -static void -nfs4_close_delegation (struct nfs4_delegation *dp) +static void nfs4_put_deleg_lease (struct nfs4_file *fp) { -dprintk ("NFSD: close_delegation dp %p\n" ,dp); -/* XXX: do we even need this check?: */ -if (dp->dl_flock) -vfs_setlease (dp->dl_vfs_file,F_UNLCK , &dp->dl_flock); +if (atomic_dec_and_test (&fp->fi_delegees)) { +vfs_setlease (fp->fi_deleg_file,F_UNLCK , &fp->fi_lease); + fp->fi_lease =NULL ; + fp->fi_deleg_file =NULL ; + } }/* Called under the state lock. */ static void unhash_delegation (struct nfs4_delegation *dp) { -list_del_init (&dp->dl_perfile);list_del_init (&dp->dl_perclnt);spin_lock (&recall_lock); +list_del_init (&dp->dl_perfile);list_del_init (&dp->dl_recall_lru);spin_unlock (&recall_lock); -nfs4_close_delegation (dp); +nfs4_put_deleg_lease (dp->dl_file);nfs4_put_delegation (dp); }
@@ -2076,6 +2065,7 @@ alloc_init_file(struct inode *ino) fp->fi_inode =igrab (ino); fp->fi_id = current_fileid++; fp->fi_had_conflict = false; + 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);
@@ -2336,9 +2326,7 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)* it's safe to take a reference: */ atomic_inc (&dp->dl_count); -spin_lock (&recall_lock);list_add_tail (&dp->dl_recall_lru, &del_recall_lru); -spin_unlock (&recall_lock);/* only place dl_time is set. protected by lock_flocks*/ dp->dl_time =get_seconds ();
@@ -2346,26 +2334,28 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)nfsd4_cb_recall (dp); } -/* -* Called from break_lease() with lock_flocks() held. -* Note: we assume break_lease will only call this *once* for any given -* lease. -*/ +/* Called from break_lease() with lock_flocks() held. */ static void nfsd_break_deleg_cb (struct file_lock *fl) { -struct nfs4_delegation *dp = (struct nfs4_delegation *)fl->fl_owner; +struct nfs4_file *fp = (struct nfs4_file *)fl->fl_owner; +struct nfs4_delegation *dp; -BUG_ON (!dp); +BUG_ON (!fp); +/* We assume break_lease is only called once per lease: */ +BUG_ON (fp->fi_had_conflict);/* * We don't want the locks code to timeout the lease for us; -* we'll remove it ourself if the delegation isn't returned +* we'll remove it ourself if a delegation isn't returned * in time: */ fl->fl_break_time =0 ; -nfsd_break_one_deleg (dp); - dp->dl_file->fi_had_conflict = true; +spin_lock (&recall_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); } static
@@ -2456,13 +2446,15 @@ nfs4_check_delegmode(struct nfs4_delegation *dp, int flags)static struct nfs4_delegation *find_delegation_file (struct nfs4_file *fp,stateid_t *stid) { -struct nfs4_delegation *dp; +struct nfs4_delegation *dp =NULL ; +spin_lock (&recall_lock);list_for_each_entry (dp, &fp->fi_delegations, dl_perfile) {if (dp->dl_stateid.si_stateownerid == stid->si_stateownerid) -return dp; +break ; } -return NULL ; +spin_unlock (&recall_lock); +return dp; }int share_access_to_flags (u32 share_access)
@@ -2650,28 +2642,51 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp, int f fl->fl_flags =FL_LEASE ; fl->fl_type = flag ==NFS4_OPEN_DELEGATE_READ ?F_RDLCK :F_WRLCK ; fl->fl_end =OFFSET_MAX ; - fl->fl_owner = (fl_owner_t )dp; - fl->fl_file = dp->dl_vfs_file; -BUG_ON (!fl->fl_file); + fl->fl_owner = (fl_owner_t)(dp->dl_file); fl->fl_pid = current->tgid; - dp->dl_flock = fl;return fl; }static int nfs4_setlease (struct nfs4_delegation *dp,int flag) { +struct nfs4_file *fp = dp->dl_file;struct file_lock *fl;int status; fl =nfs4_alloc_init_lease (dp, flag);if (!fl)return -ENOMEM ; - status =vfs_setlease (dp->dl_vfs_file, fl->fl_type, &fl); + fl->fl_file =find_readable_file (fp); +list_add (&dp->dl_perclnt, &dp->dl_client->cl_delegations); + status =vfs_setlease (fl->fl_file, fl->fl_type, &fl);if (status) { - dp->dl_flock =NULL ; +list_del_init (&dp->dl_perclnt);locks_free_lock (fl);return -ENOMEM ; } + fp->fi_lease = fl; + fp->fi_deleg_file = fl->fl_file; +get_file (fp->fi_deleg_file); +atomic_set (&fp->fi_delegees,1 ); +list_add (&dp->dl_perfile, &fp->fi_delegations); +return 0 ; +} + +static int nfs4_set_delegation (struct nfs4_delegation *dp,int flag) +{ +struct nfs4_file *fp = dp->dl_file; + +if (!fp->fi_lease) +return nfs4_setlease (dp, flag); +spin_lock (&recall_lock); +if (fp->fi_had_conflict) { +spin_unlock (&recall_lock); +return -EAGAIN ; + } +atomic_inc (&fp->fi_delegees); +list_add (&dp->dl_perfile, &fp->fi_delegations); +spin_unlock (&recall_lock); +list_add (&dp->dl_perclnt, &dp->dl_client->cl_delegations);return 0 ; }
@@ -2716,7 +2731,7 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_sta dp =alloc_init_deleg (sop->so_client, stp, fh, flag);if (dp ==NULL ) goto out_no_deleg; - status =nfs4_setlease (dp, flag); + status =nfs4_set_delegation (dp, flag);if (status) goto out_free;
@@ -2732,7 +2747,7 @@ out: open->op_delegate_type = flag;return ; out_free: -unhash_delegation (dp); +nfs4_put_delegation (dp); out_no_deleg: flag =NFS4_OPEN_DELEGATE_NONE ; goto out;
fs/nfsd/state.h +3 -2
@@ -83,8 +83,6 @@ struct nfs4_delegation {atomic_t dl_count;/* ref count */ struct nfs4_client *dl_client;struct nfs4_file *dl_file; -struct file *dl_vfs_file; -struct file_lock *dl_flock;u32 dl_type;time_t dl_time;/* For recall: */
@@ -379,6 +377,9 @@ struct nfs4_file {*/ atomic_t fi_readers;atomic_t fi_writers; +struct file *fi_deleg_file; +struct file_lock *fi_lease; +atomic_t fi_delegees;struct inode *fi_inode;u32 fi_id;/* used with stateowner->so_id * for stateid_hashtbl hash */
Signature · 1 line
1.7.1