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

[Filipe Manana <fdmanana@kernel.org>]

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