| Message ID | 20240119144322.41928-1-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] swapon01: Test on all filesystems | expand |
Hi! > testcases/kernel/syscalls/swapon/swapon01.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/testcases/kernel/syscalls/swapon/swapon01.c b/testcases/kernel/syscalls/swapon/swapon01.c > index e59fb20a1..ef4ebfdcf 100644 > --- a/testcases/kernel/syscalls/swapon/swapon01.c > +++ b/testcases/kernel/syscalls/swapon/swapon01.c > @@ -8,6 +8,7 @@ > * [Description] > * > * Checks that swapon() succeds with swapfile. > + * Testing on all filesystems which support swap file. > */ > > #include <unistd.h> > @@ -17,13 +18,14 @@ > #include "lapi/syscalls.h" > #include "libswap.h" > > -#define SWAP_FILE "swapfile01" > +#define MNTPOINT "mntpoint" > +#define SWAP_FILE MNTPOINT"/swapfile01" > > static void verify_swapon(void) > { > TST_EXP_PASS(tst_syscall(__NR_swapon, SWAP_FILE, 0)); > > - if (TST_PASS && tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { I do not think that this should be removed, if the swapon() call fails we still do not want TBROK here. > tst_brk(TBROK | TERRNO, > "Failed to turn off swapfile, system reboot recommended"); > } > @@ -31,13 +33,20 @@ static void verify_swapon(void) > > static void setup(void) > { > - is_swap_supported(SWAP_FILE); > make_swapfile(SWAP_FILE, 0); > } > > static struct tst_test test = { > - .needs_root = 1, > - .needs_tmpdir = 1, > + .mntpoint = MNTPOINT, > + .mount_device = 1, > + .needs_root = 1, /* for swapon() */ Can we please avoid comments like this? I'ts pretty clear that if you are working with devices and setting up swapfiles it's priviledged operation... > + .all_filesystems = 1, > .test_all = verify_swapon, > - .setup = setup > + .setup = setup, > + .skip_filesystems = (const char *[]) { > + "bcachefs", > + "btrfs", > + "tmpfs", So the rest of the filesystems work fine? If so this change looks fine to me. > + NULL > + }, > };
Hi Petr, Cyril, On Fri, Jan 19, 2024 at 10:43 PM Petr Vorel <pvorel@suse.cz> wrote: > Test on all filesystems to increase coverage. > Skip filesystems which does not support swap (currently bcachefs, btrfs > and tmpfs). > > Tested on 5.10, 6.6 and 6.7. > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Hi Li, Cyril, > > feel free to postpone this after the release. > If we are for adding this, I'll retest on Monday older filesystems. > > man swapon(8) mentions: > > Btrfs > Swap files on Btrfs are supported since Linux 5.0 on files with > nocow > attribute. See the btrfs(5) manual page for more details. > > => Can we pass "nocow" just to btrfs? Or should it be added to single > test, which uses just btrfs with nocow? > > NFS > Swap over NFS may not work. > > => I guess this is not important for us, as NFS is not part of > filesystems in .all_filesystems, but maybe we should still add it to the > whitelist? > > Kind regards, > Petr > > testcases/kernel/syscalls/swapon/swapon01.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/testcases/kernel/syscalls/swapon/swapon01.c > b/testcases/kernel/syscalls/swapon/swapon01.c > index e59fb20a1..ef4ebfdcf 100644 > --- a/testcases/kernel/syscalls/swapon/swapon01.c > +++ b/testcases/kernel/syscalls/swapon/swapon01.c > @@ -8,6 +8,7 @@ > * [Description] > * > * Checks that swapon() succeds with swapfile. > + * Testing on all filesystems which support swap file. > */ > > #include <unistd.h> > @@ -17,13 +18,14 @@ > #include "lapi/syscalls.h" > #include "libswap.h" > > -#define SWAP_FILE "swapfile01" > +#define MNTPOINT "mntpoint" > +#define SWAP_FILE MNTPOINT"/swapfile01" > > static void verify_swapon(void) > { > TST_EXP_PASS(tst_syscall(__NR_swapon, SWAP_FILE, 0)); > > - if (TST_PASS && tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > tst_brk(TBROK | TERRNO, > "Failed to turn off swapfile, system > reboot recommended"); > } > @@ -31,13 +33,20 @@ static void verify_swapon(void) > > static void setup(void) > { > - is_swap_supported(SWAP_FILE); > make_swapfile(SWAP_FILE, 0); > } > > static struct tst_test test = { > - .needs_root = 1, > - .needs_tmpdir = 1, > + .mntpoint = MNTPOINT, > + .mount_device = 1, > + .needs_root = 1, /* for swapon() */ > + .all_filesystems = 1, > .test_all = verify_swapon, > - .setup = setup > + .setup = setup, > + .skip_filesystems = (const char *[]) { > + "bcachefs", > + "btrfs", > + "tmpfs", > + NULL > + }, > IMHO, I hate to add the skip_filesystem in this test, because if we do, then we have to add to all of the related swap tests that, it add more tedious work to people. So, if we decide to postpone this patch to release, we can just refactor the is_swap_supported() function to make things easier. I will send an RFC patch to show something in my mind, what we need to do later is just to finalize the swap_supported_fs[] struct list.
> Hi! > > testcases/kernel/syscalls/swapon/swapon01.c | 21 +++++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/testcases/kernel/syscalls/swapon/swapon01.c b/testcases/kernel/syscalls/swapon/swapon01.c > > index e59fb20a1..ef4ebfdcf 100644 > > --- a/testcases/kernel/syscalls/swapon/swapon01.c > > +++ b/testcases/kernel/syscalls/swapon/swapon01.c > > @@ -8,6 +8,7 @@ > > * [Description] > > * > > * Checks that swapon() succeds with swapfile. > > + * Testing on all filesystems which support swap file. > > */ > > #include <unistd.h> > > @@ -17,13 +18,14 @@ > > #include "lapi/syscalls.h" > > #include "libswap.h" > > -#define SWAP_FILE "swapfile01" > > +#define MNTPOINT "mntpoint" > > +#define SWAP_FILE MNTPOINT"/swapfile01" > > static void verify_swapon(void) > > { > > TST_EXP_PASS(tst_syscall(__NR_swapon, SWAP_FILE, 0)); > > - if (TST_PASS && tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > > + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > I do not think that this should be removed, if the swapon() call fails > we still do not want TBROK here. +1, I did it by accident when rebasing the original patch. > > tst_brk(TBROK | TERRNO, > > "Failed to turn off swapfile, system reboot recommended"); > > } > > @@ -31,13 +33,20 @@ static void verify_swapon(void) > > static void setup(void) > > { > > - is_swap_supported(SWAP_FILE); > > make_swapfile(SWAP_FILE, 0); > > } > > static struct tst_test test = { > > - .needs_root = 1, > > - .needs_tmpdir = 1, > > + .mntpoint = MNTPOINT, > > + .mount_device = 1, > > + .needs_root = 1, /* for swapon() */ > Can we please avoid comments like this? Sure (again part for the original effort). > I'ts pretty clear that if you are working with devices and setting up > swapfiles it's priviledged operation... > > + .all_filesystems = 1, > > .test_all = verify_swapon, > > - .setup = setup > > + .setup = setup, > > + .skip_filesystems = (const char *[]) { > > + "bcachefs", > > + "btrfs", > > + "tmpfs", > So the rest of the filesystems work fine? If so this change looks fine > to me. Yes. Going to send v2. Kind regards, Petr > > + NULL > > + }, > > };
Hi Li, Cyril, > Hi Petr, Cyril, > On Fri, Jan 19, 2024 at 10:43 PM Petr Vorel <pvorel@suse.cz> wrote: > > Test on all filesystems to increase coverage. > > Skip filesystems which does not support swap (currently bcachefs, btrfs > > and tmpfs). > > Tested on 5.10, 6.6 and 6.7. > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > Hi Li, Cyril, > > feel free to postpone this after the release. > > If we are for adding this, I'll retest on Monday older filesystems. > > man swapon(8) mentions: > > Btrfs > > Swap files on Btrfs are supported since Linux 5.0 on files with > > nocow > > attribute. See the btrfs(5) manual page for more details. > > => Can we pass "nocow" just to btrfs? Or should it be added to single > > test, which uses just btrfs with nocow? > > NFS > > Swap over NFS may not work. > > => I guess this is not important for us, as NFS is not part of > > filesystems in .all_filesystems, but maybe we should still add it to the > > whitelist? > > Kind regards, > > Petr > > testcases/kernel/syscalls/swapon/swapon01.c | 21 +++++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/testcases/kernel/syscalls/swapon/swapon01.c > > b/testcases/kernel/syscalls/swapon/swapon01.c > > index e59fb20a1..ef4ebfdcf 100644 > > --- a/testcases/kernel/syscalls/swapon/swapon01.c > > +++ b/testcases/kernel/syscalls/swapon/swapon01.c > > @@ -8,6 +8,7 @@ > > * [Description] > > * > > * Checks that swapon() succeds with swapfile. > > + * Testing on all filesystems which support swap file. > > */ > > #include <unistd.h> > > @@ -17,13 +18,14 @@ > > #include "lapi/syscalls.h" > > #include "libswap.h" > > -#define SWAP_FILE "swapfile01" > > +#define MNTPOINT "mntpoint" > > +#define SWAP_FILE MNTPOINT"/swapfile01" > > static void verify_swapon(void) > > { > > TST_EXP_PASS(tst_syscall(__NR_swapon, SWAP_FILE, 0)); > > - if (TST_PASS && tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > > + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > > tst_brk(TBROK | TERRNO, > > "Failed to turn off swapfile, system > > reboot recommended"); > > } > > @@ -31,13 +33,20 @@ static void verify_swapon(void) > > static void setup(void) > > { > > - is_swap_supported(SWAP_FILE); > > make_swapfile(SWAP_FILE, 0); > > } > > static struct tst_test test = { > > - .needs_root = 1, > > - .needs_tmpdir = 1, > > + .mntpoint = MNTPOINT, > > + .mount_device = 1, > > + .needs_root = 1, /* for swapon() */ > > + .all_filesystems = 1, > > .test_all = verify_swapon, > > - .setup = setup > > + .setup = setup, > > + .skip_filesystems = (const char *[]) { > > + "bcachefs", > > + "btrfs", > > + "tmpfs", > > + NULL > > + }, > IMHO, I hate to add the skip_filesystem in this test, because if we do, > then we have to add to all of the related swap tests that, it add more > tedious work to people. First I thought it'd be ok to have just single test which uses all_filesystems (better one than none), but sure, having all of them would be better. > So, if we decide to postpone this patch to release, we can just refactor > the is_swap_supported() function to make things easier. I will send an RFC > patch to show something in my mind, what we need to do later is just > to finalize the swap_supported_fs[] struct list. Sure, this is not critical, it can wait if you have better solution. Thanks for spending time on it. Kind regards, Petr
Hi Petr On Mon, Jan 22, 2024 at 2:22 PM Petr Vorel <pvorel@suse.cz> wrote: > > > So the rest of the filesystems work fine? If so this change looks fine > > to me. > > Yes. > > Going to send v2. > No need, I dared to modify and combine it in the 2/4. https://lists.linux.it/pipermail/ltp/2024-January/036806.html Sorry for my arbitrary work, I just to test the patchset on my side easily.
On Mon, Jan 22, 2024 at 2:27 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, Cyril, > > > Hi Petr, Cyril, > > > On Fri, Jan 19, 2024 at 10:43 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > Test on all filesystems to increase coverage. > > > Skip filesystems which does not support swap (currently bcachefs, btrfs > > > and tmpfs). > > > > Tested on 5.10, 6.6 and 6.7. > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > --- > > > Hi Li, Cyril, > > > > feel free to postpone this after the release. > > > If we are for adding this, I'll retest on Monday older filesystems. > > > > man swapon(8) mentions: > > > > Btrfs > > > Swap files on Btrfs are supported since Linux 5.0 on files with > > > nocow > > > attribute. See the btrfs(5) manual page for more details. > > > > => Can we pass "nocow" just to btrfs? Or should it be added to single > > > test, which uses just btrfs with nocow? > > > > NFS > > > Swap over NFS may not work. > > > > => I guess this is not important for us, as NFS is not part of > > > filesystems in .all_filesystems, but maybe we should still add it to > the > > > whitelist? > > > > Kind regards, > > > Petr > > > > testcases/kernel/syscalls/swapon/swapon01.c | 21 +++++++++++++++------ > > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/testcases/kernel/syscalls/swapon/swapon01.c > > > b/testcases/kernel/syscalls/swapon/swapon01.c > > > index e59fb20a1..ef4ebfdcf 100644 > > > --- a/testcases/kernel/syscalls/swapon/swapon01.c > > > +++ b/testcases/kernel/syscalls/swapon/swapon01.c > > > @@ -8,6 +8,7 @@ > > > * [Description] > > > * > > > * Checks that swapon() succeds with swapfile. > > > + * Testing on all filesystems which support swap file. > > > */ > > > > #include <unistd.h> > > > @@ -17,13 +18,14 @@ > > > #include "lapi/syscalls.h" > > > #include "libswap.h" > > > > -#define SWAP_FILE "swapfile01" > > > +#define MNTPOINT "mntpoint" > > > +#define SWAP_FILE MNTPOINT"/swapfile01" > > > > static void verify_swapon(void) > > > { > > > TST_EXP_PASS(tst_syscall(__NR_swapon, SWAP_FILE, 0)); > > > > - if (TST_PASS && tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > > > + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > > > tst_brk(TBROK | TERRNO, > > > "Failed to turn off swapfile, system > > > reboot recommended"); > > > } > > > @@ -31,13 +33,20 @@ static void verify_swapon(void) > > > > static void setup(void) > > > { > > > - is_swap_supported(SWAP_FILE); > > > make_swapfile(SWAP_FILE, 0); > > > } > > > > static struct tst_test test = { > > > - .needs_root = 1, > > > - .needs_tmpdir = 1, > > > + .mntpoint = MNTPOINT, > > > + .mount_device = 1, > > > + .needs_root = 1, /* for swapon() */ > > > + .all_filesystems = 1, > > > .test_all = verify_swapon, > > > - .setup = setup > > > + .setup = setup, > > > + .skip_filesystems = (const char *[]) { > > > + "bcachefs", > > > + "btrfs", > > > + "tmpfs", > > > + NULL > > > + }, > > > > > IMHO, I hate to add the skip_filesystem in this test, because if we do, > > then we have to add to all of the related swap tests that, it add more > > tedious work to people. > > First I thought it'd be ok to have just single test which uses > all_filesystems > (better one than none), but sure, having all of them would be better. > > > So, if we decide to postpone this patch to release, we can just refactor > > the is_swap_supported() function to make things easier. I will send an > RFC > > patch to show something in my mind, what we need to do later is just > > to finalize the swap_supported_fs[] struct list. > > Sure, this is not critical, it can wait if you have better solution. Thanks > for spending time on it. > Yes, but it is ready there 1/4 :). https://lists.linux.it/pipermail/ltp/2024-January/036804.html The rest work is just to finalize the swap_supported_fs list. And, I agree that all of these can be postponed. Because so far it won't be a critical/urgent-fix for a new release.
Hi Li, > On Mon, Jan 22, 2024 at 2:27 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, Cyril, > > > Hi Petr, Cyril, > > > On Fri, Jan 19, 2024 at 10:43 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > Test on all filesystems to increase coverage. > > > > Skip filesystems which does not support swap (currently bcachefs, btrfs > > > > and tmpfs). > > > > Tested on 5.10, 6.6 and 6.7. > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > --- > > > > Hi Li, Cyril, > > > > feel free to postpone this after the release. > > > > If we are for adding this, I'll retest on Monday older filesystems. > > > > man swapon(8) mentions: > > > > Btrfs > > > > Swap files on Btrfs are supported since Linux 5.0 on files with > > > > nocow > > > > attribute. See the btrfs(5) manual page for more details. > > > > => Can we pass "nocow" just to btrfs? Or should it be added to single > > > > test, which uses just btrfs with nocow? > > > > NFS > > > > Swap over NFS may not work. > > > > => I guess this is not important for us, as NFS is not part of > > > > filesystems in .all_filesystems, but maybe we should still add it to > > the > > > > whitelist? > > > > Kind regards, > > > > Petr > > > > testcases/kernel/syscalls/swapon/swapon01.c | 21 +++++++++++++++------ > > > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/testcases/kernel/syscalls/swapon/swapon01.c > > > > b/testcases/kernel/syscalls/swapon/swapon01.c > > > > index e59fb20a1..ef4ebfdcf 100644 > > > > --- a/testcases/kernel/syscalls/swapon/swapon01.c > > > > +++ b/testcases/kernel/syscalls/swapon/swapon01.c > > > > @@ -8,6 +8,7 @@ > > > > * [Description] > > > > * > > > > * Checks that swapon() succeds with swapfile. > > > > + * Testing on all filesystems which support swap file. > > > > */ > > > > #include <unistd.h> > > > > @@ -17,13 +18,14 @@ > > > > #include "lapi/syscalls.h" > > > > #include "libswap.h" > > > > -#define SWAP_FILE "swapfile01" > > > > +#define MNTPOINT "mntpoint" > > > > +#define SWAP_FILE MNTPOINT"/swapfile01" > > > > static void verify_swapon(void) > > > > { > > > > TST_EXP_PASS(tst_syscall(__NR_swapon, SWAP_FILE, 0)); > > > > - if (TST_PASS && tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > > > > + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > > > > tst_brk(TBROK | TERRNO, > > > > "Failed to turn off swapfile, system > > > > reboot recommended"); > > > > } > > > > @@ -31,13 +33,20 @@ static void verify_swapon(void) > > > > static void setup(void) > > > > { > > > > - is_swap_supported(SWAP_FILE); > > > > make_swapfile(SWAP_FILE, 0); > > > > } > > > > static struct tst_test test = { > > > > - .needs_root = 1, > > > > - .needs_tmpdir = 1, > > > > + .mntpoint = MNTPOINT, > > > > + .mount_device = 1, > > > > + .needs_root = 1, /* for swapon() */ > > > > + .all_filesystems = 1, > > > > .test_all = verify_swapon, > > > > - .setup = setup > > > > + .setup = setup, > > > > + .skip_filesystems = (const char *[]) { > > > > + "bcachefs", > > > > + "btrfs", > > > > + "tmpfs", > > > > + NULL > > > > + }, > > > IMHO, I hate to add the skip_filesystem in this test, because if we do, > > > then we have to add to all of the related swap tests that, it add more > > > tedious work to people. > > First I thought it'd be ok to have just single test which uses > > all_filesystems > > (better one than none), but sure, having all of them would be better. > > > So, if we decide to postpone this patch to release, we can just refactor > > > the is_swap_supported() function to make things easier. I will send an > > RFC > > > patch to show something in my mind, what we need to do later is just > > > to finalize the swap_supported_fs[] struct list. > > Sure, this is not critical, it can wait if you have better solution. Thanks > > for spending time on it. > Yes, but it is ready there 1/4 :). > https://lists.linux.it/pipermail/ltp/2024-January/036804.html > The rest work is just to finalize the swap_supported_fs list. > And, I agree that all of these can be postponed. Because > so far it won't be a critical/urgent-fix for a new release. OTOH if you prepared this, I'm ok to review and test. I already run various tests on this. Kind regards, Petr
diff --git a/testcases/kernel/syscalls/swapon/swapon01.c b/testcases/kernel/syscalls/swapon/swapon01.c index e59fb20a1..ef4ebfdcf 100644 --- a/testcases/kernel/syscalls/swapon/swapon01.c +++ b/testcases/kernel/syscalls/swapon/swapon01.c @@ -8,6 +8,7 @@ * [Description] * * Checks that swapon() succeds with swapfile. + * Testing on all filesystems which support swap file. */ #include <unistd.h> @@ -17,13 +18,14 @@ #include "lapi/syscalls.h" #include "libswap.h" -#define SWAP_FILE "swapfile01" +#define MNTPOINT "mntpoint" +#define SWAP_FILE MNTPOINT"/swapfile01" static void verify_swapon(void) { TST_EXP_PASS(tst_syscall(__NR_swapon, SWAP_FILE, 0)); - if (TST_PASS && tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { tst_brk(TBROK | TERRNO, "Failed to turn off swapfile, system reboot recommended"); } @@ -31,13 +33,20 @@ static void verify_swapon(void) static void setup(void) { - is_swap_supported(SWAP_FILE); make_swapfile(SWAP_FILE, 0); } static struct tst_test test = { - .needs_root = 1, - .needs_tmpdir = 1, + .mntpoint = MNTPOINT, + .mount_device = 1, + .needs_root = 1, /* for swapon() */ + .all_filesystems = 1, .test_all = verify_swapon, - .setup = setup + .setup = setup, + .skip_filesystems = (const char *[]) { + "bcachefs", + "btrfs", + "tmpfs", + NULL + }, };
Test on all filesystems to increase coverage. Skip filesystems which does not support swap (currently bcachefs, btrfs and tmpfs). Tested on 5.10, 6.6 and 6.7. Signed-off-by: Petr Vorel <pvorel@suse.cz> --- Hi Li, Cyril, feel free to postpone this after the release. If we are for adding this, I'll retest on Monday older filesystems. man swapon(8) mentions: Btrfs Swap files on Btrfs are supported since Linux 5.0 on files with nocow attribute. See the btrfs(5) manual page for more details. => Can we pass "nocow" just to btrfs? Or should it be added to single test, which uses just btrfs with nocow? NFS Swap over NFS may not work. => I guess this is not important for us, as NFS is not part of filesystems in .all_filesystems, but maybe we should still add it to the whitelist? Kind regards, Petr testcases/kernel/syscalls/swapon/swapon01.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)