diff mbox series

[v3,08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

Message ID 20250228-mm-selftests-v3-8-958e3b6f0203@google.com
State New
Headers show
Series selftests/mm: Some cleanups from trying to run them | expand

Commit Message

Brendan Jackman Feb. 28, 2025, 4:54 p.m. UTC
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(-)

Comments

David Hildenbrand March 14, 2025, 12:10 p.m. UTC | #1
On 12.03.25 09:34, Brendan Jackman wrote:
> On Tue, Mar 11, 2025 at 08:53:02PM +0100, David Hildenbrand wrote:
>>> 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'm using QEMU via virtme-ng. IIUC virtme-ng knows how to use viortfs
> for the rootfs, but for individually-mounted directories with
> --rwdir/--rodir it uses 9pfs unconditionally.

Ah okay, that makes sense.

> 
> Even if it's a bug in QEMU, I think it is worth working around this
> one way or another. QEMU by far the most practical way to run these
> tests, and virtme-ng is probably the most popular/practical way to do
> that.

I'm afraid yes. Although allocating temp files form 9pfs is rather ... 
weird. :) One would assume that /tmp is usually backed by tmpfs. But 
well, a disto can do what it wants.

> I think even if we are confident it's just a bunch of broken
> code that isn't even in Linux, it's pragmatic to spend a certain
> amount of energy on having green tests there.
> 

Yeah, we're trying ...

> (Also, this f_type thing might be totally intentional specified
> filesystem behaviour, I don't know).

I assume it's broken in various ways to mimic that you are a file system 
which you are not.

Your approach is likely the easiest approach to deal with this 9pfs crap.

Can you document in the code+description better what we learned, and why 
we cannot even trust f_type with crappy 9pfs?

---
Cheers,

David / dhildenb
David Hildenbrand March 14, 2025, 12:17 p.m. UTC | #2
On 14.03.25 13:10, David Hildenbrand wrote:
> On 12.03.25 09:34, Brendan Jackman wrote:
>> On Tue, Mar 11, 2025 at 08:53:02PM +0100, David Hildenbrand wrote:
>>>> 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'm using QEMU via virtme-ng. IIUC virtme-ng knows how to use viortfs
>> for the rootfs, but for individually-mounted directories with
>> --rwdir/--rodir it uses 9pfs unconditionally.
> 
> Ah okay, that makes sense.
> 
>>
>> Even if it's a bug in QEMU, I think it is worth working around this
>> one way or another. QEMU by far the most practical way to run these
>> tests, and virtme-ng is probably the most popular/practical way to do
>> that.
> 
> I'm afraid yes. Although allocating temp files form 9pfs is rather ...
> weird. :) One would assume that /tmp is usually backed by tmpfs. But
> well, a disto can do what it wants.
> 
>> I think even if we are confident it's just a bunch of broken
>> code that isn't even in Linux, it's pragmatic to spend a certain
>> amount of energy on having green tests there.
>>
> 
> Yeah, we're trying ...
> 
>> (Also, this f_type thing might be totally intentional specified
>> filesystem behaviour, I don't know).
> 
> I assume it's broken in various ways to mimic that you are a file system
> which you are not.
> 
> Your approach is likely the easiest approach to deal with this 9pfs crap.
> 
> Can you document in the code+description better what we learned, and why
> we cannot even trust f_type with crappy 9pfs?

Staring a bit at that code, it's mostly 9p specific I think.

t14s: ~/git/linux s390x-file-thp2 $ git grep "= NFS_SUPER_MAGIC"
fs/nfs/super.c: buf->f_type = NFS_SUPER_MAGIC;
fs/nfs/super.c: sb->s_magic = NFS_SUPER_MAGIC;

t14s: ~/git/linux s390x-file-thp2 $ git grep "= V9FS_MAGIC"
fs/9p/vfs_super.c:      sb->s_magic = V9FS_MAGIC;
  $ git grep "f_type" | grep 9p
fs/9p/vfs_super.c:                      buf->f_type = rs.type;
Brendan Jackman March 14, 2025, 3:56 p.m. UTC | #3
> > Even if it's a bug in QEMU, I think it is worth working around this
> > one way or another. QEMU by far the most practical way to run these
> > tests, and virtme-ng is probably the most popular/practical way to do
> > that.
> 
> I'm afraid yes. Although allocating temp files form 9pfs is rather ...
> weird. :) One would assume that /tmp is usually backed by tmpfs. But well, a
> disto can do what it wants.

