On Jan 24, 2014, at 18:05, Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>
> 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.
In other words please replace the atomic test_bit(NFS_INO_INVALID_DATA) and test_and_clear_bit(NFS_INO_INVALID_DATA) in the above pseudocode with the appropriate tests and clears of NFS_I(inode)->cache_validity.
--
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
