[LTP] [PATCH v1] swapoff01.c: Adjust blocks size base on pagesize

[LTP] [PATCH v1] swapoff01.c: Adjust blocks size base on pagesize

[Wei Gao via ltp]

The make_swapfile function will encounter no space error if pagesize
is bigger then 4096, such as ppc64 system use default pagesize 65535.

Signed-off-by: Wei Gao <wegao@suse.com>
---
 testcases/kernel/syscalls/swapoff/swapoff01.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c b/testcases/kernel/syscalls/swapoff/swapoff01.c
index c303588df..71d6c6c04 100644
--- a/testcases/kernel/syscalls/swapoff/swapoff01.c
+++ b/testcases/kernel/syscalls/swapoff/swapoff01.c
@@ -44,7 +44,12 @@ static void setup(void)
 {
 	is_swap_supported(TEST_FILE);
 
-	if (make_swapfile(SWAP_FILE, 65536, 1))
+	int blocks = 65535;
+
+	if (getpagesize() > 4096)
+		blocks = 65535 * 4096 / getpagesize();
+
+	if (make_swapfile(SWAP_FILE, blocks, 1))
 		tst_brk(TBROK, "Failed to create file for swap");
 }
 
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] swapoff01.c: Adjust blocks size base on pagesize

[Li Wang]

On Thu, Feb 22, 2024 at 12:41 PM Wei Gao via ltp <ltp@lists.linux.it> wrote:

> The make_swapfile function will encounter no space error if pagesize
> is bigger then 4096, such as ppc64 system use default pagesize 65535.
>

What erros does it shows in the log?

I can't reproduce this on my aarch64 (pagesize == 65536).
Seems the '.dev_min_size = 350' is large enough for testing.



>
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/syscalls/swapoff/swapoff01.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c
> b/testcases/kernel/syscalls/swapoff/swapoff01.c
> index c303588df..71d6c6c04 100644
> --- a/testcases/kernel/syscalls/swapoff/swapoff01.c
> +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c
> @@ -44,7 +44,12 @@ static void setup(void)
>  {
>         is_swap_supported(TEST_FILE);
>
> -       if (make_swapfile(SWAP_FILE, 65536, 1))
> +       int blocks = 65535;
> +
> +       if (getpagesize() > 4096)
> +               blocks = 65535 * 4096 / getpagesize();
> +
> +       if (make_swapfile(SWAP_FILE, blocks, 1))
>                 tst_brk(TBROK, "Failed to create file for swap");
>  }
>
> --
> 2.35.3
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] swapoff01.c: Adjust blocks size base on pagesize

[Wei Gao via ltp]

On Thu, Feb 22, 2024 at 03:57:57PM +0800, Li Wang wrote:
> On Thu, Feb 22, 2024 at 12:41 PM Wei Gao via ltp <ltp@lists.linux.it> wrote:
> 
> > The make_swapfile function will encounter no space error if pagesize
> > is bigger then 4096, such as ppc64 system use default pagesize 65535.
> >
> 
> What erros does it shows in the log?
> 
> I can't reproduce this on my aarch64 (pagesize == 65536).
> Seems the '.dev_min_size = 350' is large enough for testing.
> 
Sorry , it should be block size == 65536.
> 
> 
> >
> > Signed-off-by: Wei Gao <wegao@suse.com>
> > ---
> >  testcases/kernel/syscalls/swapoff/swapoff01.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c
> > b/testcases/kernel/syscalls/swapoff/swapoff01.c
> > index c303588df..71d6c6c04 100644
> > --- a/testcases/kernel/syscalls/swapoff/swapoff01.c
> > +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c
> > @@ -44,7 +44,12 @@ static void setup(void)
> >  {
> >         is_swap_supported(TEST_FILE);
> >
> > -       if (make_swapfile(SWAP_FILE, 65536, 1))
> > +       int blocks = 65535;
> > +
> > +       if (getpagesize() > 4096)
> > +               blocks = 65535 * 4096 / getpagesize();
> > +
> > +       if (make_swapfile(SWAP_FILE, blocks, 1))
> >                 tst_brk(TBROK, "Failed to create file for swap");
> >  }
> >
> > --
> > 2.35.3
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> >
> >
> 
> -- 
> Regards,
> Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] swapoff01.c: Adjust blocks size base on pagesize

