Re: [PATCH] generic/736: don't run it on tmpfs

[yangerkun <yangerkun@huawei.com>]

在 2024/7/29 22:32, Filipe Manana 写道:
> On Mon, Jul 29, 2024 at 2:54 PM yangerkun <yangerkun@huawei.com> wrote:
>>
>> Hi,
>>
>> 在 2024/7/24 21:30, yangerkun 写道:
>>> Hi, All,
>>>
>>> Sorry for the delay relay(something happened, and cannot use pc
>>> before...).
>>>
>>> 在 2024/7/21 1:26, Filipe Manana 写道:
>>>> On Sat, Jul 20, 2024 at 9:38 AM Yang Erkun <yangerkun@huawei.com> wrote:
>>>>>
>>>>> We use offset_readdir for tmpfs, and every we call rename, the offset
>>>>> for the parent dir will increase by 1. So for tmpfs we will always
>>>>> fail since the infinite readdir.
>>>>
>>>> Having an infinite readdir sounds like a bug, or at least an
>>>> inconvenience and surprising for users.
>>>> We had that problem in btrfs which affected users/applications, see:
>>>>
>>>> https://lore.kernel.org/linux-btrfs/2c8c55ec-04c6-e0dc-9c5c-8c7924778c35@landley.net/
>>>>
>>>> which was surprising for them since every other filesystem they
>>>> used/tested didn't have that problem.
>>>> Why not fix tmpfs?
>>>
>>> Thanks for all your advise, I will give a detail analysis first(maybe
>>> until last week I can do it), and after we give a conclusion about does
>>> this behavior a bug or something expected to occur, I will choose the
>>> next step!
>>
>> The case generic/736 do something like below:
>>
>> 1. create 5000 files(1 2 3 ...) under one dir(testdir)
>> 2. call readdir(man 3 readdir) once, and get entry
>> 3. rename(entry, "TEMPFILE"), then rename("TMPFILE", entry)
>> 4. loop 2~3, until readdir return nothing of we loop too many times(15000)
>>
>> For tmpfs before a2e459555c5f("shmem: stable directory offsets"), every
>> rename called, the new dentry will insert to d_subdirs *head* of parent
>> dentry, and dcache_readdir won't reenter this dentry if we have already
>> enter the dentry, so in step 4 we will break the test since readdir
>> return nothing  (I have try to change __d_move the insert to the "tail"
>> of d_sub_dirs, problem can still happend).
>>
>> But after commit a2e459555c5f("shmem: stable directory offsets"),
>> simple_offset_rename will just add the new dentry to the maple tree of
>> &SHMEM_I(inode)->dir_offsets->mt with the key always inc by 1(since
>> simple_offset_add we will find free entry start with octx->newx_offset,
>> so the entry freed in simple_offset_remove won't be found). And the same
>> case upper will be break since we loop too many times(we can fall into
>> infinite readdir without this break).
>>
>> I prefer this is really a bug, and for the way to fix it, I think we can
>> just use the same logic what 9b378f6ad48cf("btrfs: fix infinite
>> directory reads") has did, introduce a last_index when we open the dir,
>> and then readdir will not return the entry which index greater than the
>> last index.
> 
> Don't forget to reset the index to whatever is the current last index
> when rewind() is called.
> We ended up with that bug in btrfs later, see:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e60aa5da14d01fed8411202dbe4adf6c44bd2a57

Thanks for your reminder, will change offset_dir_llseek too!

> 
> Anyway, if the same mistake is made, it would be caught by a test case
> for fstests I submitted after:
> 
> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=68b958f5dc4ab13cfd86f7fb82621f9f022b7626
> 
> 
> 
>>
>> Looking forward to your comments!
>>
>> Thanks,
>> Erkun.
>>
>>
>>
>>>
>>> Thanks again for all your advise!
>>>
>>>
>>>>
>>>> Thanks.
>>>>
>>>>>
>>>>> Signed-off-by: Yang Erkun <yangerkun@huawei.com>
>>>>> ---
>>>>>    tests/generic/736 | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tests/generic/736 b/tests/generic/736
>>>>> index d2432a82..9fafa8df 100755
>>>>> --- a/tests/generic/736
>>>>> +++ b/tests/generic/736
>>>>> @@ -18,7 +18,7 @@ _cleanup()
>>>>>           rm -fr $target_dir
>>>>>    }
>>>>>
>>>>> -_supported_fs generic
>>>>> +_supported_fs generic ^tmpfs
>>>>>    _require_test
>>>>>    _require_test_program readdir-while-renames
>>>>>
>>>>> --
>>>>> 2.39.2
>>>>>
>>>>>