On Jan 24, 2014, at 17:54, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Fri, 24 Jan 2014 17:39:45 -0700
> Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>
>>
>> On Jan 24, 2014, at 14:21, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>>
>>> On Fri, 24 Jan 2014 11:46:41 -0700
>>> Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>>>>
>>>> Convert your patch to use wait_on_bit(), and then to call
>>>> wait_on_bit_lock() if and only if you see NFS_INO_INVALID_DATA is set.
>>>>
>>>
>>> I think that too would be racy...
>>>
>>> We have to clear NFS_INO_INVALID_DATA while holding the i_lock, but we
>>> can't wait_on_bit_lock() under that. So (pseudocode):
>>>
>>> wait_on_bit
>>> take i_lock
>>> check and clear NFS_INO_INVALID_DATA
>>> drop i_lock
>>> wait_on_bit_lock
>>>
>>> ...so between dropping the i_lock and wait_on_bit_lock, we have a place
>>> where another task could check the flag and find it clear.
>>
>>
>> for(;;) {
>> wait_on_bit(NFS_INO_INVALIDATING)
>> /* Optimisation: don’t lock NFS_INO_INVALIDATING
>> * if NFS_INO_INVALID_DATA was cleared while we waited.
>> */
>> if (!test_bit(NFS_INO_INVALID_DATA))
>> return;
>> if (!test_and_set_bit_lock(NFS_INO_INVALIDATING))
>> break;
>> }
>> spin_lock(inode->i_lock);
>> if (!test_and_clear_bit(NFS_INO_INVALID_DATA)) {
>> spin_unlock(inode->i_lock);
>> goto out_raced;
>> }
>> ….
>> out_raced:
>> clear_bit(NFS_INO_INVALIDATING)
>> wake_up_bit(NFS_INO_INVALIDATING)
>>
>>
>> --
>> Trond Myklebust
>> Linux NFS client maintainer
>>
>
> Hmm maybe. OTOH, if we're using atomic bitops do we need to deal with
> the spinlock? I'll ponder it over the weekend and give it a harder
> look on Monday.
>
The NFS_I(inode)->cache_validity doesn’t use bitops, so the correct behaviour is to put NFS_INO_INVALIDATING inside NFS_I(inode)->flags (which is an atomic bit op field), and then continue to use the spin lock for NFS_INO_INVALID_DATA.
--
Trond Myklebust
Linux NFS client maintainer
--
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
