[PATCH 1/2] nfsd: nfs4_open_delegation needs to remove_stid rather than unhash_stid
bug fix
[PATCH 2/2] nfsd: remove_stid can be incorporated into nfs4_put_delegation
cleanup
In the out_free: path, the newly allocated stid must be removed rather
than unhashed so it can never be found.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 64c167f..b8f3c7e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3157,7 +3157,7 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
return;
out_free:
- unhash_stid(&dp->dl_stid);
+ remove_stid(&dp->dl_stid);
nfs4_put_delegation(dp);
out_no_deleg:
open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
--
1.8.3.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 <[email protected]>
> ---
> 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
>
All calls to nfs4_put_delegation are preceded with remove_stid.
Signed-off-by: Benny Halevy <[email protected]>
---
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
On Tue, Oct 15, 2013 at 09:57:55AM +0300, Benny Halevy wrote:
> In the out_free: path, the newly allocated stid must be removed rather
> than unhashed so it can never be found.
Applying, thanks.--b.
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 64c167f..b8f3c7e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3157,7 +3157,7 @@ static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
> open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> return;
> out_free:
> - unhash_stid(&dp->dl_stid);
> + remove_stid(&dp->dl_stid);
> nfs4_put_delegation(dp);
> out_no_deleg:
> open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
> --
> 1.8.3.1
>
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 <[email protected]>
>> ---
>> 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
>>
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.
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.
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 <[email protected]>
> ---
> 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
>
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
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.
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
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.