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