| Message ID | 20250228-mm-selftests-v3-8-958e3b6f0203@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/mm: Some cleanups from trying to run them | expand |
On 28.02.25 17:54, Brendan Jackman wrote: > Some filesystems don't support funtract()ing unlinked files. They return > ENOENT. In that case, skip the test. > That's not documented in the man page, so is this a bug of these filesystems? What are examples for these weird filesystems? As we have the fstype available, we could instead simply reject more filesystems earlier. See fs_is_unknown(). > Signed-off-by: Brendan Jackman <jackmanb@google.com> > --- > tools/testing/selftests/mm/gup_longterm.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c > index 879e9e4e8cce8127656fabe098abf7db5f6c5e23..494ec4102111b9c96fb4947b29c184735ceb8e1c 100644 > --- a/tools/testing/selftests/mm/gup_longterm.c > +++ b/tools/testing/selftests/mm/gup_longterm.c > @@ -96,7 +96,15 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) > int ret; > > if (ftruncate(fd, size)) { > - ksft_test_result_fail("ftruncate() failed (%s)\n", strerror(errno)); > + if (errno == ENOENT) { > + /* > + * This can happen if the file has been unlinked and the > + * filesystem doesn't support truncating unlinked files. > + */ > + ksft_test_result_skip("ftruncate() failed with ENOENT\n"); > + } else { > + ksft_test_result_fail("ftruncate() failed (%s)\n", strerror(errno)); > + } > return; > } > >
On Thu, Mar 06, 2025 at 10:28:09AM +0100, David Hildenbrand wrote: > On 28.02.25 17:54, Brendan Jackman wrote: > > Some filesystems don't support funtract()ing unlinked files. They return > > ENOENT. In that case, skip the test. > > > > That's not documented in the man page, so is this a bug of these > filesystems? Um... unlink(2) does say: If the name was the last link to a file but any processes still have the file open, the file will remain in existence until the last file descriptor referring to it is closed. And POSIX says If one or more processes have the file open when the last link is removed, the link shall be removed before unlink() returns, but the removal of the file contents shall be postponed until all references to the file are closed I didn't call it a bug in the commit message because my impression was always that filesystem semantics are broadly determined by vibes. But looking at the above I do feel more confident that the "unlink isn't delete" thing is actually a pretty solid expectation. > What are examples for these weird filesystems? My experience of the issue is with 9pfs. broonie reported on #mm that NFS can display similar issues but I haven't hit it myself. > As we have the fstype available, we could instead simply reject more > filesystems earlier. See fs_is_unknown(). Oh. I didn't know this was so easy, I thought that checking the filesystem type would require some awful walk to find the mountpoint and join it against the mount list. (Now I think about it, I should have recorded this rationale in the commit message, so you could easily see my bogus reasoning). If there's a syscall to just say "what FS is this file on please?" we should just do that and explicitly denylist the systems that are known to have issues. I will just do 9pfs for now. Maybe we can log warning if the error shows up on systems that aren't listed, then if someone does run into it on NFS they should get a strong clue about what the problem is. Thanks!
On 06.03.25 13:42, Brendan Jackman wrote: > On Thu, Mar 06, 2025 at 10:28:09AM +0100, David Hildenbrand wrote: >> On 28.02.25 17:54, Brendan Jackman wrote: >>> Some filesystems don't support funtract()ing unlinked files. They return >>> ENOENT. In that case, skip the test. >>> >> >> That's not documented in the man page, so is this a bug of these >> filesystems? > Note that I meant that ftruncate doesn't mention this in the man page. The only occurrence is "ENOENT The named file does not exist.", and that only applies to truncate, not ftruncate. > Um... > > unlink(2) does say: > > If the name was the last link to a file but any processes still have > the file open, the file will remain in existence until the last file > descriptor referring to it is closed. > > And POSIX says > > If one or more processes have the file open when the last link is > removed, the link shall be removed before unlink() returns, but the > removal of the file contents shall be postponed until all references > to the file are closed Right, it's supposed to just stay around, it simply cannot be looked up anymore. > I didn't call it a bug in the commit message because my impression was > always that filesystem semantics are broadly determined by vibes. But > looking at the above I do feel more confident that the "unlink isn't > delete" thing is actually a pretty solid expectation. I have a faint recollection that 9pfs is problematic with unlink ... and indeed: https://gitlab.com/qemu-project/qemu/-/issues/103 I'm not sure at this point what's expected to work and what not with 9pfs at this point. > >> What are examples for these weird filesystems? > > My experience of the issue is with 9pfs. broonie reported on #mm that > NFS can display similar issues but I haven't hit it myself. > >> As we have the fstype available, we could instead simply reject more >> filesystems earlier. See fs_is_unknown(). > > Oh. I didn't know this was so easy, I thought that checking the > filesystem type would require some awful walk to find the mountpoint > and join it against the mount list. (Now I think about it, I should > have recorded this rationale in the commit message, so you could > easily see my bogus reasoning). > > If there's a syscall to just say "what FS is this file on please?" > we should just do that and explicitly denylist the systems that are > known to have issues. I will just do 9pfs for now. Maybe we can log > warning if the error shows up on systems that aren't listed, then if > someone does run into it on NFS they should get a strong clue about > what the problem is. Yes, just skip 9pfs early, and mention in the commit message that 9pfs has a history of being probematic with "use-after-unlink", maybe mentioning the discussion I linked above. Maybe something like this would work? diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c index 9423ad439a614..349e40d3979f2 100644 --- a/tools/testing/selftests/mm/gup_longterm.c +++ b/tools/testing/selftests/mm/gup_longterm.c @@ -47,6 +47,16 @@ static __fsword_t get_fs_type(int fd) return ret ? 0 : fs.f_type; } +static bool fs_is_problematic(__fsword_t fs_type) +{ + switch (fs_type) { + case V9FS_MAGIC: + return false; + default: + return true; + } +} + static bool fs_is_unknown(__fsword_t fs_type) { /* @@ -95,6 +105,11 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) char *mem; int ret; + if (fs_is_problematic(fs_type)) { + ksft_test_result_skip("problematic fs\n"); + return; + } + if (ftruncate(fd, size)) { ksft_test_result_fail("ftruncate() failed\n"); return; I am not 100% sure if V9FS_MAGIC is what we should be using? "man fstatfs" lists most magic values.
On Thu, 6 Mar 2025 at 15:40, David Hildenbrand <david@redhat.com> wrote: > Yes, just skip 9pfs early, and mention in the commit message that 9pfs > has a history of being probematic with "use-after-unlink", maybe > mentioning the discussion I linked above. > > Maybe something like this would work? > > diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c > index 9423ad439a614..349e40d3979f2 100644 > --- a/tools/testing/selftests/mm/gup_longterm.c > +++ b/tools/testing/selftests/mm/gup_longterm.c > @@ -47,6 +47,16 @@ static __fsword_t get_fs_type(int fd) > return ret ? 0 : fs.f_type;q > } > > +static bool fs_is_problematic(__fsword_t fs_type) > +{ > + switch (fs_type) { > + case V9FS_MAGIC: > + return false; > + default: > + return true; > + } > +} Ugh, some fun discoveries. 1. fstatfs() seems to have the same bug as ftruncate() i.e. it doesn't work on unlinked files on 9pfs. This can be worked around by calling it on the parent directory, but... 2. 9pfs seems to pass the f_type through from the host. So you can't detect it this way anyway. [3. I guess overlayfs & friends would also be an issue here although that doesn't affect my usecase.] Anyway, I think we would have to scrape /proc/mounts to do this :( I think the proper way to deal with this is something like what I've described here[0]. I.e. have a central facility as part of kselftest to detect relevant characteristics of the platform. This logic could be written in a proper programming language or in Bash, then the relevant info could be passed in via the environment or whatever (e.g. export KSFT_SYSENV_cwd_ftruncate_unlinked_works=1). [0] https://lore.kernel.org/all/Z8WJEsEAwUPeMkqy@google.com/ But, to find an immediate way to get these tests working, I think we are stuck with just peeking at errno and guessing for the time being.
On 11.03.25 14:00, Brendan Jackman wrote: > On Thu, 6 Mar 2025 at 15:40, David Hildenbrand <david@redhat.com> wrote: >> Yes, just skip 9pfs early, and mention in the commit message that 9pfs >> has a history of being probematic with "use-after-unlink", maybe >> mentioning the discussion I linked above. >> >> Maybe something like this would work? >> >> diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c >> index 9423ad439a614..349e40d3979f2 100644 >> --- a/tools/testing/selftests/mm/gup_longterm.c >> +++ b/tools/testing/selftests/mm/gup_longterm.c >> @@ -47,6 +47,16 @@ static __fsword_t get_fs_type(int fd) >> return ret ? 0 : fs.f_type;q >> } >> >> +static bool fs_is_problematic(__fsword_t fs_type) >> +{ >> + switch (fs_type) { >> + case V9FS_MAGIC: >> + return false; >> + default: >> + return true; >> + } >> +} > > Ugh, some fun discoveries. > > 1. fstatfs() seems to have the same bug as ftruncate() i.e. it doesn't > work on unlinked files on 9pfs. This can be worked around by calling > it on the parent directory, but... oO what a piece of bad software :( > > 2. 9pfs seems to pass the f_type through from the host. So you can't > detect it this way anyway. > > [3. I guess overlayfs & friends would also be an issue here although > that doesn't affect my usecase.] > > Anyway, I think we would have to scrape /proc/mounts to do this :( > The question I am asking myself: is this a 9pfs design bug or is it a 9pfs hypervisor bug. Because we shouldn't try too hard to work around hypervisor bugs. Which 9pfs implementation are you using in the hypervisor? > I think the proper way to deal with this is something like what I've > described here[0]. I.e. have a central facility as part of kselftest > to detect relevant characteristics of the platform. This logic could > be written in a proper programming language or in Bash, then the > relevant info could be passed in via the environment or whatever (e.g. > export KSFT_SYSENV_cwd_ftruncate_unlinked_works=1). > > [0] https://lore.kernel.org/all/Z8WJEsEAwUPeMkqy@google.com/ > > But, to find an immediate way to get these tests working, I think we > are stuck with just peeking at errno and guessing for the time being.
diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c index 879e9e4e8cce8127656fabe098abf7db5f6c5e23..494ec4102111b9c96fb4947b29c184735ceb8e1c 100644 --- a/tools/testing/selftests/mm/gup_longterm.c +++ b/tools/testing/selftests/mm/gup_longterm.c @@ -96,7 +96,15 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) int ret; if (ftruncate(fd, size)) { - ksft_test_result_fail("ftruncate() failed (%s)\n", strerror(errno)); + if (errno == ENOENT) { + /* + * This can happen if the file has been unlinked and the + * filesystem doesn't support truncating unlinked files. + */ + ksft_test_result_skip("ftruncate() failed with ENOENT\n"); + } else { + ksft_test_result_fail("ftruncate() failed (%s)\n", strerror(errno)); + } return; }
Some filesystems don't support funtract()ing unlinked files. They return ENOENT. In that case, skip the test. Signed-off-by: Brendan Jackman <jackmanb@google.com> --- tools/testing/selftests/mm/gup_longterm.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)