On 11/19/2010 05:27 PM, Andy Adamson wrote:
>
> On Nov 18, 2010, at 12:18 PM, Benny Halevy wrote:
>
>> On 2010-11-18 14:44, Boaz Harrosh wrote:
>>> On 11/17/2010 07:41 PM, Benny Halevy wrote:
>>>> squash into 4a28356 pnfs: CB_NOTIFY_DEVICEID
>>>>
>>>> We currently call pnfs_unhash_deviceid under spin_lock c->dc_lock
>>>> But pnfs_unhash_deviceid calls synchronize_rcu which may sched.
>>>> This resulted in the following BUG with the cthon tests:
>>>>
>>>> Nov 17 18:54:56 tl1 kernel: BUG: spinlock wrong CPU on CPU#1, test5/2170
>>>> Nov 17 18:54:56 tl1 kernel: lock: ffff880070559ff0, .magic: dead4ead, .owner: test5/2170, .owner_cpu: 0
>>>> Nov 17 18:54:56 tl1 kernel: Pid: 2170, comm: test5 Not tainted 2.6.37-rc2-pnfs #167
>>>> Nov 17 18:54:56 tl1 kernel: Call Trace:
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff8122cfc5>] spin_bug+0x9c/0xa3
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff8122d042>] do_raw_spin_unlock+0x76/0x8d
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff814534ea>] _raw_spin_unlock+0x2b/0x30
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffffa03c62fc>] pnfs_put_deviceid+0x59/0x64 [nfs]
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffffa0018551>] filelayout_free_lseg+0x5a/0x6f [nfs_layout_nfsv41_files]
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffffa03c6ea8>] pnfs_free_lseg_list+0x4e/0x8b [nfs]
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffffa03c85a4>] _pnfs_return_layout+0xe3/0x213 [nfs]
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffffa039bcb2>] nfs4_evict_inode+0x41/0x74 [nfs]
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff8112cab6>] evict+0x27/0x97
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff8112d051>] iput+0x20c/0x243
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff8112476c>] do_unlinkat+0x11c/0x16f
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff81118750>] ? fsnotify_modify+0x66/0x6e
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff81452cbe>] ? lockdep_sys_exit_thunk+0x35/0x67
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff810a0b51>] ? audit_syscall_entry+0x11e/0x14a
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff811247d5>] sys_unlink+0x16/0x18
>>>> Nov 17 18:54:56 tl1 kernel: [<ffffffff8100acf2>] system_call_fastpath+0x16/0x1b
>>>>
>>>> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxxx>
>>>> ---
>>>> fs/nfs/pnfs.c | 5 ++---
>>>> 1 files changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>>> index 559fcce..39c7d9f 100644
>>>> --- a/fs/nfs/pnfs.c
>>>> +++ b/fs/nfs/pnfs.c
>>>> @@ -1651,7 +1651,6 @@ pnfs_unhash_deviceid(struct pnfs_deviceid_cache *c,
>>>> hlist_for_each_entry_rcu(d, n, &c->dc_deviceids[h], de_node)
>>>> if (!memcmp(&d->de_id, id, sizeof(*id))) {
>>>> hlist_del_rcu(&d->de_node);
>>>> - synchronize_rcu();
>>>> return d;
>>>> }
>>>>
>>>> @@ -1672,7 +1671,7 @@ pnfs_put_deviceid(struct pnfs_deviceid_cache *c,
>>>>
>>>> pnfs_unhash_deviceid(c, &devid->de_id);
>>>> spin_unlock(&c->dc_lock);
>>>> -
>>>> + synchronize_rcu();
>>>> c->dc_free_callback(devid);
>>>> }
>>>> EXPORT_SYMBOL_GPL(pnfs_put_deviceid);
>>>> @@ -1686,7 +1685,7 @@ pnfs_delete_deviceid(struct pnfs_deviceid_cache *c,
>>>> spin_lock(&c->dc_lock);
>>>> devid = pnfs_unhash_deviceid(c, id);
>>>> spin_unlock(&c->dc_lock);
>>>> -
>>>> + synchronize_rcu();
>>>> dprintk("%s [%d]\n", __func__, atomic_read(&devid->de_ref));
>>>> if (atomic_dec_and_test(&devid->de_ref))
>>>> c->dc_free_callback(devid);
>>>
>>> OK, so I don't like this fix because before we only synchronize_rcu()
>>> after an actual hlist_del_rcu.
>>>
>>> If we look at the two callers
>>>
>>> ()
>>> if (!atomic_dec_and_lock(&devid->de_ref, &c->dc_lock))
>>> return;
>>>
>>> pnfs_unhash_deviceid(c, &devid->de_id);
>>> spin_unlock(&c->dc_lock);
>>>
>>> pnfs_delete_deviceid
>>> spin_lock(&c->dc_lock);
>>> devid = pnfs_unhash_deviceid(c, id);
>>> spin_unlock(&c->dc_lock);
>>>
>>>
>>> The pnfs_put_deviceid does an atomic_dec_and_lock so not to race with
>>> pnfs_find_get_deviceid. But in pnfs_find_get_deviceid we do atomic_inc_not_zero
>>> so we don't need that we can change pnfs_put_deviceid to just atomic_dec_and_test
>>> and then take the lock inside pnfs_unhash_deviceid, and remove from callers.
>>> Some thing like below. (Untested, only compiled)
>>
>> OK, that makes sense, BUT
>> If pnfs_find_get_deviceid can see de_ref==0 it's racing with pnfs_put_deviceid
>> which is about to free the structure - i.e. pnfs_find_get_deviceid may cause
>> use after free bugs.
>> So I'm confused if the current scheme works at all.
>>
>> Andy, it seems to me this optimization is a bit premature
>> and we'd be better off reverting to using simple spin locks rather the
>> rcu locks, certainly until we have a good testing infrastructure
>> for cb_notifydeviceid. I'm not sure how much we're saving anyhow.
>
> from rfc5661
> NOTIFY4_DEVICEID_DELETE
> Deletes a device ID from the mappings. This notification MUST NOT
> be sent if the client has a layout that refers to the device ID.
> In other words, if the server is sending a delete device ID
> notification, one of the following is true for layouts associated
> with the layout type:
>
> * The client never had a layout referring to that device ID.
>
> * The client has returned all layouts referring to that device
> ID.
>
> * The server has revoked all layouts referring to that device ID.
>
> In our current implementation, when the client has no layout segments referencing a device ID, it has been removed. So, what is gained by using NOTIFY_DEVICEID_DELETE?
>
> I see NOTIFY_DEVICEID4_CHANGE as useful. But I would treat it as an error case - very unlikely to happen. When it does happen, all I/0 using the deviceID needs to be drained, and new i/o stopped with associated rpc tasks put to sleep. Same for LAYOUTGETs. Then the state manager should be run to remap the deviceid, then wake up the rpc tasks. When only the state manager can access the deviceid list, this problem goes away.
>
> The current code - nfs4_callback_devicenotify - tries to remove the deviceid from the list. What it should be doing is checking to make sure that the deviceid is already gone, because the client is not supposed to be holding any layouts that reference it.
>
> -->Andy
>
No!
I thought you promised me that the release of devices from cache if not referenced
by a layout will be changed to a better cache policy like for example "only release
on umount". "The current implementation is only to fix the bugs with the code and
until we actually do more with the layouts" you said.
So please don't build any new assumptions on a broken model.
That said, We still have bugs at hand please lets fix them?
Boaz
>>
>> Benny
>>
>>>
>>> ---
>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>> index 61310c7..5d4e14d 100644
>>> --- a/fs/nfs/pnfs.c
>>> +++ b/fs/nfs/pnfs.c
>>> @@ -1598,16 +1598,21 @@ pnfs_unhash_deviceid(struct pnfs_deviceid_cache *c,
>>> {
>>> struct pnfs_deviceid_node *d;
>>> struct hlist_node *n;
>>> - long h = nfs4_deviceid_hash(id);
>>> + long h;
>>> +
>>> + spin_lock(&c->dc_lock);
>>> + h = nfs4_deviceid_hash(id);
>>>
>>> dprintk("%s hash %ld\n", __func__, h);
>>> hlist_for_each_entry_rcu(d, n, &c->dc_deviceids[h], de_node)
>>> if (!memcmp(&d->de_id, id, sizeof(*id))) {
>>> hlist_del_rcu(&d->de_node);
>>> + spin_unlock(&c->dc_lock);
>>> synchronize_rcu();
>>> return d;
>>> }
>>>
>>> + spin_unlock(&c->dc_lock);
>>> return NULL;
>>> }
>>>
>>> @@ -1620,11 +1625,10 @@ pnfs_put_deviceid(struct pnfs_deviceid_cache *c,
>>> struct pnfs_deviceid_node *devid)
>>> {
>>> dprintk("%s [%d]\n", __func__, atomic_read(&devid->de_ref));
>>> - if (!atomic_dec_and_lock(&devid->de_ref, &c->dc_lock))
>>> + if (!atomic_dec_and_test(&devid->de_ref))
>>> return;
>>>
>>> pnfs_unhash_deviceid(c, &devid->de_id);
>>> - spin_unlock(&c->dc_lock);
>>>
>>> c->dc_free_callback(devid);
>>> }
>>> @@ -1636,9 +1640,7 @@ pnfs_delete_deviceid(struct pnfs_deviceid_cache *c,
>>> {
>>> struct pnfs_deviceid_node *devid;
>>>
>>> - spin_lock(&c->dc_lock);
>>> devid = pnfs_unhash_deviceid(c, id);
>>> - spin_unlock(&c->dc_lock);
>>>
>>> dprintk("%s [%d]\n", __func__, atomic_read(&devid->de_ref));
>>> if (atomic_dec_and_test(&devid->de_ref))
>>>
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
