diff mbox

[2/2] nfsd: remove_stid can be incorporated into nfs4_put_delegation

Message ID 1381820280-14120-1-git-send-email-bhalevy@primarydata.com (mailing list archive)
State New, archived
Headers show

Commit Message

Benny Halevy Oct. 15, 2013, 6:58 a.m. UTC 
All calls to nfs4_put_delegation are preceded with remove_stid.

Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
---
 fs/nfsd/nfs4state.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

J. Bruce Fields Oct. 28, 2013, 8:03 p.m. UTC | #1 
On Tue, Oct 15, 2013 at 09:58:00AM +0300, Benny Halevy wrote:
> All calls to nfs4_put_delegation are preceded with remove_stid.

Applying, thanks.  Should be at git://linux-nfs.org/~bfields/linux.git
for-3.13 soon.  Let me know if any you have any other patches ready to
merge now.

--b.

> 
> Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
> ---
>  fs/nfsd/nfs4state.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b8f3c7e..93160b6 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -410,6 +410,7 @@ static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
>  void
>  nfs4_put_delegation(struct nfs4_delegation *dp)
>  {
> +	remove_stid(&dp->dl_stid);
>  	if (atomic_dec_and_test(&dp->dl_count)) {
>  		nfs4_free_stid(deleg_slab, &dp->dl_stid);
>  		num_delegations--;
> @@ -450,14 +451,12 @@ static void unhash_stid(struct nfs4_stid *s)
>  static void destroy_revoked_delegation(struct nfs4_delegation *dp)
>  {
>  	list_del_init(&dp->dl_recall_lru);
> -	remove_stid(&dp->dl_stid);
>  	nfs4_put_delegation(dp);
>  }
>  
>  static void destroy_delegation(struct nfs4_delegation *dp)
>  {
>  	unhash_delegation(dp);
> -	remove_stid(&dp->dl_stid);
>  	nfs4_put_delegation(dp);
>  }
>  
> @@ -3157,7 +3156,6 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
>  	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
>  	return;
>  out_free:
> -	remove_stid(&dp->dl_stid);
>  	nfs4_put_delegation(dp);
>  out_no_deleg:
>  	open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
> -- 
> 1.8.3.1
> 
--
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
J. Bruce Fields Nov. 4, 2013, 10:47 p.m. UTC | #2 
On Tue, Oct 15, 2013 at 09:58:00AM +0300, Benny Halevy wrote:
> All calls to nfs4_put_delegation are preceded with remove_stid.

Whoops, no, we missed the nfs4_put_delegation call in
fs/nfsd/nfs4callback.c.

Noticed because some pynfs tests triggered idr warnings about freeing
the same id twice.

I guess I'll revert.

--b.

> 
> Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
> ---
>  fs/nfsd/nfs4state.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b8f3c7e..93160b6 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -410,6 +410,7 @@ static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
>  void
>  nfs4_put_delegation(struct nfs4_delegation *dp)
>  {
> +	remove_stid(&dp->dl_stid);
>  	if (atomic_dec_and_test(&dp->dl_count)) {
>  		nfs4_free_stid(deleg_slab, &dp->dl_stid);
>  		num_delegations--;
> @@ -450,14 +451,12 @@ static void unhash_stid(struct nfs4_stid *s)
>  static void destroy_revoked_delegation(struct nfs4_delegation *dp)
>  {
>  	list_del_init(&dp->dl_recall_lru);
> -	remove_stid(&dp->dl_stid);
>  	nfs4_put_delegation(dp);
>  }
>  
>  static void destroy_delegation(struct nfs4_delegation *dp)
>  {
>  	unhash_delegation(dp);
> -	remove_stid(&dp->dl_stid);
>  	nfs4_put_delegation(dp);
>  }
>  
> @@ -3157,7 +3156,6 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
>  	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
>  	return;
>  out_free:
> -	remove_stid(&dp->dl_stid);
>  	nfs4_put_delegation(dp);
>  out_no_deleg:
>  	open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
> -- 
> 1.8.3.1
> 
--
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
Benny Halevy Nov. 5, 2013, 4:05 a.m. UTC | #3 
On 2013-11-04 14:47, J. Bruce Fields wrote:
> On Tue, Oct 15, 2013 at 09:58:00AM +0300, Benny Halevy wrote:
>> All calls to nfs4_put_delegation are preceded with remove_stid.
> 
> Whoops, no, we missed the nfs4_put_delegation call in
> fs/nfsd/nfs4callback.c.
> 
> Noticed because some pynfs tests triggered idr warnings about freeing
> the same id twice.
> 
> I guess I'll revert.

OK.

Benny

> 
> --b.
> 
>>
>> Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
>> ---
>>  fs/nfsd/nfs4state.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index b8f3c7e..93160b6 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -410,6 +410,7 @@ static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
>>  void
>>  nfs4_put_delegation(struct nfs4_delegation *dp)
>>  {
>> +	remove_stid(&dp->dl_stid);
>>  	if (atomic_dec_and_test(&dp->dl_count)) {
>>  		nfs4_free_stid(deleg_slab, &dp->dl_stid);
>>  		num_delegations--;
>> @@ -450,14 +451,12 @@ static void unhash_stid(struct nfs4_stid *s)
>>  static void destroy_revoked_delegation(struct nfs4_delegation *dp)
>>  {
>>  	list_del_init(&dp->dl_recall_lru);
>> -	remove_stid(&dp->dl_stid);
>>  	nfs4_put_delegation(dp);
>>  }
>>  
>>  static void destroy_delegation(struct nfs4_delegation *dp)
>>  {
>>  	unhash_delegation(dp);
>> -	remove_stid(&dp->dl_stid);
>>  	nfs4_put_delegation(dp);
>>  }
>>  
>> @@ -3157,7 +3156,6 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
>>  	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
>>  	return;
>>  out_free:
>> -	remove_stid(&dp->dl_stid);
>>  	nfs4_put_delegation(dp);
>>  out_no_deleg:
>>  	open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
>> -- 
>> 1.8.3.1
>>
--
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
Christoph Hellwig Nov. 7, 2013, 5:02 p.m. UTC | #4 
On Mon, Nov 04, 2013 at 08:05:08PM -0800, Benny Halevy wrote:
> On 2013-11-04 14:47, J. Bruce Fields wrote:
> > On Tue, Oct 15, 2013 at 09:58:00AM +0300, Benny Halevy wrote:
> >> All calls to nfs4_put_delegation are preceded with remove_stid.
> > 
> > Whoops, no, we missed the nfs4_put_delegation call in
> > fs/nfsd/nfs4callback.c.
> > 
> > Noticed because some pynfs tests triggered idr warnings about freeing
> > the same id twice.
> > 
> > I guess I'll revert.
> 
> OK.

Not sure which patch in your submitted series it was, but with the
whole series xfstests on NFS 4.1 crashed and burned early on.  I'd
recommend you run xfstests to test any future changes to the state
handling code in nfsd.
--
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
J. Bruce Fields Nov. 7, 2013, 5:05 p.m. UTC | #5 
On Thu, Nov 07, 2013 at 09:02:04AM -0800, Christoph Hellwig wrote:
> On Mon, Nov 04, 2013 at 08:05:08PM -0800, Benny Halevy wrote:
> > On 2013-11-04 14:47, J. Bruce Fields wrote:
> > > On Tue, Oct 15, 2013 at 09:58:00AM +0300, Benny Halevy wrote:
> > >> All calls to nfs4_put_delegation are preceded with remove_stid.
> > > 
> > > Whoops, no, we missed the nfs4_put_delegation call in
> > > fs/nfsd/nfs4callback.c.
> > > 
> > > Noticed because some pynfs tests triggered idr warnings about freeing
> > > the same id twice.
> > > 
> > > I guess I'll revert.
> > 
> > OK.
> 
> Not sure which patch in your submitted series it was, but with the
> whole series xfstests on NFS 4.1 crashed and burned early on.  I'd
> recommend you run xfstests to test any future changes to the state
> handling code in nfsd.

What did you run it on exactly?  (One of my branches or one of Benny's?)

I keep saying I should do regular xfstest runs and keep not doing it.
Do you have a sample commandline to share for nfs testing?

--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
Christoph Hellwig Nov. 7, 2013, 5:19 p.m. UTC | #6 
On Thu, Nov 07, 2013 at 12:05:53PM -0500, J. Bruce Fields wrote:
> What did you run it on exactly?  (One of my branches or one of Benny's?)

Bennys tree (+ some local changes)

> I keep saying I should do regular xfstest runs and keep not doing it.
> Do you have a sample commandline to share for nfs testing?

cat > /etc/xfsqa.config << EOF
TEST_DIR=/mnt/nfs1
TEST_DEV=127.0.0.1:/mnt/test
EOF

./check -nfs -g auto
--
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
Christoph Hellwig Nov. 7, 2013, 5:47 p.m. UTC | #7 
On Thu, Nov 07, 2013 at 09:19:08AM -0800, Christoph Hellwig wrote:
> On Thu, Nov 07, 2013 at 12:05:53PM -0500, J. Bruce Fields wrote:
> > What did you run it on exactly?  (One of my branches or one of Benny's?)
> 
> Bennys tree (+ some local changes)
> 
> > I keep saying I should do regular xfstest runs and keep not doing it.
> > Do you have a sample commandline to share for nfs testing?
> 
> cat > /etc/xfsqa.config << EOF
> TEST_DIR=/mnt/nfs1
> TEST_DEV=127.0.0.1:/mnt/test
> EOF

Sorry, this missed half of what's needed:

SCRATCH_MNT=/mnt/nfs1
SCRATCH_DEV=127.0.0.1:/mnt/scratch

For historical reason xfstests mixes up the scratch and test devices
for NFS in many older tests.

--
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
Benny Halevy Nov. 7, 2013, 8:01 p.m. UTC | #8 
On 2013-11-07 09:02, Christoph Hellwig wrote:
> On Mon, Nov 04, 2013 at 08:05:08PM -0800, Benny Halevy wrote:
>> On 2013-11-04 14:47, J. Bruce Fields wrote:
>>> On Tue, Oct 15, 2013 at 09:58:00AM +0300, Benny Halevy wrote:
>>>> All calls to nfs4_put_delegation are preceded with remove_stid.
>>>
>>> Whoops, no, we missed the nfs4_put_delegation call in
>>> fs/nfsd/nfs4callback.c.
>>>
>>> Noticed because some pynfs tests triggered idr warnings about freeing
>>> the same id twice.
>>>
>>> I guess I'll revert.
>>
>> OK.
> 
> Not sure which patch in your submitted series it was, but with the
> whole series xfstests on NFS 4.1 crashed and burned early on.  I'd
> recommend you run xfstests to test any future changes to the state
> handling code in nfsd.
> 

Thanks. Will do.
--
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
Christoph Hellwig Nov. 7, 2013, 9:36 p.m. UTC | #9 
FYI, here is the actual oops:

generic/013 9s ...[  252.796884] ------------[ cut here ]------------
[  252.797661] kernel BUG at fs/nfsd/nfs4state.c:141!
[  252.798391] invalid opcode: 0000 [#1] SMP 
[  252.799191] Modules linked in:
[  252.799780] CPU: 1 PID: 466 Comm: kworker/1:1 Not tainted 3.12.0-rc3+ #97
[  252.799996] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  252.799996] Workqueue: rpciod rpc_async_schedule
[  252.799996] task: ffff88007d79e640 ti: ffff88007cc5a000 task.ti: ffff88007cc5a000
[  252.799996] RIP: 0010:[<ffffffff8134c6b1>]  [<ffffffff8134c6b1>] nfs4_assert_state_locked+0x11/0x20
[  252.799996] RSP: 0018:ffff88007cc5bce8  EFLAGS: 00010246
[  252.799996] RAX: 0000000000000001 RBX: ffff880078f8ed48 RCX: 0000000000000000
[  252.799996] RDX: dead000000200200 RSI: 0000000000000000 RDI: ffff880078f8ed48
[  252.799996] RBP: ffff88007cc5bce8 R08: 0000000000000000 R09: ffff88007bb24350
[  252.799996] R10: ffff88007fbfbfd0 R11: 0000000000000000 R12: ffff88007bb24800
[  252.799996] R13: ffffffff81afd0f0 R14: 0000000000000000 R15: 0000000000000000
[  252.799996] FS:  0000000000000000(0000) GS:ffff88007fc80000(0000) knlGS:0000000000000000
[  252.799996] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b [  252.799996] CR2: ffffffffff600400 CR3: 0000000075204000 CR4: 00000000000006e0
[  252.799996] Stack:
[  252.799996]  ffff88007cc5bd08 ffffffff8134ccdc ffff880078f8ed48 ffff88007bb24800
[  252.799996]  ffff88007cc5bd28 ffffffff8134ce11 ffff88007cc5bd38 ffff880078f8ee40
[  252.799996]  ffff88007cc5bd48 ffffffff81355b6a ffff88007cdce480 0000000000000e81
[  252.799996] Call Trace:
[  252.799996]  [<ffffffff8134ccdc>] nfsd4_remove_stid+0x1c/0x40
[  252.799996]  [<ffffffff8134ce11>] nfs4_put_delegation+0x11/0x40
[  252.799996]  [<ffffffff81355b6a>] nfsd4_cb_recall_release+0x6a/0x80
[  252.799996]  [<ffffffff81afd67e>] rpc_free_task+0x2e/0x50
[  252.799996]  [<ffffffff81afd6f5>] rpc_final_put_task+0x55/0x60
[  252.799996]  [<ffffffff81afd87c>] __rpc_execute+0x17c/0x280
[  252.799996]  [<ffffffff81afd9a5>] rpc_async_schedule+0x25/0x40
[  252.799996]  [<ffffffff810ab58c>] process_one_work+0x16c/0x3e0
[  252.799996]  [<ffffffff810abc39>] worker_thread+0x119/0x370
[  252.799996]  [<ffffffff810abb20>] ? rescuer_thread+0x2e0/0x2e0
[  252.799996]  [<ffffffff810b1c6b>] kthread+0xbb/0xc0
[  252.799996]  [<ffffffff81040000>] ? vmx_handle_exit+0x450/0x8d0
[  252.799996]  [<ffffffff810b1bb0>] ? kthread_freezable_should_stop+0x60/0x60
[  252.799996]  [<ffffffff81bc78cc>] ret_from_fork+0x7c/0xb0
[  252.799996]  [<ffffffff810b1bb0>] ? kthread_freezable_should_stop+0x60/0x60
[  252.799996] Code: 16 82 48 89 e5 e8 c0 06 87 00 5d c3 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 8b 05 ba 27 e2 00 55 48 89 e5 83 f8 01 74 02 5d c3 <0f> 0b 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 41 57 

--
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 mbox

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b8f3c7e..93160b6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -410,6 +410,7 @@  static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
 void
 nfs4_put_delegation(struct nfs4_delegation *dp)
 {
+	remove_stid(&dp->dl_stid);
 	if (atomic_dec_and_test(&dp->dl_count)) {
 		nfs4_free_stid(deleg_slab, &dp->dl_stid);
 		num_delegations--;
@@ -450,14 +451,12 @@  static void unhash_stid(struct nfs4_stid *s)
 static void destroy_revoked_delegation(struct nfs4_delegation *dp)
 {
 	list_del_init(&dp->dl_recall_lru);
-	remove_stid(&dp->dl_stid);
 	nfs4_put_delegation(dp);
 }
 
 static void destroy_delegation(struct nfs4_delegation *dp)
 {
 	unhash_delegation(dp);
-	remove_stid(&dp->dl_stid);
 	nfs4_put_delegation(dp);
 }
 
@@ -3157,7 +3156,6 @@  static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
 	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
 	return;
 out_free:
-	remove_stid(&dp->dl_stid);
 	nfs4_put_delegation(dp);
 out_no_deleg:
 	open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;