[Cyril Hrubis]

Hi!
> The make_swapfile function will encounter no space error if pagesize
> is bigger then 4096, such as ppc64 system use default pagesize 65535.
                                                                    ^
								 65536?

Isn't this more about Btrfs though? Looking at the make_swapfile() we do
use statvfs to get filesystem block size and if that is Btrfs with 64k
blocks we end up with swapfile of a size of 4GB that sounds like a bit
too much I guess.

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  testcases/kernel/syscalls/swapoff/swapoff01.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c b/testcases/kernel/syscalls/swapoff/swapoff01.c
> index c303588df..71d6c6c04 100644
> --- a/testcases/kernel/syscalls/swapoff/swapoff01.c
> +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c
> @@ -44,7 +44,12 @@ static void setup(void)
>  {
>  	is_swap_supported(TEST_FILE);
>  
> -	if (make_swapfile(SWAP_FILE, 65536, 1))
> +	int blocks = 65535;
> +
> +	if (getpagesize() > 4096)
> +		blocks = 65535 * 4096 / getpagesize();
> +
> +	if (make_swapfile(SWAP_FILE, blocks, 1))
>  		tst_brk(TBROK, "Failed to create file for swap");


I do not think that this is a right solution though. Is there any reason
why we pass number of blocks to the make_swapfile instead of megabytes?

>  }
>  
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] swapoff01.c: Adjust blocks size base on pagesize

[Wei Gao via ltp]

On Thu, Feb 22, 2024 at 09:39:43AM +0100, Cyril Hrubis wrote:
> Hi!
> > The make_swapfile function will encounter no space error if pagesize
> > is bigger then 4096, such as ppc64 system use default pagesize 65535.
>                                                                     ^
> 								 65536?
Yes
> 
> Isn't this more about Btrfs though? Looking at the make_swapfile() we do
> use statvfs to get filesystem block size and if that is Btrfs with 64k
> blocks we end up with swapfile of a size of 4GB that sounds like a bit
> too much I guess.
> 

Sorry for confusing. It should caused by block size instead of page size.
The code change should change like following, but i suppose you will not agree
on this solution, correct?

--- a/testcases/kernel/syscalls/swapoff/swapoff01.c
+++ b/testcases/kernel/syscalls/swapoff/swapoff01.c
@@ -12,6 +12,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <stdlib.h>
+#include <sys/statvfs.h>

 #include "tst_test.h"
 #include "lapi/syscalls.h"
@@ -44,8 +45,21 @@ static void setup(void)
 {
        is_swap_supported(TEST_FILE);

-       if (make_swapfile(SWAP_FILE, 65536, 1))
+       struct statvfs fs_info;
+       unsigned long blk_size;
+       if (statvfs(".", &fs_info) == -1)
+               tst_brk(TBROK, "Failed to get statvfs info");
+
+       blk_size = fs_info.f_bsize;
+
+       int blocks = 65535;
+
+       if (blk_size > 4096)
+               blocks = 65535 * 4096 / blk_size;
+
+       if (make_swapfile(SWAP_FILE, blocks, 1))
                tst_brk(TBROK, "Failed to create file for swap");
+
 }


> > Signed-off-by: Wei Gao <wegao@suse.com>
> > ---
> >  testcases/kernel/syscalls/swapoff/swapoff01.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c b/testcases/kernel/syscalls/swapoff/swapoff01.c
> > index c303588df..71d6c6c04 100644
> > --- a/testcases/kernel/syscalls/swapoff/swapoff01.c
> > +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c
> > @@ -44,7 +44,12 @@ static void setup(void)
> >  {
> >  	is_swap_supported(TEST_FILE);
> >  
> > -	if (make_swapfile(SWAP_FILE, 65536, 1))
> > +	int blocks = 65535;
> > +
> > +	if (getpagesize() > 4096)
> > +		blocks = 65535 * 4096 / getpagesize();
> > +
> > +	if (make_swapfile(SWAP_FILE, blocks, 1))
> >  		tst_brk(TBROK, "Failed to create file for swap");
> 
> 
> I do not think that this is a right solution though. Is there any reason
> why we pass number of blocks to the make_swapfile instead of megabytes?
> 
@Li Wang, could you give some clue for above question(for why pass number of blocks instead of megabytes)?

Thanks all for comments!

> >  }
> >  
> > -- 
> > 2.35.3
> > 
> > 
> > -- 
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] swapoff01.c: Adjust blocks size base on pagesize

