<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 Wed, Jun 5, 2019 at 5:30 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 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>
> >> 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 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", 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 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 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></blockquote><div><br></div><div class="gmail_default" style="font-size:small">I'm thinking maybe we cann't avoid adding a whilelist in the test, at least for known filesystem without FIBMAP supported. </div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">FYI: what do you think if change the is_swap_supported(...) like this?</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">void is_swap_supported(void (cleanup)(void), const char *filename)<br>{<br> int fibmap = tst_fibmap(filename);<br><br> if (fibmap == 1) {<br> int ret;</div><div class="gmail_default" style="font-size:small"> long fs_type = tst_fs_type(cleanup, filename);</div><div class="gmail_default" style="font-size:small"> const char *fstype = tst_fs_type_name(fs_type);</div><div class="gmail_default" style="font-size:small"><br> ret = make_swapfile(NULL, filename, 1);</div><div class="gmail_default" style="font-size:small"> if (ret != 0) {</div><div class="gmail_default" style="font-size:small"> if (fs_type == TST_NFS_MAGIC ||</div><div class="gmail_default" style="font-size:small"> fs_type == TST_TMPFS_MAGIC ||</div><div class="gmail_default" style="font-size:small"> fs_type == TST_BRTFS_MAGIC) {<br> tst_brkm(TFAIL, cleanup,<br> "mkswap on %s failed", fstype);<br> } else {<br> tst_brkm(TCONF, cleanup,<br> "mkswap on %s not supported", fstype);<br> }<br> }<br><br> TEST(ltp_syscall(__NR_swapon, filename, 0));<br> if (TEST_RETURN == -1) {<br> if (errno == EINVAL) {<br> tst_brkm(TCONF, cleanup,<br> "Swapfile on %s not implemented", fstype);<br> } else {<br> tst_brkm(TFAIL | TERRNO, cleanup,<br> "swapon on %s failed", fstype);<br> }<br> }<br><br> TEST(ltp_syscall(__NR_swapoff, filename, 0));</div><div class="gmail_default" style="font-size:small"> if (TEST_RETURN == -1) {<br> tst_brkm(TFAIL | TERRNO, cleanup,<br> "swapoff on %s failed", fstype);<br> }<br> }<br>}</div></div><div class="gmail_quote"><div class="gmail_default" style="font-size:small"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I'm going to dig more on fibmap/mkswap/swapon/swapoff support status of<br>
varies filesystems.<br>
<br>
> <br>
> <br>
> > + tst_brkm(TCONF, cleanup,<br>
> >> + "mkswap on %s not supported", fstype);<br>
> >> + } else {<br>
> >> + tst_brkm(TFAIL, cleanup,<br>
> >> + "mkswap on %s failed", fstype);<br>
> >> + }<br>
> >> + }<br>
> >> +<br>
> >> + TEST(ltp_syscall(__NR_swapon, filename, 0));<br>
> >> + if (TEST_RETURN == -1) {<br>
> >> + if (fibmap != 0 && errno == EINVAL) {<br>
> >> + tst_brkm(TCONF, cleanup,<br>
> >> + "Swapfile on %s not implemented", fstype);<br>
> >><br>
> ><br>
> > Maybe there is unnecessary to check fibmap value again? Since if the<br>
> > fibmap is 1, it has already broken in make_swapfile() error handler and<br>
> > never coming here?<br>
<br>
If mkswap succeeds, we are coming here.<br></blockquote><div><br></div><div class="gmail_default" style="font-size:small">Ah yes. I missed that situation.</div><div class="gmail_default" style="font-size:small"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks for reviewing!<br>
Murphy<br>
<br>
> ><br>
> ><br>
> ><br>
> >> + } else {<br>
> >> + tst_brkm(TFAIL | TERRNO, cleanup,<br>
> >> + "swapon on %s failed", fstype);<br>
> >> + }<br>
> >> + }<br>
> >> +<br>
> >> + TEST(ltp_syscall(__NR_swapoff, filename, 0));<br>
> >> + if (TEST_RETURN == -1) {<br>
> >> + tst_brkm(TFAIL | TERRNO, cleanup,<br>
> >> + "swapoff on %s failed", fstype);<br>
> >> + }<br>
> >> }<br>
> >> diff --git a/testcases/kernel/syscalls/swapon/libswapon.h<br>
> >> b/testcases/kernel/syscalls/swapon/libswapon.h<br>
> >> index 7f7211eb4..a51833ec1 100644<br>
> >> --- a/testcases/kernel/syscalls/swapon/libswapon.h<br>
> >> +++ b/testcases/kernel/syscalls/swapon/libswapon.h<br>
> >> @@ -29,6 +29,11 @@<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 safe);<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>
> >> #endif /* __LIBSWAPON_H__ */<br>
> >> --<br>
> >> 2.21.0<br>
> >><br>
> >><br>
> >> --<br>
> >> Mailing list info: <a href="https://lists.linux.it/listinfo/ltp" rel="noreferrer" target="_blank">https://lists.linux.it/listinfo/ltp</a><br>
> >><br>
> ><br>
> ><br>
> > --<br>
> > Regards,<br>
> > Li Wang<br>
> ><br>
> <br>
> <br>
> -- <br>
> Regards,<br>
> Li Wang<br>
</blockquote></div><br clear="all"><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>