<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 10, 2019 at 2:12 PM Murphy Zhou <<a href="mailto:xzhou@redhat.com">xzhou@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Jun 05, 2019 at 05:53:13PM +0800, Li Wang wrote:<br>
> On Wed, Jun 5, 2019 at 5:30 PM Murphy Zhou <<a href="mailto:xzhou@redhat.com" target="_blank">xzhou@redhat.com</a>> wrote:<br>
> <br>
> > On Wed, Jun 05, 2019 at 02:55:47PM +0800, Li Wang wrote:<br>
> > > On Wed, Jun 5, 2019 at 1:51 PM Li Wang <<a href="mailto:liwang@redhat.com" target="_blank">liwang@redhat.com</a>> wrote:<br>
> > ><br>
> > > ><br>
> > > ><br>
> > > > On Tue, May 28, 2019 at 10:13 PM Murphy Zhou <<a href="mailto:xzhou@redhat.com" target="_blank">xzhou@redhat.com</a>> wrote:<br>
> > > ><br>
> > > >> To check if the filesystem we are testing on supports FIBMAP, mkswap,<br>
> > > >> swapon and swapoff operations.<br>
> > > >> Modify make_swapfile function to test mkswap support status safely.<br>
> > > >><br>
> > > >> Signed-off-by: Murphy Zhou <<a href="mailto:xzhou@redhat.com" target="_blank">xzhou@redhat.com</a>><br>
> > > >> ---<br>
> > > >> testcases/kernel/syscalls/swapon/libswapon.c | 45<br>
> > +++++++++++++++++++-<br>
> > > >> testcases/kernel/syscalls/swapon/libswapon.h | 7 ++-<br>
> > > >> 2 files changed, 49 insertions(+), 3 deletions(-)<br>
> > > >><br>
> > > >> diff --git a/testcases/kernel/syscalls/swapon/libswapon.c<br>
> > > >> b/testcases/kernel/syscalls/swapon/libswapon.c<br>
> > > >> index cf6a98891..f66d19548 100644<br>
> > > >> --- a/testcases/kernel/syscalls/swapon/libswapon.c<br>
> > > >> +++ b/testcases/kernel/syscalls/swapon/libswapon.c<br>
> > > >> @@ -19,13 +19,15 @@<br>
> > > >> *<br>
> > > >> */<br>
> > > >><br>
> > > >> +#include <errno.h><br>
> > > >> +#include "lapi/syscalls.h"<br>
> > > >> #include "test.h"<br>
> > > >> #include "libswapon.h"<br>
> > > >><br>
> > > >> /*<br>
> > > >> * Make a swap file<br>
> > > >> */<br>
> > > >> -void make_swapfile(void (cleanup)(void), const char *swapfile)<br>
> > > >> +int make_swapfile(void (cleanup)(void), const char *swapfile, int<br>
> > safe)<br>
> > > >> {<br>
> > > >> if (!tst_fs_has_free(NULL, ".", sysconf(_SC_PAGESIZE) * 10,<br>
> > > >> TST_BYTES)) {<br>
> > > >> @@ -45,5 +47,44 @@ void make_swapfile(void (cleanup)(void), const char<br>
> > > >> *swapfile)<br>
> > > >> argv[1] = swapfile;<br>
> > > >> argv[2] = NULL;<br>
> > > >><br>
> > > >> - tst_run_cmd(cleanup, argv, "/dev/null", "/dev/null", 0);<br>
> > > >> + return tst_run_cmd(cleanup, argv, "/dev/null", "/dev/null",<br>
> > safe);<br>
> > > >> +}<br>
> > > >> +<br>
> > > >> +/*<br>
> > > >> + * Check swapon/swapoff support status of filesystems or files<br>
> > > >> + * we are testing on.<br>
> > > >> + */<br>
> > > >> +void is_swap_supported(void (cleanup)(void), const char *filename)<br>
> > > >> +{<br>
> > > >> + int fibmap = tst_fibmap(filename);<br>
> > > >> + long fs_type = tst_fs_type(cleanup, filename);<br>
> > > >> + const char *fstype = tst_fs_type_name(fs_type);<br>
> > > >> +<br>
> > > >> + int ret = make_swapfile(NULL, filename, 1);<br>
> > > >> + if (ret != 0) {<br>
> > > >> + if (fibmap != 0) {<br>
> > > >><br>
> > > ><br>
> > > > As I replied in patch 1/4, how do we know that means FIBMAP not<br>
> > support if<br>
> > > > just verify fibmap != 0?<br>
> > > > So I would suggest to make the return value of tst_fibmap() is more<br>
> > > > precise and do if (fibmap == 1) check here.<br>
> ><br>
> > Very good catch. The return value should be more precise. Thanks!<br>
> ><br>
> > > ><br>
> > ><br>
> > > And also, imagine that if swapon01 test failed on BRTFS or NFS(support<br>
> > > swapfile but not<br>
> > > support FIBMAP ioctl), then here will report the new bug as a TCONF to<br>
> > LTP.<br>
> ><br>
> > Here is testing mkswap for swapon test preparation. If mkswap fail, and<br>
> > FIBMAP not supported, it's reasonable to me that we should not go on to<br>
> > test swapon.<br>
> ><br>
> > But yes, if a regression causes mkswap fail without FIBMAP supported, we<br>
> > could miss the bug here like you described. This situation should be<br>
> > covered by tcase for mkswap IMO.<br>
> ><br>
> <br>
> I'm thinking maybe we cann't avoid adding a whilelist in the test, at least<br>
> for known filesystem without FIBMAP supported.<br>
<br>
No, I think we don't need a whilelist here. Amir and I discussed long way<br>
to here, you can check that. We are not using fibmap test as a verdict for<br>
swapfile is supported or not. Doing a mkswap/swapon/swapoff test before<br>
real tests to detect the support situation. tst_fibmap result helps us<br>
to decide how to report. If the underneath filesystem can survive these<br>
test, it should be supporting swapfiles and swapon/swapoff tests should<br>
run.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
The whitelisted nfs and tmpfs are well covered by these pre-tests. More, <br>
NFS should not be skipped as if we turn on kernle configs for NFS_SWAP,<br>
these tests can run on NFS.<br>
<br>
Whitelist could skip more tests and bugs.<br>
<br>
I tested some fibmap/mkswap/swapon/swapoff tests for your reference:<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Thanks a lot for the test report, good to know these details.</div></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
1 means positive, 0 means negative.<br>
<br>
-------- On 5.3-rc3+ kernel, NFS_SWAP=y SUNRPC_SWAP=y -------<br>
fibamp mkswap swapon swapoff<br>
xfs 1 1 1 1<br>
ext4 1 1 1 1<br>
<br>
btrfs 0 1 0 0<br>
tmpfs 0 1 0 0<br>
<br>
ovl 1 1 1 1<br>
<br>
cifs<br>
v3.11 0 1 0 0<br>
v1 0 1 0 0<br>
v2 0 1 0 0<br>
<br>
nfs<br>
v4.2 0 1 1* 1<br>
v4.1 0 1 1 1<br>
v4.0 0 1 1* 1<br>
v3 0 1 1 1<br>
<br>
* hang sometimes<br>
<br>
--------- On 3.10.0 based kernel ----------------<br>
fibamp mkswap swapon swapoff<br>
xfs 1 1 1 1<br>
ext4 1 1 1 1<br>
<br>
btrfs 0 1 0 0<br>
tmpfs 0 1 0 0<br>
<br>
ovl 1 1 1 1<br>
<br>
cifs<br>
v3.0 0 1 0 0<br>
<br>
nfs<br>
v4.2 0 1 0 0<br>
<br>
<br>
---------- On 2.6.32 based kernel ---------------<br>
fibamp mkswap swapon swapoff<br>
tmpfs 0 1 0 0<br>
xfs 1 1 1 1<br>
ext4 1 1 1 1<br>
cifs 0 0 0 0<br>
nfs 0 0 0 0<br>
<br>
> <br>
> FYI: what do you think if change the is_swap_supported(...) like this?<br>
> <br>
> void is_swap_supported(void (cleanup)(void), const char *filename)<br>
> {<br>
> int fibmap = tst_fibmap(filename);<br>
> <br>
> if (fibmap == 1) {<br>
<br>
fibmap == 0 does not mean swapfile is supported. We need to make sure that<br>
survivors of this function are supporting swapfiles.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Yes, it sounds quite reasonable. So besides that return value of tst_fibmap() is not precise, patch V6 LGTM. Feel free to add my review in patch v7:</div></div></div><div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Reviewed-by: Li Wang <<a href="mailto:liwang@redhat.com">liwang@redhat.com</a>></div></div><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>