| Message ID | 20231011162428.583911-2-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | swapon01: Test on all filesystems, cleanup | expand |
Hi! > Test on all filesystems to increase coverage. btrfs and tmpfs > currently does not support swap file, but keep it in case this get > changed in the future. > > Testing on all filesystems usually requests root, but we don't require > it with .needs_root = 1. But using swapon() requires it as well, thus > keep it defined. > > Also detect the support on the same file as which is being tested. For the previous iteration of this patch Li wasn't sure if this adds enough value since there isn't ton of filesystem specific code involved unless we actually start writing to the swapfile. And as the patch is I would agree with that. What may make sense I think is to require certain filesystem to support swap, i.e. fail the test in the case that we haven't managed to create and enable the swapfile where it's supposed to work. That would guard about accidental breakages, as it is the test would not catch these because it woult TCONF in the setup. > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > IMHO it's not a big slowdown thus I keep btrfs and tmpfs. > They might get support one day. > > testcases/kernel/syscalls/swapon/swapon01.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/testcases/kernel/syscalls/swapon/swapon01.c b/testcases/kernel/syscalls/swapon/swapon01.c > index b5c3f359c..6b7f96cf7 100644 > --- a/testcases/kernel/syscalls/swapon/swapon01.c > +++ b/testcases/kernel/syscalls/swapon/swapon01.c > @@ -9,6 +9,9 @@ > * Checks that swapon() succeds with swapfile. > */ > > +#define MNTPOINT "mntpoint" > +#define SWAP_FILE MNTPOINT"/swapfile01" > + > #include <unistd.h> > #include <errno.h> > #include <stdlib.h> > @@ -18,14 +21,14 @@ > > static void verify_swapon(void) > { > - TEST(tst_syscall(__NR_swapon, "./swapfile01", 0)); > + TEST(tst_syscall(__NR_swapon, SWAP_FILE, 0)); > > if (TST_RET == -1) { > tst_res(TFAIL | TTERRNO, "Failed to turn on swapfile"); > } else { > tst_res(TPASS, "Succeeded to turn on swapfile"); > /*we need to turn this swap file off for -i option */ > - if (tst_syscall(__NR_swapoff, "./swapfile01") != 0) { > + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > tst_brk(TBROK | TERRNO, "Failed to turn off swapfile," > " system reboot after execution of LTP " > "test suite is recommended."); > @@ -35,13 +38,15 @@ static void verify_swapon(void) > > static void setup(void) > { > - is_swap_supported("./tstswap"); > - make_swapfile("swapfile01", 0); > + 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 > }; > -- > 2.42.0 >
On Fri, Jan 19, 2024 at 7:42 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > Test on all filesystems to increase coverage. btrfs and tmpfs > > currently does not support swap file, but keep it in case this get > > changed in the future. > > > > Testing on all filesystems usually requests root, but we don't require > > it with .needs_root = 1. But using swapon() requires it as well, thus > > keep it defined. > > > > Also detect the support on the same file as which is being tested. > > For the previous iteration of this patch Li wasn't sure if this adds > enough value since there isn't ton of filesystem specific code involved > unless we actually start writing to the swapfile. > Yes, if testing with all_filesystems, only make_swapfile process makes sense IMO. The reset swapon() operation does the same thing to the kernel. > > And as the patch is I would agree with that. What may make sense I think > is to require certain filesystem to support swap, i.e. fail the test in > the case that we haven't managed to create and enable the swapfile where > it's supposed to work. That would guard about accidental breakages, as > it is the test would not catch these because it woult TCONF in the > setup. > +1 This sounds reasonable, looks like we need a whitelist to guarantee those filesystems that support swapfile, otherwise it's easy to miss some false negatives with report TCONF by is_swap_supported(). > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > IMHO it's not a big slowdown thus I keep btrfs and tmpfs. > > They might get support one day. > > > > testcases/kernel/syscalls/swapon/swapon01.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/testcases/kernel/syscalls/swapon/swapon01.c > b/testcases/kernel/syscalls/swapon/swapon01.c > > index b5c3f359c..6b7f96cf7 100644 > > --- a/testcases/kernel/syscalls/swapon/swapon01.c > > +++ b/testcases/kernel/syscalls/swapon/swapon01.c > > @@ -9,6 +9,9 @@ > > * Checks that swapon() succeds with swapfile. > > */ > > > > +#define MNTPOINT "mntpoint" > > +#define SWAP_FILE MNTPOINT"/swapfile01" > > + > > #include <unistd.h> > > #include <errno.h> > > #include <stdlib.h> > > @@ -18,14 +21,14 @@ > > > > static void verify_swapon(void) > > { > > - TEST(tst_syscall(__NR_swapon, "./swapfile01", 0)); > > + TEST(tst_syscall(__NR_swapon, SWAP_FILE, 0)); > > > > if (TST_RET == -1) { > > tst_res(TFAIL | TTERRNO, "Failed to turn on swapfile"); > > } else { > > tst_res(TPASS, "Succeeded to turn on swapfile"); > > /*we need to turn this swap file off for -i option */ > > - if (tst_syscall(__NR_swapoff, "./swapfile01") != 0) { > > + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > > tst_brk(TBROK | TERRNO, "Failed to turn off > swapfile," > > " system reboot after execution of LTP " > > "test suite is recommended."); > > @@ -35,13 +38,15 @@ static void verify_swapon(void) > > > > static void setup(void) > > { > > - is_swap_supported("./tstswap"); > > - make_swapfile("swapfile01", 0); > > + 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 > > }; > > -- > > 2.42.0 > > > > -- > Cyril Hrubis > chrubis@suse.cz > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > >
> On Fri, Jan 19, 2024 at 7:42 PM Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > > > Test on all filesystems to increase coverage. btrfs and tmpfs > > > currently does not support swap file, but keep it in case this get > > > changed in the future. > > > Testing on all filesystems usually requests root, but we don't require > > > it with .needs_root = 1. But using swapon() requires it as well, thus > > > keep it defined. > > > Also detect the support on the same file as which is being tested. > > For the previous iteration of this patch Li wasn't sure if this adds > > enough value since there isn't ton of filesystem specific code involved > > unless we actually start writing to the swapfile. I wonder how to force. We could setup sysctl vm.swappiness=100 But how to consume enough RAM to be actually written? > Yes, if testing with all_filesystems, only make_swapfile process makes > sense IMO. > The reset swapon() operation does the same thing to the kernel. > > And as the patch is I would agree with that. What may make sense I think > > is to require certain filesystem to support swap, i.e. fail the test in > > the case that we haven't managed to create and enable the swapfile where > > it's supposed to work. That would guard about accidental breakages, as > > it is the test would not catch these because it woult TCONF in the > > setup. > +1 > This sounds reasonable, looks like we need a whitelist to guarantee > those filesystems that support swapfile, otherwise it's easy to miss > some false negatives with report TCONF by is_swap_supported(). OK, even without writing it would make sense to test on all filesystems. Replacing is_swap_supported() with a static list sounds good to me. We might endup to check kernel release for certain filesystem (if it got swap support later). Going to send a patch. Kind regards, Petr > > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > --- > > > IMHO it's not a big slowdown thus I keep btrfs and tmpfs. > > > They might get support one day. > > > testcases/kernel/syscalls/swapon/swapon01.c | 17 +++++++++++------ > > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > diff --git a/testcases/kernel/syscalls/swapon/swapon01.c > > b/testcases/kernel/syscalls/swapon/swapon01.c > > > index b5c3f359c..6b7f96cf7 100644 > > > --- a/testcases/kernel/syscalls/swapon/swapon01.c > > > +++ b/testcases/kernel/syscalls/swapon/swapon01.c > > > @@ -9,6 +9,9 @@ > > > * Checks that swapon() succeds with swapfile. > > > */ > > > +#define MNTPOINT "mntpoint" > > > +#define SWAP_FILE MNTPOINT"/swapfile01" > > > + > > > #include <unistd.h> > > > #include <errno.h> > > > #include <stdlib.h> > > > @@ -18,14 +21,14 @@ > > > static void verify_swapon(void) > > > { > > > - TEST(tst_syscall(__NR_swapon, "./swapfile01", 0)); > > > + TEST(tst_syscall(__NR_swapon, SWAP_FILE, 0)); > > > if (TST_RET == -1) { > > > tst_res(TFAIL | TTERRNO, "Failed to turn on swapfile"); > > > } else { > > > tst_res(TPASS, "Succeeded to turn on swapfile"); > > > /*we need to turn this swap file off for -i option */ > > > - if (tst_syscall(__NR_swapoff, "./swapfile01") != 0) { > > > + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { > > > tst_brk(TBROK | TERRNO, "Failed to turn off > > swapfile," > > > " system reboot after execution of LTP " > > > "test suite is recommended."); > > > @@ -35,13 +38,15 @@ static void verify_swapon(void) > > > static void setup(void) > > > { > > > - is_swap_supported("./tstswap"); > > > - make_swapfile("swapfile01", 0); > > > + 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 > > > }; > > > -- > > > 2.42.0 > > -- > > Cyril Hrubis > > chrubis@suse.cz > > -- > > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi! > > > For the previous iteration of this patch Li wasn't sure if this adds > > > enough value since there isn't ton of filesystem specific code involved > > > unless we actually start writing to the swapfile. > > I wonder how to force. We could setup > > sysctl vm.swappiness=100 > > But how to consume enough RAM to be actually written? One way is put the process into a cgroup with fairly small RAM limit and then allocate and fault memory, getting this right is not that easy though.
diff --git a/testcases/kernel/syscalls/swapon/swapon01.c b/testcases/kernel/syscalls/swapon/swapon01.c index b5c3f359c..6b7f96cf7 100644 --- a/testcases/kernel/syscalls/swapon/swapon01.c +++ b/testcases/kernel/syscalls/swapon/swapon01.c @@ -9,6 +9,9 @@ * Checks that swapon() succeds with swapfile. */ +#define MNTPOINT "mntpoint" +#define SWAP_FILE MNTPOINT"/swapfile01" + #include <unistd.h> #include <errno.h> #include <stdlib.h> @@ -18,14 +21,14 @@ static void verify_swapon(void) { - TEST(tst_syscall(__NR_swapon, "./swapfile01", 0)); + TEST(tst_syscall(__NR_swapon, SWAP_FILE, 0)); if (TST_RET == -1) { tst_res(TFAIL | TTERRNO, "Failed to turn on swapfile"); } else { tst_res(TPASS, "Succeeded to turn on swapfile"); /*we need to turn this swap file off for -i option */ - if (tst_syscall(__NR_swapoff, "./swapfile01") != 0) { + if (tst_syscall(__NR_swapoff, SWAP_FILE) != 0) { tst_brk(TBROK | TERRNO, "Failed to turn off swapfile," " system reboot after execution of LTP " "test suite is recommended."); @@ -35,13 +38,15 @@ static void verify_swapon(void) static void setup(void) { - is_swap_supported("./tstswap"); - make_swapfile("swapfile01", 0); + 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 };
Test on all filesystems to increase coverage. btrfs and tmpfs currently does not support swap file, but keep it in case this get changed in the future. Testing on all filesystems usually requests root, but we don't require it with .needs_root = 1. But using swapon() requires it as well, thus keep it defined. Also detect the support on the same file as which is being tested. Signed-off-by: Petr Vorel <pvorel@suse.cz> --- IMHO it's not a big slowdown thus I keep btrfs and tmpfs. They might get support one day. testcases/kernel/syscalls/swapon/swapon01.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)