[Cyril Hrubis]

Hi!
> > Isn't this more about Btrfs though? Looking at the make_swapfile() we do
> > use statvfs to get filesystem block size and if that is Btrfs with 64k
> > blocks we end up with swapfile of a size of 4GB that sounds like a bit
> > too much I guess.
> > 
> 
> Sorry for confusing. It should caused by block size instead of page size.
> The code change should change like following, but i suppose you will not agree
> on this solution, correct?
>
> --- a/testcases/kernel/syscalls/swapoff/swapoff01.c
> +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c
> @@ -12,6 +12,7 @@
>  #include <unistd.h>
>  #include <errno.h>
>  #include <stdlib.h>
> +#include <sys/statvfs.h>
> 
>  #include "tst_test.h"
>  #include "lapi/syscalls.h"
> @@ -44,8 +45,21 @@ static void setup(void)
>  {
>         is_swap_supported(TEST_FILE);
> 
> -       if (make_swapfile(SWAP_FILE, 65536, 1))
> +       struct statvfs fs_info;
> +       unsigned long blk_size;
> +       if (statvfs(".", &fs_info) == -1)
> +               tst_brk(TBROK, "Failed to get statvfs info");
> +
> +       blk_size = fs_info.f_bsize;
> +
> +       int blocks = 65535;
> +
> +       if (blk_size > 4096)
> +               blocks = 65535 * 4096 / blk_size;
> +
> +       if (make_swapfile(SWAP_FILE, blocks, 1))
>                 tst_brk(TBROK, "Failed to create file for swap");
> +
>  }

It may be easier to change the make_swapfile() function to accept
megabytes instead of working around it like this.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] swapoff01.c: Adjust blocks size base on pagesize

[Li Wang]

Hi Wei, Cyril,


> > I do not think that this is a right solution though. Is there any reason
> > why we pass number of blocks to the make_swapfile instead of megabytes?
> >
> @Li Wang, could you give some clue for above question(for why pass number
> of blocks instead of megabytes)?
>

I just keep the function interface like the original
(it wasn't designed by me:), but I guess the only
advantage is to test 1 block for different FS types.

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] swapoff01.c: Adjust blocks size base on pagesize

[Petr Vorel]

Hi all,

> Hi Wei, Cyril,


> > > I do not think that this is a right solution though. Is there any reason
> > > why we pass number of blocks to the make_swapfile instead of megabytes?

> > @Li Wang, could you give some clue for above question(for why pass number
> > of blocks instead of megabytes)?


> I just keep the function interface like the original
> (it wasn't designed by me:), but I guess the only
> advantage is to test 1 block for different FS types.

We use this function with 1 block, 10 blocks and 65536 blocks

How about to have special function for 1 block and then other function which
accepts MB? (e.g. 1 MB and 256 MB)?

But could we first merge Yang Xu patchset so that he does not have to rebase it
for ever?

https://patchwork.ozlabs.org/project/ltp/list/?series=395713&state=*

Also it'd be nice to have, but this can definitely wait after other things are
solved.

-int make_swapfile(const char *swapfile, int blocks, int safe)
+int _make_swapfile(const char *swapfile, int blocks, int safe)

+#define MAKE_SWAPFILE(const char *swapfile, int blocks, int safe) \
+	_make_swapfile(swapfile, blocks, 0)

+#define SAFE_SWAPFILE(const char *swapfile, int blocks, int safe) \
+	_make_swapfile(swapfile, blocks, 1)

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp