Message ID | 20240322030208.3278120-1-liwang@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series | [v3,1/2] libswap: add two methods to create swapfile | expand |
Hi Li, ... > Signed-off-by: Li Wang <liwang@redhat.com> > Signed-off-by: Wei Gao <wegao@suse.com> > --- ... -int make_swapfile(const char *swapfile, int blocks, int safe) +int make_swapfile_(const char *file, const int lineno, + const char *swapfile, unsigned int num, + int safe, enum swapfile_method method) { struct statvfs fs_info; - unsigned long blk_size, bs; + unsigned long blk_size; + unsigned int blocks = 0; size_t pg_size = sysconf(_SC_PAGESIZE); - char mnt_path[100]; + char mnt_path[128]; nit: why this increase to 128? Why not PATH_MAX? > /* To guarantee at least one page can be swapped out */ > - if (blk_size * blocks < pg_size) > - bs = pg_size; > - else > - bs = blk_size; > + if (blk_size * blocks < pg_size) { > + tst_res(TWARN, "Swapfile size is less than the system page size. \ > + Using page size (%lu bytes) instead of block size (%lu bytes).", libswap.c:163: WARNING: Avoid line continuations in quoted strings This will fix it: tst_res(TWARN, "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; > + } > if (sscanf(swapfile, "%[^/]", mnt_path) != 1) > - tst_brk(TBROK, "sscanf failed"); > + tst_brk_(file, lineno, TBROK, "sscanf failed"); > - if (!tst_fs_has_free(mnt_path, bs * blocks, TST_BYTES)) > - tst_brk(TCONF, "Insufficient disk space to create swap file"); > + if (!tst_fs_has_free(mnt_path, blk_size * blocks, TST_BYTES)) > + tst_brk_(file, lineno, TCONF, "Insufficient disk space to create swap file"); > /* create file */ > - if (prealloc_contiguous_file(swapfile, bs, blocks) != 0) > - tst_brk(TBROK, "Failed to create swapfile"); > + if (prealloc_contiguous_file(swapfile, blk_size, blocks) != 0) > + tst_brk_(file, lineno, TBROK, "Failed to create swapfile"); > /* Fill the file if needed (specific to old xfs filesystems) */ > if (tst_fs_type(swapfile) == TST_XFS_MAGIC) { > - if (tst_fill_file(swapfile, 0, bs, blocks) != 0) > - tst_brk(TBROK, "Failed to fill swapfile"); > + if (tst_fill_file(swapfile, 0, blk_size, blocks) != 0) > + tst_brk_(file, lineno, TBROK, "Failed to fill swapfile"); > } > /* make the file swapfile */ > - const char *argv[2 + 1]; > - > - argv[0] = "mkswap"; > - argv[1] = swapfile; > - argv[2] = NULL; > + const char *argv[] = {"mkswap", swapfile, NULL}; libswap.c:186: WARNING: char * array declaration might be better as static const This will fix it: const char *const argv[] = {"mkswap", swapfile, NULL}; > return tst_cmd(argv, "/dev/null", "/dev/null", safe ? > - TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING : 0); > + TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING : 0); The rest LGTM. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
On Fri, Mar 22, 2024 at 1:00 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > ... > > Signed-off-by: Li Wang <liwang@redhat.com> > > Signed-off-by: Wei Gao <wegao@suse.com> > > --- > ... > -int make_swapfile(const char *swapfile, int blocks, int safe) > +int make_swapfile_(const char *file, const int lineno, > + const char *swapfile, unsigned int num, > + int safe, enum swapfile_method method) > { > struct statvfs fs_info; > - unsigned long blk_size, bs; > + unsigned long blk_size; > + unsigned int blocks = 0; > size_t pg_size = sysconf(_SC_PAGESIZE); > - char mnt_path[100]; > + char mnt_path[128]; > > nit: why this increase to 128? Why not PATH_MAX? > No special meaning, I just thought of using 2 raised to the nth power. But you're right, PATCH_MAX will be safer. > > > /* To guarantee at least one page can be swapped out */ > > - if (blk_size * blocks < pg_size) > > - bs = pg_size; > > - else > > - bs = blk_size; > > + if (blk_size * blocks < pg_size) { > > + tst_res(TWARN, "Swapfile size is less than the system page > size. \ > > + Using page size (%lu bytes) instead of block size > (%lu bytes).", > > libswap.c:163: WARNING: Avoid line continuations in quoted strings > > This will fix it: > > tst_res(TWARN, "Swapfile size is less than the system page > size. " > "Using page size (%lu bytes) instead of block size > (%lu bytes).", > +1 > > + (unsigned long)pg_size, blk_size); > > + blk_size = pg_size; > > + } > > > if (sscanf(swapfile, "%[^/]", mnt_path) != 1) > > - tst_brk(TBROK, "sscanf failed"); > > + tst_brk_(file, lineno, TBROK, "sscanf failed"); > > > - if (!tst_fs_has_free(mnt_path, bs * blocks, TST_BYTES)) > > - tst_brk(TCONF, "Insufficient disk space to create swap > file"); > > + if (!tst_fs_has_free(mnt_path, blk_size * blocks, TST_BYTES)) > > + tst_brk_(file, lineno, TCONF, "Insufficient disk space to > create swap file"); > > > /* create file */ > > - if (prealloc_contiguous_file(swapfile, bs, blocks) != 0) > > - tst_brk(TBROK, "Failed to create swapfile"); > > + if (prealloc_contiguous_file(swapfile, blk_size, blocks) != 0) > > + tst_brk_(file, lineno, TBROK, "Failed to create swapfile"); > > > /* Fill the file if needed (specific to old xfs filesystems) */ > > if (tst_fs_type(swapfile) == TST_XFS_MAGIC) { > > - if (tst_fill_file(swapfile, 0, bs, blocks) != 0) > > - tst_brk(TBROK, "Failed to fill swapfile"); > > + if (tst_fill_file(swapfile, 0, blk_size, blocks) != 0) > > + tst_brk_(file, lineno, TBROK, "Failed to fill > swapfile"); > > } > > > /* make the file swapfile */ > > - const char *argv[2 + 1]; > > - > > - argv[0] = "mkswap"; > > - argv[1] = swapfile; > > - argv[2] = NULL; > > + const char *argv[] = {"mkswap", swapfile, NULL}; > > libswap.c:186: WARNING: char * array declaration might be better as static > const > > This will fix it: > > const char *const argv[] = {"mkswap", swapfile, NULL}; > +1 > > return tst_cmd(argv, "/dev/null", "/dev/null", safe ? > > - TST_CMD_PASS_RETVAL | > TST_CMD_TCONF_ON_MISSING : 0); > > + TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING : > 0); > > The rest LGTM. > Thanks, I would give the patch set more time in case others have comments. > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > Petr > >
Hi Li, > > - char mnt_path[100]; > > + char mnt_path[128]; > > nit: why this increase to 128? Why not PATH_MAX? > No special meaning, I just thought of using 2 raised to the nth power. > But you're right, PATCH_MAX will be safer. +1 ... > Thanks, I would give the patch set more time in case others have comments. Sure. Thanks for a nice work! Kind regards, Petr
Petr Vorel <pvorel@suse.cz> wrote: > > > Thanks, I would give the patch set more time in case others have > comments. > > Sure. Thanks for a nice work! > Patch set applied.
Hi Li, > Petr Vorel <pvorel@suse.cz> wrote: > > > Thanks, I would give the patch set more time in case others have > > comments. > > Sure. Thanks for a nice work! > Patch set applied. FYI it looks like this patch merge as: f987ffff5 ("libswap: add two methods to create swapfile") introduced TWARN on all tests which touched: swapoff0[12], swapon0[1-3] on openSUSE Tumbleweed on ppc64le (e.g. very close to mainline stable kernels: 6.8.x). Have you noticed this on Fedora as well? I hope to manage to have look on it soon, but maybe it's obvious to you already. libswap.c:163: TWARN: Swapfile size is less than the system page size. Using page size (65536 bytes) instead of block size (4096 bytes). Example of the warning: * swapoff01 on all of the filesystems: tst_test.c:1701: TINFO: === Testing on ext2 === tst_test.c:1117: TINFO: Formatting /dev/loop0 with ext2 opts='' extra opts='' mke2fs 1.47.0 (5-Feb-2023) tst_test.c:1131: TINFO: Mounting /dev/loop0 to /var/tmp/LTP_swaWdvJcZ/mntpoint fstyp=ext2 flags=0 libswap.c:156: TINFO: create a swapfile with 10 block numbers libswap.c:163: TWARN: Swapfile size is less than the system page size. Using page size (65536 bytes) instead of block size (4096 bytes). * swapon03 tst_test.c:1701: TINFO: === Testing on ext2 === tst_test.c:1117: TINFO: Formatting /dev/loop0 with ext2 opts='' extra opts='' mke2fs 1.47.0 (5-Feb-2023) tst_test.c:1131: TINFO: Mounting /dev/loop0 to /var/tmp/LTP_swalq2Qxc/mntpoint fstyp=ext2 flags=0 libswap.c:156: TINFO: create a swapfile with 10 block numbers libswap.c:163: TWARN: Swapfile size is less than the system page size. Using page size (65536 bytes) instead of block size (4096 bytes). tst_ioctl.c:26: TINFO: FIBMAP ioctl is supported tst_kconfig.c:88: TINFO: Parsing kernel config '/proc/config.gz' tst_kconfig.c:88: TINFO: Parsing kernel config '/proc/config.gz' tst_kconfig.c:88: TINFO: Parsing kernel config '/proc/config.gz' tst_kconfig.c:88: TINFO: Parsing kernel config '/proc/config.gz' libswap.c:156: TINFO: create a swapfile with 10 block numbers libswap.c:163: TWARN: Swapfile size is less than the system page size. Using page size (65536 bytes) instead of block size (4096 bytes). libswap.c:156: TINFO: create a swapfile with 10 block numbers libswap.c:163: TWARN: Swapfile size is less than the system page size. Using page size (65536 bytes) instead of block size (4096 bytes). libswap.c:156: TINFO: create a swapfile with 10 block numbers libswap.c:163: TWARN: Swapfile size is less than the system page size. Using page size (65536 bytes) instead of block size (4096 bytes). ... Kind regards, Petr
Hi Petr, Thanks for reporting this. On Mon, Apr 15, 2024 at 11:20 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > Petr Vorel <pvorel@suse.cz> wrote: > > > > > Thanks, I would give the patch set more time in case others have > > > comments. > > > > Sure. Thanks for a nice work! > > > > Patch set applied. > > FYI it looks like this patch merge as: > > f987ffff5 ("libswap: add two methods to create swapfile") > > introduced TWARN on all tests which touched: swapoff0[12], swapon0[1-3] on > openSUSE Tumbleweed > on ppc64le (e.g. very close to mainline stable kernels: 6.8.x). Have you > noticed > this on Fedora as well? I hope to manage to have look on it soon, but > maybe > it's obvious to you already. > > libswap.c:163: TWARN: Swapfile size is less than the system page size. > Using page size (65536 bytes) instead of block size (4096 bytes). > Yes, that is expected on a system with a larger page size, so it could guarantee at least one page can be swapped out. Do you want to switch to using TINFO for the choice? TBH it doesn't matter to report that WARNING message.
diff --git a/include/libswap.h b/include/libswap.h index 8c75e20ae..96e718542 100644 --- a/include/libswap.h +++ b/include/libswap.h @@ -11,10 +11,49 @@ #ifndef __LIBSWAP_H__ #define __LIBSWAP_H__ +enum swapfile_method { + SWAPFILE_BY_SIZE, + SWAPFILE_BY_BLKS +}; + /* - * Make a swap file + * Create a swapfile of a specified size or number of blocks. + */ +int make_swapfile_(const char *file, const int lineno, + const char *swapfile, unsigned int num, + int safe, enum swapfile_method method); + +static inline int make_swapfile(const char *swapfile, unsigned int num, + int safe, enum swapfile_method method) +{ + return make_swapfile_(__FILE__, __LINE__, swapfile, num, safe, method); +} + +/** + * Macro to create swapfile size in megabytes (MB). + */ +#define MAKE_SWAPFILE_SIZE(swapfile, size) \ + make_swapfile(swapfile, size, 0, SWAPFILE_BY_SIZE) + +/** + * Macro to create swapfile size in block numbers. + */ +#define MAKE_SWAPFILE_BLKS(swapfile, blocks) \ + make_swapfile(swapfile, blocks, 0, SWAPFILE_BY_BLKS) + +/** + * Macro to safely create swapfile size in megabytes (MB). + * Includes safety checks to handle potential errors. + */ +#define SAFE_MAKE_SWAPFILE_SIZE(swapfile, size) \ + make_swapfile(swapfile, size, 1, SWAPFILE_BY_SIZE) + +/** + * Macro to safely create swapfile size in block numbers. + * Includes safety checks to handle potential errors. */ -int make_swapfile(const char *swapfile, int blocks, int safe); +#define SAFE_MAKE_SWAPFILE_BLKS(swapfile, blocks) \ + make_swapfile(swapfile, blocks, 1, SWAPFILE_BY_BLKS) /* * Check swapon/swapoff support status of filesystems or files diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c index a26ea25e4..a3c47e9e2 100644 --- a/libs/libltpswap/libswap.c +++ b/libs/libltpswap/libswap.c @@ -133,55 +133,66 @@ out: return contiguous; } -int make_swapfile(const char *swapfile, int blocks, int safe) +int make_swapfile_(const char *file, const int lineno, + const char *swapfile, unsigned int num, + int safe, enum swapfile_method method) { struct statvfs fs_info; - unsigned long blk_size, bs; + unsigned long blk_size; + unsigned int blocks = 0; size_t pg_size = sysconf(_SC_PAGESIZE); - char mnt_path[100]; + char mnt_path[128]; if (statvfs(".", &fs_info) == -1) - return -1; + tst_brk_(file, lineno, TBROK, "statvfs failed"); blk_size = fs_info.f_bsize; + if (method == SWAPFILE_BY_SIZE) { + tst_res(TINFO, "create a swapfile size of %u megabytes (MB)", num); + blocks = num * 1024 * 1024 / blk_size; + } else if (method == SWAPFILE_BY_BLKS) { + blocks = num; + tst_res(TINFO, "create a swapfile with %u block numbers", blocks); + } else { + tst_brk_(file, lineno, TBROK, "Invalid method, please see include/libswap.h"); + } + /* To guarantee at least one page can be swapped out */ - if (blk_size * blocks < pg_size) - bs = pg_size; - else - bs = blk_size; + if (blk_size * blocks < pg_size) { + tst_res(TWARN, "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; + } if (sscanf(swapfile, "%[^/]", mnt_path) != 1) - tst_brk(TBROK, "sscanf failed"); + tst_brk_(file, lineno, TBROK, "sscanf failed"); - if (!tst_fs_has_free(mnt_path, bs * blocks, TST_BYTES)) - tst_brk(TCONF, "Insufficient disk space to create swap file"); + if (!tst_fs_has_free(mnt_path, blk_size * blocks, TST_BYTES)) + tst_brk_(file, lineno, TCONF, "Insufficient disk space to create swap file"); /* create file */ - if (prealloc_contiguous_file(swapfile, bs, blocks) != 0) - tst_brk(TBROK, "Failed to create swapfile"); + if (prealloc_contiguous_file(swapfile, blk_size, blocks) != 0) + tst_brk_(file, lineno, TBROK, "Failed to create swapfile"); /* Fill the file if needed (specific to old xfs filesystems) */ if (tst_fs_type(swapfile) == TST_XFS_MAGIC) { - if (tst_fill_file(swapfile, 0, bs, blocks) != 0) - tst_brk(TBROK, "Failed to fill swapfile"); + if (tst_fill_file(swapfile, 0, blk_size, blocks) != 0) + tst_brk_(file, lineno, TBROK, "Failed to fill swapfile"); } /* make the file swapfile */ - const char *argv[2 + 1]; - - argv[0] = "mkswap"; - argv[1] = swapfile; - argv[2] = NULL; + const char *argv[] = {"mkswap", swapfile, NULL}; return tst_cmd(argv, "/dev/null", "/dev/null", safe ? - TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING : 0); + TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING : 0); } bool is_swap_supported(const char *filename) { int i, sw_support = 0; - int ret = make_swapfile(filename, 10, 1); + int ret = SAFE_MAKE_SWAPFILE_BLKS(filename, 10); int fi_contiguous = file_is_contiguous(filename); long fs_type = tst_fs_type(filename); const char *fstype = tst_fs_type_name(fs_type); diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c b/testcases/kernel/syscalls/swapoff/swapoff01.c index 2a0b683c1..cf13907e7 100644 --- a/testcases/kernel/syscalls/swapoff/swapoff01.c +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c @@ -44,7 +44,7 @@ static void setup(void) { is_swap_supported(TEST_FILE); - if (make_swapfile(SWAP_FILE, 65536, 1)) + if (SAFE_MAKE_SWAPFILE_BLKS(SWAP_FILE, 65536)) tst_brk(TBROK, "Failed to create file for swap"); } diff --git a/testcases/kernel/syscalls/swapoff/swapoff02.c b/testcases/kernel/syscalls/swapoff/swapoff02.c index 52906848f..61536dda4 100644 --- a/testcases/kernel/syscalls/swapoff/swapoff02.c +++ b/testcases/kernel/syscalls/swapoff/swapoff02.c @@ -88,7 +88,7 @@ static void setup(void) is_swap_supported(TEST_FILE); - if (make_swapfile(SWAP_FILE, 10, 1)) + if (SAFE_MAKE_SWAPFILE_BLKS(SWAP_FILE, 10)) tst_brk(TBROK, "Failed to create file for swap"); } diff --git a/testcases/kernel/syscalls/swapon/swapon01.c b/testcases/kernel/syscalls/swapon/swapon01.c index d406e4bd9..b986d97c4 100644 --- a/testcases/kernel/syscalls/swapon/swapon01.c +++ b/testcases/kernel/syscalls/swapon/swapon01.c @@ -38,7 +38,7 @@ static void verify_swapon(void) static void setup(void) { is_swap_supported(SWAP_FILE); - make_swapfile(SWAP_FILE, 10, 0); + MAKE_SWAPFILE_BLKS(SWAP_FILE, 10); SAFE_CG_PRINTF(tst_cg, "cgroup.procs", "%d", getpid()); SAFE_CG_PRINTF(tst_cg, "memory.max", "%lu", TESTMEM); diff --git a/testcases/kernel/syscalls/swapon/swapon02.c b/testcases/kernel/syscalls/swapon/swapon02.c index 7e876d26a..e5e29b8e7 100644 --- a/testcases/kernel/syscalls/swapon/swapon02.c +++ b/testcases/kernel/syscalls/swapon/swapon02.c @@ -50,8 +50,8 @@ static void setup(void) is_swap_supported(TEST_FILE); SAFE_TOUCH(NOTSWAP_FILE, 0777, NULL); - make_swapfile(SWAP_FILE, 10, 0); - make_swapfile(USED_FILE, 10, 0); + MAKE_SWAPFILE_BLKS(SWAP_FILE, 10); + MAKE_SWAPFILE_BLKS(USED_FILE, 10); if (tst_syscall(__NR_swapon, USED_FILE, 0)) tst_res(TWARN | TERRNO, "swapon(alreadyused) failed"); diff --git a/testcases/kernel/syscalls/swapon/swapon03.c b/testcases/kernel/syscalls/swapon/swapon03.c index 6f47fc01f..5295a6a73 100644 --- a/testcases/kernel/syscalls/swapon/swapon03.c +++ b/testcases/kernel/syscalls/swapon/swapon03.c @@ -49,7 +49,7 @@ static int setup_swap(void) /* Create the swapfile */ snprintf(filename, sizeof(filename), "%s%02d", TEST_FILE, j + 2); - make_swapfile(filename, 10, 0); + MAKE_SWAPFILE_BLKS(filename, 10); /* turn on the swap file */ TST_EXP_PASS_SILENT(swapon(filename, 0)); @@ -62,7 +62,7 @@ static int setup_swap(void) tst_brk(TFAIL, "Failed to setup swap files"); tst_res(TINFO, "Successfully created %d swap files", swapfiles); - make_swapfile(TEST_FILE, 10, 0); + MAKE_SWAPFILE_BLKS(TEST_FILE, 10); return 0; }