| Message ID | 20240401150015.301640-1-samasth.norway.ananda@oracle.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mremap06: fallocate is not supported on nfsv3 | expand |
Hi! > The function fallocate() is not supported on nfsv3. Thus when we run the > mremap06 test over a nfsv3 filesystem the test fails. Can we rather than this just skip the test on nfsv3? If we want to skip the test on nfs in generall we can just set .skip_filesystems = {"nfs", NULL} in the tst_test structure. I'm not sure if we can easily detect the nfs version. The test library does that by checking the NFS_SUPER_MAGIC againts stat, but there is a single SUPER_MAGIC for all nfs versions. So if we want to keep the test enabled for nfsv4 we can exit with TCONF when the call fails only when we are on NFS with tst_fs_type() and check that against TST_NFS_MAGIC. > Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> > --- > testcases/kernel/syscalls/mremap/mremap06.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/mremap/mremap06.c b/testcases/kernel/syscalls/mremap/mremap06.c > index 3bbaf441a..362b03e19 100644 > --- a/testcases/kernel/syscalls/mremap/mremap06.c > +++ b/testcases/kernel/syscalls/mremap/mremap06.c > @@ -104,8 +104,14 @@ static void setup(void) > fd = SAFE_OPEN("testfile", O_CREAT | O_RDWR | O_TRUNC, 0600); > > ret = fallocate(fd, 0, 0, mmap_size); > - if (ret == -1) > + if (ret != 0) { > + if (errno == EOPNOTSUPP || errno == ENOSYS) { > + tst_brk(TCONF, > + "fallocate system call is not implemented"); > + } > tst_brk(TBROK, "fallocate() failed"); > + return; The return shouldn't be here, tst_brk() does not return. > + } > > buf = SAFE_MMAP(0, mmap_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0); > > -- > 2.43.0 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On 4/5/24 5:01 AM, Cyril Hrubis wrote: > Hi! >> The function fallocate() is not supported on nfsv3. Thus when we run the >> mremap06 test over a nfsv3 filesystem the test fails. > > Can we rather than this just skip the test on nfsv3? > > If we want to skip the test on nfs in generall we can just set > .skip_filesystems = {"nfs", NULL} in the tst_test structure. > > I'm not sure if we can easily detect the nfs version. The test library > does that by checking the NFS_SUPER_MAGIC againts stat, but there is a > single SUPER_MAGIC for all nfs versions. > > So if we want to keep the test enabled for nfsv4 we can exit with TCONF > when the call fails only when we are on NFS with tst_fs_type() and check > that against TST_NFS_MAGIC. > Hi Cyril, Wanted to mention that fallocate() is only supported over NFSv4.2 in particular. it's not supported over NFSv3, v4.0 or v4.1. I could even see on the fallocate ltp tests that it is handled the same manner. Thanks, Samasth. >> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> >> --- >> testcases/kernel/syscalls/mremap/mremap06.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/testcases/kernel/syscalls/mremap/mremap06.c b/testcases/kernel/syscalls/mremap/mremap06.c >> index 3bbaf441a..362b03e19 100644 >> --- a/testcases/kernel/syscalls/mremap/mremap06.c >> +++ b/testcases/kernel/syscalls/mremap/mremap06.c >> @@ -104,8 +104,14 @@ static void setup(void) >> fd = SAFE_OPEN("testfile", O_CREAT | O_RDWR | O_TRUNC, 0600); >> >> ret = fallocate(fd, 0, 0, mmap_size); >> - if (ret == -1) >> + if (ret != 0) { >> + if (errno == EOPNOTSUPP || errno == ENOSYS) { >> + tst_brk(TCONF, >> + "fallocate system call is not implemented"); >> + } >> tst_brk(TBROK, "fallocate() failed"); >> + return; > > The return shouldn't be here, tst_brk() does not return. > >> + } >> >> buf = SAFE_MMAP(0, mmap_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0); >> >> -- >> 2.43.0 >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp >
Hi all, > On 4/5/24 5:01 AM, Cyril Hrubis wrote: > > Hi! > > > The function fallocate() is not supported on nfsv3. Thus when we run the > > > mremap06 test over a nfsv3 filesystem the test fails. > > Can we rather than this just skip the test on nfsv3? > > If we want to skip the test on nfs in generall we can just set > > .skip_filesystems = {"nfs", NULL} in the tst_test structure. > > I'm not sure if we can easily detect the nfs version. The test library > > does that by checking the NFS_SUPER_MAGIC againts stat, but there is a > > single SUPER_MAGIC for all nfs versions. Yeah, include/uapi/linux/magic.h in kernel contains only single magic. BTW I wonder if magic is stored to the disk or there is some other complication which prevent to have more magic values which would allow to distinguish version. I wonder if it makes sense to add support to detect NFS version (read vers parameter in /proc/mounts). The only reason why to do that would be to make sure NFS v4.2 did not lost fallocate support by bug/regression. If we don't care, checking errno is better option (will reflect kernel changes without a need to adapt LTP test). > > So if we want to keep the test enabled for nfsv4 we can exit with TCONF > > when the call fails only when we are on NFS with tst_fs_type() and check > > that against TST_NFS_MAGIC. > Hi Cyril, > Wanted to mention that fallocate() is only supported over NFSv4.2 in > particular. it's not supported over NFSv3, v4.0 or v4.1. > I could even see on the fallocate ltp tests that it is handled the same > manner. Yeah, these tests have it: testcases/kernel/mem/hugetlb/hugefallocate/hugefallocate01.c testcases/kernel/mem/hugetlb/hugefallocate/hugefallocate02.c testcases/kernel/syscalls/fallocate/fallocate01.c testcases/kernel/syscalls/fallocate/fallocate02.c testcases/kernel/syscalls/fallocate/fallocate03.c testcases/kernel/syscalls/fallocate/fallocate04.c I guess we should write SAFE_FALLOCATE(). @Samasth any change you would write it (as a separate effort). > Thanks, > Samasth. > > > Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> > > > --- > > > testcases/kernel/syscalls/mremap/mremap06.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > diff --git a/testcases/kernel/syscalls/mremap/mremap06.c b/testcases/kernel/syscalls/mremap/mremap06.c > > > index 3bbaf441a..362b03e19 100644 > > > --- a/testcases/kernel/syscalls/mremap/mremap06.c > > > +++ b/testcases/kernel/syscalls/mremap/mremap06.c > > > @@ -104,8 +104,14 @@ static void setup(void) > > > fd = SAFE_OPEN("testfile", O_CREAT | O_RDWR | O_TRUNC, 0600); > > > ret = fallocate(fd, 0, 0, mmap_size); > > > - if (ret == -1) > > > + if (ret != 0) { > > > + if (errno == EOPNOTSUPP || errno == ENOSYS) { > > > + tst_brk(TCONF, > > > + "fallocate system call is not implemented"); > > > + } > > > tst_brk(TBROK, "fallocate() failed"); > > > + return; I suggest to merge this now, without return (it can be removed before merge). Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
Hi! > > > > - if (ret == -1) > > > > + if (ret != 0) { > > > > + if (errno == EOPNOTSUPP || errno == ENOSYS) { > > > > + tst_brk(TCONF, > > > > + "fallocate system call is not implemented"); > > > > + } > > > > tst_brk(TBROK, "fallocate() failed"); > > > > + return; > > I suggest to merge this now, without return (it can be removed before merge). > Reviewed-by: Petr Vorel <pvorel@suse.cz> Shouldn't we limit this only to nfs, to make sure that fallocate() is not accidentally disabled on other filesystems? At least that what I suggested when I replied to the patch.
> Hi! > > > > > - if (ret == -1) > > > > > + if (ret != 0) { > > > > > + if (errno == EOPNOTSUPP || errno == ENOSYS) { > > > > > + tst_brk(TCONF, > > > > > + "fallocate system call is not implemented"); > > > > > + } > > > > > tst_brk(TBROK, "fallocate() failed"); > > > > > + return; > > I suggest to merge this now, without return (it can be removed before merge). > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > Shouldn't we limit this only to nfs, to make sure that fallocate() is > not accidentally disabled on other filesystems? At least that what I > suggested when I replied to the patch. That's what I also meant by my earlier suggestion (or, better I was not sure myself if it's needed). I would vote for this check. But since we already have this problem in other files, I would be ok to merge this first and handle proper fix as a separate approach if Samasth does not have time to solve it himself. @Samasth WDYT? The solution would be to at least check for tst_fs_type(".") == TST_NFS_MAGIC. Perfect solution would IMHO be to check for NFS version (e.g. in /proc/mounts) and TCONF only on NFSv3. BTW When I first read man fallocate(2), I interpreted following note related to FALLOC_FL_PUNCH_HOLE about general fallocate() support: Not all filesystems support FALLOC_FL_PUNCH_HOLE; if a filesystem doesn't support the operation, an error is returned. The operation is supported on at least the following filesystems: • XFS (since Linux 2.6.38) • ext4 (since Linux 3.0) • Btrfs (since Linux 3.7) • tmpfs(5) (since Linux 3.5) • gfs2(5) (since Linux 4.16) Some of the tests which are using fallocate() indeed test FALLOC_FL_PUNCH_HOLE: $ git grep -l FALLOC_FL_PUNCH_HOLE testcases/ testcases/kernel/mem/hugetlb/hugefallocate/hugefallocate01.c testcases/kernel/mem/hugetlb/hugefallocate/hugefallocate02.c testcases/kernel/syscalls/fallocate/fallocate04.c testcases/kernel/syscalls/fallocate/fallocate05.c testcases/kernel/syscalls/fallocate/fallocate06.c testcases/kernel/syscalls/memfd_create/memfd_create_common.c and others not, e.g.: testcases/kernel/syscalls/fallocate/fallocate0[1-3].c testcases/kernel/syscalls/mremap/mremap06.c (which does not check errno at all, likely not needed). Therefore also check for FALLOC_FL_PUNCH_HOLE would be needed (in addition to all NFS or NFSv3). BTW the man page does not mention NFS at all. @Samasth would you please send a patch to linux-man ML? Kind regards, Petr
Hi, ... > > Shouldn't we limit this only to nfs, to make sure that fallocate() is > > not accidentally disabled on other filesystems? At least that what I > > suggested when I replied to the patch. > That's what I also meant by my earlier suggestion (or, better I was not sure > myself if it's needed). I would vote for this check. But since we already have > this problem in other files, I would be ok to merge this first and handle > proper fix as a separate approach if Samasth does not have time to solve it > himself. @Samasth WDYT? > The solution would be to at least check for tst_fs_type(".") == TST_NFS_MAGIC. > Perfect solution would IMHO be to check for NFS version (e.g. in /proc/mounts) > and TCONF only on NFSv3. > BTW When I first read man fallocate(2), I interpreted following note related to > FALLOC_FL_PUNCH_HOLE about general fallocate() support: > Not all filesystems support FALLOC_FL_PUNCH_HOLE; if a filesystem doesn't > support the operation, an error is returned. The operation is supported on at > least the following filesystems: > • XFS (since Linux 2.6.38) > • ext4 (since Linux 3.0) > • Btrfs (since Linux 3.7) > • tmpfs(5) (since Linux 3.5) > • gfs2(5) (since Linux 4.16) Actually, I correct myself. Very proper check would be should TCONF only on all filesystems we test (fs_type_whitelist[]) + these listed here from particular version. i.e. not only NFSv3 is affected. But given it works on NFSv4*, it'd be good to check the current support. Or we might just ignore the TCONF check, as most of the filesystems got support in kernel 3.x and the oldest (gfs2) is 4.16. Kind regards, Petr > Some of the tests which are using fallocate() indeed test FALLOC_FL_PUNCH_HOLE: > $ git grep -l FALLOC_FL_PUNCH_HOLE testcases/ > testcases/kernel/mem/hugetlb/hugefallocate/hugefallocate01.c > testcases/kernel/mem/hugetlb/hugefallocate/hugefallocate02.c > testcases/kernel/syscalls/fallocate/fallocate04.c > testcases/kernel/syscalls/fallocate/fallocate05.c > testcases/kernel/syscalls/fallocate/fallocate06.c > testcases/kernel/syscalls/memfd_create/memfd_create_common.c > and others not, e.g.: > testcases/kernel/syscalls/fallocate/fallocate0[1-3].c > testcases/kernel/syscalls/mremap/mremap06.c (which does not check errno at all, > likely not needed). > Therefore also check for FALLOC_FL_PUNCH_HOLE would be needed (in addition to > all NFS or NFSv3). > BTW the man page does not mention NFS at all. @Samasth would you please send a > patch to linux-man ML? > Kind regards, > Petr
Hi! > The solution would be to at least check for tst_fs_type(".") == TST_NFS_MAGIC. I would go for this at the moment, that is the easiest fix at the moment. > Perfect solution would IMHO be to check for NFS version (e.g. in /proc/mounts) > and TCONF only on NFSv3. And is nfsv4 enough, wasn't it added in nfsv4.2? Do we need to parse /proc/net/rpc/nfs to figure out if nfs fallocate() is supported?
> Hi! > > The solution would be to at least check for tst_fs_type(".") == TST_NFS_MAGIC. > I would go for this at the moment, that is the easiest fix at the > moment. So you mean we risk we lost fallocate support in NFSv4.2 for the simplicity. OK for me. > > Perfect solution would IMHO be to check for NFS version (e.g. in /proc/mounts) > > and TCONF only on NFSv3. > And is nfsv4 enough, wasn't it added in nfsv4.2? Do we need to parse > /proc/net/rpc/nfs to figure out if nfs fallocate() is supported? Yes. First, I got confused 95d871f03cae ("nfsd: Add ALLOCATE support") [1] which is not mentioning NFS version, because it's for nfsd. But looking other commits, e.g. 95d871f03cae ("nfsd: Add ALLOCATE support") [2], 624bd5b7b683 ("nfs: Add DEALLOCATE support") [3] and the docs from merge commit in 3.19 [4] and kernelnewbies [5], it's clear: it's for NFSv4.2. BTW is that whole fallocate() support? [4] mentions "NFSv4.2 client support for hole punching and preallocation.", commit [3]: "This patch adds support for using the NFS v4.2 operation DEALLOCATE to punch holes in a file.", commit b0cb9085239a ("nfsd: Add DEALLOCATE support") [6] uses FALLOC_FL_PUNCH_HOLE flag. All that IMHO means it's adding just FALLOC_FL_PUNCH_HOLE support into NFSv4.2. Thus other (basic) fallocate() might work in all NFS versions, right? That would suggest when addin safe_fallocate() we should check for NFS (tst_fs_type(".") == TST_NFS_MAGIC) only when FALLOC_FL_PUNCH_HOLE is used. (I got from your answer we ignore missing support for older filesystems, we can always add the check once somebody reports invalid error). Kind regards, Petr [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=95d871f03cae6b49de040265cf88cbe2a16b9f05 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f4ac1674f5da420ef17896f0f222c5215ebcde80 [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=624bd5b7b683c978c6d5f4e9f6142cfb3470983d [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e20db597b6264de55ea6636fc79b1e4aaa89d129 [5] https://kernelnewbies.org/Linux_3.19#NFSv4.2_support_for_hole_punching_and_preallocation [6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0cb9085239a20b7482ddd4839dd1d5476801dfa
On 4/10/24 6:33 AM, Petr Vorel wrote: >> Hi! >>> The solution would be to at least check for tst_fs_type(".") == TST_NFS_MAGIC. > >> I would go for this at the moment, that is the easiest fix at the >> moment. > > So you mean we risk we lost fallocate support in NFSv4.2 for the simplicity. OK > for me. > >>> Perfect solution would IMHO be to check for NFS version (e.g. in /proc/mounts) >>> and TCONF only on NFSv3. > >> And is nfsv4 enough, wasn't it added in nfsv4.2? Do we need to parse >> /proc/net/rpc/nfs to figure out if nfs fallocate() is supported? > > Yes. First, I got confused 95d871f03cae ("nfsd: Add ALLOCATE support") [1] which is > not mentioning NFS version, because it's for nfsd. But looking other commits, > e.g. 95d871f03cae ("nfsd: Add ALLOCATE support") [2], 624bd5b7b683 ("nfs: Add > DEALLOCATE support") [3] and the docs from merge commit in 3.19 [4] and > kernelnewbies [5], it's clear: it's for NFSv4.2. > > BTW is that whole fallocate() support? [4] mentions "NFSv4.2 client support for > hole punching and preallocation.", commit [3]: "This patch adds support for > using the NFS v4.2 operation DEALLOCATE to punch holes in a file.", commit > b0cb9085239a ("nfsd: Add DEALLOCATE support") [6] uses FALLOC_FL_PUNCH_HOLE > flag. All that IMHO means it's adding just FALLOC_FL_PUNCH_HOLE support into > NFSv4.2. Thus other (basic) fallocate() might work in all NFS versions, right? > > That would suggest when addin safe_fallocate() we should check for NFS > (tst_fs_type(".") == TST_NFS_MAGIC) only when FALLOC_FL_PUNCH_HOLE is used. > (I got from your answer we ignore missing support for older filesystems, > we can always add the check once somebody reports invalid error). Thanks for the detailed explanation of the issue. I will send out a v2 patch which would check the file system using tst_fs_type(".") before printing TCONF. I will also send a patch to update the man page to include the information about nfs. Thanks, Samasth. > > Kind regards, > Petr > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=95d871f03cae6b49de040265cf88cbe2a16b9f05 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f4ac1674f5da420ef17896f0f222c5215ebcde80 > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=624bd5b7b683c978c6d5f4e9f6142cfb3470983d > [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e20db597b6264de55ea6636fc79b1e4aaa89d129 > [5] https://kernelnewbies.org/Linux_3.19#NFSv4.2_support_for_hole_punching_and_preallocation > [6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b0cb9085239a20b7482ddd4839dd1d5476801dfa
diff --git a/testcases/kernel/syscalls/mremap/mremap06.c b/testcases/kernel/syscalls/mremap/mremap06.c index 3bbaf441a..362b03e19 100644 --- a/testcases/kernel/syscalls/mremap/mremap06.c +++ b/testcases/kernel/syscalls/mremap/mremap06.c @@ -104,8 +104,14 @@ static void setup(void) fd = SAFE_OPEN("testfile", O_CREAT | O_RDWR | O_TRUNC, 0600); ret = fallocate(fd, 0, 0, mmap_size); - if (ret == -1) + if (ret != 0) { + if (errno == EOPNOTSUPP || errno == ENOSYS) { + tst_brk(TCONF, + "fallocate system call is not implemented"); + } tst_brk(TBROK, "fallocate() failed"); + return; + } buf = SAFE_MMAP(0, mmap_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
The function fallocate() is not supported on nfsv3. Thus when we run the mremap06 test over a nfsv3 filesystem the test fails. Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> --- testcases/kernel/syscalls/mremap/mremap06.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)