On Mon, Jul 29, 2024 at 2:54 PM yangerkun <yangerkun@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxx/ > >> > >> 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@xxxxxxxxxx> > >>> --- > >>> 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 > >>> > >>>