Ah yeah but these tests also use mkstemp() in the CWD i.e. the
location of run_vmtests.sh, it isn't /tmp that is causing this at the
moment. (At some point I thought I was hitting the issue there too,
but I think I was mistaken, like just not reading the test logs
properly or something).

> > I think even if we are confident it's just a bunch of broken
> > code that isn't even in Linux, it's pragmatic to spend a certain
> > amount of energy on having green tests there.
> > 
> 
> Yeah, we're trying ...
> 
> > (Also, this f_type thing might be totally intentional specified
> > filesystem behaviour, I don't know).
> 
> I assume it's broken in various ways to mimic that you are a file system
> which you are not.
> 
> Your approach is likely the easiest approach to deal with this 9pfs crap.
> 
> Can you document in the code+description better what we learned, and why we
> cannot even trust f_type with crappy 9pfs?

Sure, I will be more verbose about it.

I've already sent v4 here:

https://lore.kernel.org/all/20250311-mm-selftests-v4-7-dec210a658f5@google.com/

So I will wait and see if there are any comments on the v4, if there
are I'll spin the extra commentary into v5 otherwise send it as a
followup, does that sound OK?
David Hildenbrand March 14, 2025, 9:19 p.m. UTC | #4
On 14.03.25 16:56, Brendan Jackman wrote:
>>> Even if it's a bug in QEMU, I think it is worth working around this
>>> one way or another. QEMU by far the most practical way to run these
>>> tests, and virtme-ng is probably the most popular/practical way to do
>>> that.
>>
>> I'm afraid yes. Although allocating temp files form 9pfs is rather ...
>> weird. :) One would assume that /tmp is usually backed by tmpfs. But well, a
>> disto can do what it wants.
> 
> Ah yeah but these tests also use mkstemp() in the CWD i.e. the
> location of run_vmtests.sh, it isn't /tmp that is causing this at the
> moment. (At some point I thought I was hitting the issue there too,
> but I think I was mistaken, like just not reading the test logs
> properly or something).

Ah, yes run_with_local_tmpfile() ... jep, I wrote that test, now my 
memory comes back; we wanted to test with actual filesystems (e.g., 
ext4, xfs) easily.

> 
>>> I think even if we are confident it's just a bunch of broken
>>> code that isn't even in Linux, it's pragmatic to spend a certain
>>> amount of energy on having green tests there.
>>>
>>
>> Yeah, we're trying ...
>>
>>> (Also, this f_type thing might be totally intentional specified
>>> filesystem behaviour, I don't know).
>>
>> I assume it's broken in various ways to mimic that you are a file system
>> which you are not.
>>
>> Your approach is likely the easiest approach to deal with this 9pfs crap.
>>
>> Can you document in the code+description better what we learned, and why we
>> cannot even trust f_type with crappy 9pfs?
> 
> Sure, I will be more verbose about it.
> 
> I've already sent v4 here:
> 
> https://lore.kernel.org/all/20250311-mm-selftests-v4-7-dec210a658f5@google.com/
> 
> So I will wait and see if there are any comments on the v4, if there
> are I'll spin the extra commentary into v5 otherwise send it as a
> followup, does that sound OK?


You can just ask Andrew to fixup the comment or description in a reply 
to the v4 patch. Andrew will let you know if he prefers a resend.

Thanks!
diff mbox series

Patch

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