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

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

[Yang Erkun]

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.

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

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

[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.

>
> 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
>
>

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

[Christoph Hellwig]

On Sat, Jul 20, 2024 at 04:35:38PM +0800, Yang Erkun 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.

This honestly sounds like a bug in tmpfs, so maybe we should discuss
the behavior first?

If the changes goes in please write a comment documenting it in the
test case.

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

[Chuck Lever III]

> On Jul 22, 2024, at 10:21 AM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Sat, Jul 20, 2024 at 04:35:38PM +0800, Yang Erkun 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.
> 
> This honestly sounds like a bug in tmpfs, so maybe we should discuss
> the behavior first?

Agreed, sounds like there should be some root cause analysis
before the tests are altered.

Is this problem addressed in recent kernels?

--
Chuck Lever

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

[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!

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
>>
>>

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

[yangerkun]

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.

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
>>>
>>>

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

[Christoph Hellwig]

On Mon, Jul 29, 2024 at 09:53:52PM +0800, yangerkun wrote:
> 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.
> 
> Looking forward to your comments!

I agree to all of the above.

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

[yangerkun]

在 2024/7/29 22:21, Christoph Hellwig 写道:
> On Mon, Jul 29, 2024 at 09:53:52PM +0800, yangerkun wrote:
>> 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.
>>
>> Looking forward to your comments!
> 
> I agree to all of the above.
> 

Thanks, I will try to write a patch for this!

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

[Chuck Lever III]

> On Jul 29, 2024, at 9:53 AM, 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.
> 
> Looking forward to your comments!

Is this the same bug as https://bugzilla.kernel.org/show_bug.cgi?id=219094 ?


> 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
>>>> 
>>>> 

--
Chuck Lever

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

[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

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
> >>>
> >>>

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

[Filipe Manana]

On Mon, Jul 29, 2024 at 3:30 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Jul 29, 2024, at 9:53 AM, 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.
> >
> > Looking forward to your comments!
>
> Is this the same bug as https://bugzilla.kernel.org/show_bug.cgi?id=219094 ?

Yes, my last comment there explicitly points to this thread.

>
>
> > 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
> >>>>
> >>>>
>
> --
> Chuck Lever
>
>

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

[yangerkun]

在 2024/7/29 22:29, Chuck Lever III 写道:
> 
> 
>> On Jul 29, 2024, at 9:53 AM, 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.
>>
>> Looking forward to your comments!
> 
> Is this the same bug as https://bugzilla.kernel.org/show_bug.cgi?id=219094 ?

Yes.


> 
> 
>> 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
>>>>>
>>>>>
> 
> --
> Chuck Lever
> 
> 

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

[yangerkun]

在 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
>>>>>
>>>>>