| Message ID | 20240417090222.707302-1-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] libswap: Change TWARN message to TINFO | expand |
Hi Petr, On 4/17/24 11:02, Petr Vorel wrote: > This is hit on systems with 64kb page size (e.g. on aarch64 with > CONFIG_ARM64_64K_PAGES=y or on ppc64le with CONFIG_PAGE_SIZE_64KB=y). > Using TINFO causes test not "failing" with non-zero exit code. > > Fixes: f987ffff5 ("libswap: add two methods to create swapfile") > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > libs/libltpswap/libswap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > index 313a15f24..6d4184ef9 100644 > --- a/libs/libltpswap/libswap.c > +++ b/libs/libltpswap/libswap.c > @@ -160,7 +160,7 @@ int make_swapfile_(const char *file, const int lineno, > > /* To guarantee at least one page can be swapped out */ > if (blk_size * blocks < pg_size) { > - tst_res(TWARN, "Swapfile size is less than the system page size. " > + tst_res(TINFO, "Swapfile size is less than the system page size. " > "Using page size (%lu bytes) instead of block size (%lu bytes).", > (unsigned long)pg_size, blk_size); > blk_size = pg_size; Are we sure about this? TWARN wasn't a bad choice, considering the meaning of the message. Andrea
Hi, > This is hit on systems with 64kb page size (e.g. on aarch64 with > CONFIG_ARM64_64K_PAGES=y or on ppc64le with CONFIG_PAGE_SIZE_64KB=y). > Using TINFO causes test not "failing" with non-zero exit code. Also, sometimes we use: tst_res(TINFO, "WARNING: ...", e.g. TWARN which would still keep the test exiting 0 (I know, i bit strange). Maybe we would need some specific TWARN_NOFAIL, which would print as TWARN (=> more visible) but test exit code would be 0. Kind regards, Petr > Fixes: f987ffff5 ("libswap: add two methods to create swapfile") > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > libs/libltpswap/libswap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > index 313a15f24..6d4184ef9 100644 > --- a/libs/libltpswap/libswap.c > +++ b/libs/libltpswap/libswap.c > @@ -160,7 +160,7 @@ int make_swapfile_(const char *file, const int lineno, > /* To guarantee at least one page can be swapped out */ > if (blk_size * blocks < pg_size) { > - tst_res(TWARN, "Swapfile size is less than the system page size. " > + tst_res(TINFO, "Swapfile size is less than the system page size. " > "Using page size (%lu bytes) instead of block size (%lu bytes).", > (unsigned long)pg_size, blk_size); > blk_size = pg_size;
Hi! > This is hit on systems with 64kb page size (e.g. on aarch64 with > CONFIG_ARM64_64K_PAGES=y or on ppc64le with CONFIG_PAGE_SIZE_64KB=y). > Using TINFO causes test not "failing" with non-zero exit code. > > Fixes: f987ffff5 ("libswap: add two methods to create swapfile") > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > libs/libltpswap/libswap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > index 313a15f24..6d4184ef9 100644 > --- a/libs/libltpswap/libswap.c > +++ b/libs/libltpswap/libswap.c > @@ -160,7 +160,7 @@ int make_swapfile_(const char *file, const int lineno, > > /* To guarantee at least one page can be swapped out */ > if (blk_size * blocks < pg_size) { > - tst_res(TWARN, "Swapfile size is less than the system page size. " > + tst_res(TINFO, "Swapfile size is less than the system page size. " > "Using page size (%lu bytes) instead of block size (%lu bytes).", > (unsigned long)pg_size, blk_size); This looks like we are working around a test bug, which test is triggering this condition? > blk_size = pg_size; > -- > 2.43.0 >
> Hi! > > This is hit on systems with 64kb page size (e.g. on aarch64 with > > CONFIG_ARM64_64K_PAGES=y or on ppc64le with CONFIG_PAGE_SIZE_64KB=y). > > Using TINFO causes test not "failing" with non-zero exit code. > > Fixes: f987ffff5 ("libswap: add two methods to create swapfile") > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > libs/libltpswap/libswap.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > > index 313a15f24..6d4184ef9 100644 > > --- a/libs/libltpswap/libswap.c > > +++ b/libs/libltpswap/libswap.c > > @@ -160,7 +160,7 @@ int make_swapfile_(const char *file, const int lineno, > > /* To guarantee at least one page can be swapped out */ > > if (blk_size * blocks < pg_size) { > > - tst_res(TWARN, "Swapfile size is less than the system page size. " > > + tst_res(TINFO, "Swapfile size is less than the system page size. " > > "Using page size (%lu bytes) instead of block size (%lu bytes).", > > (unsigned long)pg_size, blk_size); > This looks like we are working around a test bug, which test is > triggering this condition? All swap tests: swapoff0[12], swapon0[1-3] https://lore.kernel.org/ltp/CAEemH2ev62JxH7-DA5Sc2LjMKrquYqt927ATHZefNPAOiXb5qA@mail.gmail.com/ Kind regards, Petr > > blk_size = pg_size; > > -- > > 2.43.0
Hi!
Looking at the patches we get the warning from the is_swap_supported()
because we create only 10 blocks worth of swapfile there, right?
So the easiest fix is to make sure we create big enough swapfile there
as well, what about?
diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c
index 313a15f24..36b5af483 100644
--- a/libs/libltpswap/libswap.c
+++ b/libs/libltpswap/libswap.c
@@ -192,7 +192,7 @@ int make_swapfile_(const char *file, const int lineno,
bool is_swap_supported(const char *filename)
{
int i, sw_support = 0;
- int ret = SAFE_MAKE_SWAPFILE_BLKS(filename, 10);
+ int ret = SAFE_MAKE_SWAPFILE_SIZE(filename, 1);
int fi_contiguous = file_is_contiguous(filename);
long fs_type = tst_fs_type(filename);
const char *fstype = tst_fs_type_name(fs_type);
> Hi! > Looking at the patches we get the warning from the is_swap_supported() > because we create only 10 blocks worth of swapfile there, right? > So the easiest fix is to make sure we create big enough swapfile there > as well, what about? > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > index 313a15f24..36b5af483 100644 > --- a/libs/libltpswap/libswap.c > +++ b/libs/libltpswap/libswap.c > @@ -192,7 +192,7 @@ int make_swapfile_(const char *file, const int lineno, > bool is_swap_supported(const char *filename) > { > int i, sw_support = 0; > - int ret = SAFE_MAKE_SWAPFILE_BLKS(filename, 10); > + int ret = SAFE_MAKE_SWAPFILE_SIZE(filename, 1); Let me first check where it's needed, because SAFE_MAKE_SWAPFILE_BLKS() is called on more places. Also, first is needed to move __FILE__, __LINE__ in macros (from function), sending patch... Kind regards, Petr > int fi_contiguous = file_is_contiguous(filename); > long fs_type = tst_fs_type(filename); > const char *fstype = tst_fs_type_name(fs_type);
> > Hi! > > Looking at the patches we get the warning from the is_swap_supported() > > because we create only 10 blocks worth of swapfile there, right? > > So the easiest fix is to make sure we create big enough swapfile there > > as well, what about? > > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > > index 313a15f24..36b5af483 100644 > > --- a/libs/libltpswap/libswap.c > > +++ b/libs/libltpswap/libswap.c > > @@ -192,7 +192,7 @@ int make_swapfile_(const char *file, const int lineno, > > bool is_swap_supported(const char *filename) > > { > > int i, sw_support = 0; > > - int ret = SAFE_MAKE_SWAPFILE_BLKS(filename, 10); > > + int ret = SAFE_MAKE_SWAPFILE_SIZE(filename, 1); > Let me first check where it's needed, because SAFE_MAKE_SWAPFILE_BLKS() is > called on more places. Yes, not only it's directly used from the library, which affects all swapo{ff,n} tests, but MAKE_SWAPFILE_BLKS() is also used directly by some of the tests. Changing it to {SAFE_,}MAKE_SWAPFILE_SIZE() on all places would mean we don't need {SAFE_,}MAKE_SWAPFILE_BLKS() at all. Therefore I think this check should be suppressed to TINFO. Other option is to use high enough value (more blocks) (not sure if using really small swap file makes sense anyway). > Also, first is needed to move __FILE__, __LINE__ in macros (from function), > sending patch... https://lore.kernel.org/ltp/20240417123113.731780-1-pvorel@suse.cz/ https://lore.kernel.org/ltp/20240417123113.731780-1-pvorel@suse.cz/ Kind regards, Petr > Kind regards, > Petr > > int fi_contiguous = file_is_contiguous(filename); > > long fs_type = tst_fs_type(filename); > > const char *fstype = tst_fs_type_name(fs_type);
Hi! > Changing it to {SAFE_,}MAKE_SWAPFILE_SIZE() on all places would mean we don't need > {SAFE_,}MAKE_SWAPFILE_BLKS() at all. Therefore I think this check should be > suppressed to TINFO. Other option is to use high enough value (more blocks) > (not sure if using really small swap file makes sense anyway). I do not think that we should use too small swap file even for the case where we try to detect if swapfiles are supported. Currently it seems that kernel accepts swapfiles smaller than page size, but it may also start rejecting them with EINVAL in the future. So I would keep the warning and rather tried to fix all the parts where we attempt to create a swapfile smaller than page size.
> Hi! > > Changing it to {SAFE_,}MAKE_SWAPFILE_SIZE() on all places would mean we don't need > > {SAFE_,}MAKE_SWAPFILE_BLKS() at all. Therefore I think this check should be > > suppressed to TINFO. Other option is to use high enough value (more blocks) > > (not sure if using really small swap file makes sense anyway). > I do not think that we should use too small swap file even for the case > where we try to detect if swapfiles are supported. Currently it seems > that kernel accepts swapfiles smaller than page size, but it may also > start rejecting them with EINVAL in the future. > So I would keep the warning and rather tried to fix all the parts where > we attempt to create a swapfile smaller than page size. +1, setting "changes requested", I'll send v2 with higher number of blocks to fix this. Kind regards, Petr
On Thu, Apr 18, 2024 at 2:38 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi! > > > Changing it to {SAFE_,}MAKE_SWAPFILE_SIZE() on all places would mean > we don't need > > > {SAFE_,}MAKE_SWAPFILE_BLKS() at all. Therefore I think this check > should be > > > suppressed to TINFO. Other option is to use high enough value (more > blocks) > > > (not sure if using really small swap file makes sense anyway). > > > I do not think that we should use too small swap file even for the case > > where we try to detect if swapfiles are supported. Currently it seems > > that kernel accepts swapfiles smaller than page size, but it may also > > start rejecting them with EINVAL in the future. > > > So I would keep the warning and rather tried to fix all the parts where > > we attempt to create a swapfile smaller than page size. > > +1, setting "changes requested", I'll send v2 with higher number of blocks > to > fix this. > Agree.
diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c index 313a15f24..6d4184ef9 100644 --- a/libs/libltpswap/libswap.c +++ b/libs/libltpswap/libswap.c @@ -160,7 +160,7 @@ int make_swapfile_(const char *file, const int lineno, /* To guarantee at least one page can be swapped out */ if (blk_size * blocks < pg_size) { - tst_res(TWARN, "Swapfile size is less than the system page size. " + tst_res(TINFO, "Swapfile size is less than the system page size. " "Using page size (%lu bytes) instead of block size (%lu bytes).", (unsigned long)pg_size, blk_size); blk_size = pg_size;
This is hit on systems with 64kb page size (e.g. on aarch64 with CONFIG_ARM64_64K_PAGES=y or on ppc64le with CONFIG_PAGE_SIZE_64KB=y). Using TINFO causes test not "failing" with non-zero exit code. Fixes: f987ffff5 ("libswap: add two methods to create swapfile") Signed-off-by: Petr Vorel <pvorel@suse.cz> --- libs/libltpswap/libswap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)