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

[Filipe Manana <fdmanana@kernel.org>]

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