| Message ID | 20240720083538.2999155-1-yangerkun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic/736: don't run it on tmpfs | expand |
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 > >
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.
> 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
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 >> >>
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 >>> >>>
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.
在 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!
> 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
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 > >>> > >>>
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 > >
在 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 > >
在 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 >>>>> >>>>>
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
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(-)