| Message ID | 20240123114840.2610533-5-liwang@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | improvement work on libswap library | expand |
Hi Li,
swapoff01 fails on TMPDIR on btrfs (regardless kernel version):
# ./swapoff01
rm -f -f -r swapoff01 swapoff02 *.o *.pyc .cache.mk *.dwo .*.dwo
BUILD libltpswap.a
make[1]: Nothing to be done for 'all'.
CC testcases/kernel/syscalls/swapoff/swapoff01
CC testcases/kernel/syscalls/swapoff/swapoff02
tst_test.c:1709: TINFO: LTP version: 20230929-295-gc20ab499a
tst_test.c:1595: TINFO: Timeout per run is 0h 00m 30s
tst_ioctl.c:21: TINFO: FIBMAP ioctl is NOT supported: EINVAL (22)
libswap.c:33: TINFO: FS_NOCOW_FL attribute set on ./tstswap
swapoff01.c:24: TFAIL: Failed to turn on the swap file, skipping test iteration: EINVAL (22)
=> I guess we would need to replace tst_fill_file() with
prealloc_contiguous_file() (which is not public), or use make_swapfile()
directly. But here we create file first with 65536 blocks and make_swapfile()
creates 10 block file (with prealloc_contiguous_file() or previously also with
tst_fill_file()).
Kind regards,
Petr
--- testcases/kernel/syscalls/swapoff/swapoff01.c
+++ testcases/kernel/syscalls/swapoff/swapoff01.c
@@ -44,11 +44,8 @@ static void setup(void)
tst_brk(TBROK,
"Insufficient disk space to create swap file");
- if (tst_fill_file("swapfile01", 0x00, 1024, 65536))
+ if (make_swapfile("swapfile01", 1))
tst_brk(TBROK, "Failed to create file for swap");
-
- if (system("mkswap swapfile01 > tmpfile 2>&1") != 0)
- tst_brk(TBROK, "Failed to make swapfile");
}
Hi Li, > Hi Li, > swapoff01 fails on TMPDIR on btrfs (regardless kernel version): FYI it works on Tumbleweed with 6.7 kernel. It's broken on some older SLES with 5.14 and all older kernels. I suppose with nocow (fixes I suggested previously) would work as expected (TPASS, or TCONF on kernel < 5.0). Kind regards, Petr > # ./swapoff01 > rm -f -f -r swapoff01 swapoff02 *.o *.pyc .cache.mk *.dwo .*.dwo > BUILD libltpswap.a > make[1]: Nothing to be done for 'all'. > CC testcases/kernel/syscalls/swapoff/swapoff01 > CC testcases/kernel/syscalls/swapoff/swapoff02 > tst_test.c:1709: TINFO: LTP version: 20230929-295-gc20ab499a > tst_test.c:1595: TINFO: Timeout per run is 0h 00m 30s > tst_ioctl.c:21: TINFO: FIBMAP ioctl is NOT supported: EINVAL (22) > libswap.c:33: TINFO: FS_NOCOW_FL attribute set on ./tstswap > swapoff01.c:24: TFAIL: Failed to turn on the swap file, skipping test iteration: EINVAL (22) > => I guess we would need to replace tst_fill_file() with > prealloc_contiguous_file() (which is not public), or use make_swapfile() > directly. But here we create file first with 65536 blocks and make_swapfile() > creates 10 block file (with prealloc_contiguous_file() or previously also with > tst_fill_file()). > Kind regards, > Petr > --- testcases/kernel/syscalls/swapoff/swapoff01.c > +++ testcases/kernel/syscalls/swapoff/swapoff01.c > @@ -44,11 +44,8 @@ static void setup(void) > tst_brk(TBROK, > "Insufficient disk space to create swap file"); > - if (tst_fill_file("swapfile01", 0x00, 1024, 65536)) > + if (make_swapfile("swapfile01", 1)) > tst_brk(TBROK, "Failed to create file for swap"); > - > - if (system("mkswap swapfile01 > tmpfile 2>&1") != 0) > - tst_brk(TBROK, "Failed to make swapfile"); > }
Hi Petr, On Tue, Jan 23, 2024 at 8:37 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > Hi Li, > > > swapoff01 fails on TMPDIR on btrfs (regardless kernel version): > > FYI it works on Tumbleweed with 6.7 kernel. It's broken on some older SLES > with 5.14 and all older kernels. I suppose with nocow (fixes I suggested > previously) would work as expected (TPASS, or TCONF on kernel < 5.0). > You're right. We have to guarantee the swapfile is a contiguous file whatever the FS type is. So here making use of make_swapfile() is a hard requirement. And, I don't think the file first with 65536 blocks (in swapoff01) is not necessary. > Kind regards, > Petr > > > # ./swapoff01 > > rm -f -f -r swapoff01 swapoff02 *.o *.pyc .cache.mk *.dwo .*.dwo > > BUILD libltpswap.a > > make[1]: Nothing to be done for 'all'. > > CC testcases/kernel/syscalls/swapoff/swapoff01 > > CC testcases/kernel/syscalls/swapoff/swapoff02 > > tst_test.c:1709: TINFO: LTP version: 20230929-295-gc20ab499a > > tst_test.c:1595: TINFO: Timeout per run is 0h 00m 30s > > tst_ioctl.c:21: TINFO: FIBMAP ioctl is NOT supported: EINVAL (22) > > libswap.c:33: TINFO: FS_NOCOW_FL attribute set on ./tstswap > > swapoff01.c:24: TFAIL: Failed to turn on the swap file, skipping test > iteration: EINVAL (22) > > > => I guess we would need to replace tst_fill_file() with > > prealloc_contiguous_file() (which is not public), or use make_swapfile() > > directly. But here we create file first with 65536 blocks and > make_swapfile() > > creates 10 block file (with prealloc_contiguous_file() or previously > also with > > tst_fill_file()). > > > Kind regards, > > Petr > > > --- testcases/kernel/syscalls/swapoff/swapoff01.c > > +++ testcases/kernel/syscalls/swapoff/swapoff01.c > > @@ -44,11 +44,8 @@ static void setup(void) > > tst_brk(TBROK, > > "Insufficient disk space to create swap file"); > > > - if (tst_fill_file("swapfile01", 0x00, 1024, 65536)) > > + if (make_swapfile("swapfile01", 1)) > > tst_brk(TBROK, "Failed to create file for swap"); > > - > > - if (system("mkswap swapfile01 > tmpfile 2>&1") != 0) > > - tst_brk(TBROK, "Failed to make swapfile"); > > } > > >
> Hi Petr, > On Tue, Jan 23, 2024 at 8:37 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, > > > Hi Li, > > > swapoff01 fails on TMPDIR on btrfs (regardless kernel version): > > FYI it works on Tumbleweed with 6.7 kernel. It's broken on some older SLES > > with 5.14 and all older kernels. I suppose with nocow (fixes I suggested > > previously) would work as expected (TPASS, or TCONF on kernel < 5.0). > You're right. > We have to guarantee the swapfile is a contiguous file whatever the FS type > is. > So here making use of make_swapfile() is a hard requirement. > And, I don't think the file first with 65536 blocks (in swapoff01) is not > necessary. Maybe not, but now we test on single swap size. Testing small swap and big swap was IMHO more testing coverage (various filesystems behave differently on different size), but given this would be more important for whole .all_filesystems = 1 testing I'm ok with the change. Kind regards, Petr > > Kind regards, > > Petr > > > # ./swapoff01 > > > rm -f -f -r swapoff01 swapoff02 *.o *.pyc .cache.mk *.dwo .*.dwo > > > BUILD libltpswap.a > > > make[1]: Nothing to be done for 'all'. > > > CC testcases/kernel/syscalls/swapoff/swapoff01 > > > CC testcases/kernel/syscalls/swapoff/swapoff02 > > > tst_test.c:1709: TINFO: LTP version: 20230929-295-gc20ab499a > > > tst_test.c:1595: TINFO: Timeout per run is 0h 00m 30s > > > tst_ioctl.c:21: TINFO: FIBMAP ioctl is NOT supported: EINVAL (22) > > > libswap.c:33: TINFO: FS_NOCOW_FL attribute set on ./tstswap > > > swapoff01.c:24: TFAIL: Failed to turn on the swap file, skipping test > > iteration: EINVAL (22) > > > => I guess we would need to replace tst_fill_file() with > > > prealloc_contiguous_file() (which is not public), or use make_swapfile() > > > directly. But here we create file first with 65536 blocks and > > make_swapfile() > > > creates 10 block file (with prealloc_contiguous_file() or previously > > also with > > > tst_fill_file()). > > > Kind regards, > > > Petr > > > --- testcases/kernel/syscalls/swapoff/swapoff01.c > > > +++ testcases/kernel/syscalls/swapoff/swapoff01.c > > > @@ -44,11 +44,8 @@ static void setup(void) > > > tst_brk(TBROK, > > > "Insufficient disk space to create swap file"); > > > - if (tst_fill_file("swapfile01", 0x00, 1024, 65536)) > > > + if (make_swapfile("swapfile01", 1)) > > > tst_brk(TBROK, "Failed to create file for swap"); > > > - > > > - if (system("mkswap swapfile01 > tmpfile 2>&1") != 0) > > > - tst_brk(TBROK, "Failed to make swapfile"); > > > }
Hi Li, > Hi Petr, > On Tue, Jan 23, 2024 at 8:37 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Li, > > > Hi Li, > > > swapoff01 fails on TMPDIR on btrfs (regardless kernel version): > > FYI it works on Tumbleweed with 6.7 kernel. It's broken on some older SLES > > with 5.14 and all older kernels. I suppose with nocow (fixes I suggested > > previously) would work as expected (TPASS, or TCONF on kernel < 5.0). > You're right. > We have to guarantee the swapfile is a contiguous file whatever the FS type > is. > So here making use of make_swapfile() is a hard requirement. > And, I don't think the file first with 65536 blocks (in swapoff01) is not > necessary. Unfortunately this commit or the following (libswap: Introduce file contiguity check) breaks swapon01.c on older SLES (4.4 based kernel and older) on XFS: tst_ioctl.c:26: TINFO: FIBMAP ioctl is supported libswap.c:191: TFAIL: swapon() on xfs failed: EINVAL (22) The failure is in is_swap_supported(). I'll try to give more info tomorrow. Kind regards, Petr tst_test.c:1709: TINFO: LTP version: 20230929 tst_test.c:1595: TINFO: Timeout per run is 0h 00m 30s tst_supported_fs_types.c:97: TINFO: Kernel supports ext2 tst_supported_fs_types.c:62: TINFO: mkfs.ext2 does exist tst_supported_fs_types.c:97: TINFO: Kernel supports ext3 tst_supported_fs_types.c:62: TINFO: mkfs.ext3 does exist tst_supported_fs_types.c:97: TINFO: Kernel supports ext4 tst_supported_fs_types.c:62: TINFO: mkfs.ext4 does exist tst_supported_fs_types.c:97: TINFO: Kernel supports xfs tst_supported_fs_types.c:62: TINFO: mkfs.xfs does exist tst_supported_fs_types.c:97: TINFO: Kernel supports btrfs tst_supported_fs_types.c:62: TINFO: mkfs.btrfs does exist tst_supported_fs_types.c:105: TINFO: Skipping bcachefs because of FUSE blacklist tst_supported_fs_types.c:97: TINFO: Kernel supports vfat tst_supported_fs_types.c:62: TINFO: mkfs.vfat does exist tst_supported_fs_types.c:128: TINFO: Filesystem exfat is not supported tst_supported_fs_types.c:128: TINFO: Filesystem ntfs is not supported tst_supported_fs_types.c:97: TINFO: Kernel supports tmpfs tst_supported_fs_types.c:49: TINFO: mkfs is not needed for tmpfs tst_test.c:1669: TINFO: === Testing on ext2 === tst_test.c:1118: TINFO: Formatting /dev/loop0 with ext2 opts='' extra opts='' mke2fs 1.42.11 (09-Jul-2014) tst_test.c:1132: TINFO: Mounting /dev/loop0 to /tmp/LTP_swa4rYYz4/mntpoint fstyp=ext2 flags=0 tst_ioctl.c:26: TINFO: FIBMAP ioctl is supported swapon01.c:27: TPASS: tst_syscall(__NR_swapon, SWAP_FILE, 0) passed swapon01.c:30: TINFO: SwapCached: 348 Kb tst_test.c:1669: TINFO: === Testing on ext3 === tst_test.c:1118: TINFO: Formatting /dev/loop0 with ext3 opts='' extra opts='' mke2fs 1.42.11 (09-Jul-2014) tst_test.c:1132: TINFO: Mounting /dev/loop0 to /tmp/LTP_swa4rYYz4/mntpoint fstyp=ext3 flags=0 tst_ioctl.c:26: TINFO: FIBMAP ioctl is supported swapon01.c:27: TPASS: tst_syscall(__NR_swapon, SWAP_FILE, 0) passed swapon01.c:30: TINFO: SwapCached: 136 Kb tst_test.c:1669: TINFO: === Testing on ext4 === tst_test.c:1118: TINFO: Formatting /dev/loop0 with ext4 opts='' extra opts='' mke2fs 1.42.11 (09-Jul-2014) tst_test.c:1132: TINFO: Mounting /dev/loop0 to /tmp/LTP_swa4rYYz4/mntpoint fstyp=ext4 flags=0 tst_ioctl.c:26: TINFO: FIBMAP ioctl is supported swapon01.c:27: TPASS: tst_syscall(__NR_swapon, SWAP_FILE, 0) passed swapon01.c:30: TINFO: SwapCached: 116 Kb tst_test.c:1669: TINFO: === Testing on xfs === tst_test.c:1118: TINFO: Formatting /dev/loop0 with xfs opts='' extra opts='' tst_test.c:1132: TINFO: Mounting /dev/loop0 to /tmp/LTP_swa4rYYz4/mntpoint fstyp=xfs flags=0 tst_ioctl.c:26: TINFO: FIBMAP ioctl is supported libswap.c:191: TFAIL: swapon() on xfs failed: EINVAL (22) Summary: passed 3 failed 1 broken 0 skipped 0 warnings 0
On Tue, Jan 23, 2024 at 11:47 PM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Petr, > > > On Tue, Jan 23, 2024 at 8:37 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > Hi Li, > > > > > Hi Li, > > > > > swapoff01 fails on TMPDIR on btrfs (regardless kernel version): > > > > FYI it works on Tumbleweed with 6.7 kernel. It's broken on some older > SLES > > > with 5.14 and all older kernels. I suppose with nocow (fixes I > suggested > > > previously) would work as expected (TPASS, or TCONF on kernel < 5.0). > > > > You're right. > > > We have to guarantee the swapfile is a contiguous file whatever the FS > type > > is. > > So here making use of make_swapfile() is a hard requirement. > > And, I don't think the file first with 65536 blocks (in swapoff01) is not > > necessary. > > Maybe not, but now we test on single swap size. Testing small swap and big > swap > was IMHO more testing coverage (various filesystems behave differently on > different size), but given this would be more important for whole > .all_filesystems = 1 testing I'm ok with the change. > Ok, that could be achieved by customizing the swap file size later. It's not very hard. But now I don't want to increase the patchset number just for more coverage, that will be a burden for release testing work.
Hi Petr, On Wed, Jan 24, 2024 at 1:40 AM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > Hi Petr, > > > On Tue, Jan 23, 2024 at 8:37 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > Hi Li, > > > > > Hi Li, > > > > > swapoff01 fails on TMPDIR on btrfs (regardless kernel version): > > > > FYI it works on Tumbleweed with 6.7 kernel. It's broken on some older > SLES > > > with 5.14 and all older kernels. I suppose with nocow (fixes I > suggested > > > previously) would work as expected (TPASS, or TCONF on kernel < 5.0). > > > > You're right. > > > We have to guarantee the swapfile is a contiguous file whatever the FS > type > > is. > > So here making use of make_swapfile() is a hard requirement. > > And, I don't think the file first with 65536 blocks (in swapoff01) is not > > necessary. > > Unfortunately this commit or the following (libswap: Introduce file > contiguity > check) breaks swapon01.c on older SLES (4.4 based kernel and older) on XFS: > > tst_ioctl.c:26: TINFO: FIBMAP ioctl is supported > libswap.c:191: TFAIL: swapon() on xfs failed: EINVAL (22) > > The failure is in is_swap_supported(). > Good catch. After testing on my side, reproduced that easily with old XFS. The reason is probably old XFS expects the swap file to be initialized in a certain way. So a simple fix is just to fill full of the file after preallocating space: --- a/libs/libltpswap/libswap.c +++ b/libs/libltpswap/libswap.c @@ -140,6 +140,11 @@ int make_swapfile(const char *swapfile, int safe) if (prealloc_contiguous_file(swapfile, sysconf(_SC_PAGESIZE), 10) != 0) tst_brk(TBROK, "Failed to create swapfile"); + if (tst_fs_type(swapfile) == TST_XFS_MAGIC) { + if (tst_fill_file(swapfile, 0, sysconf(_SC_PAGESIZE), 10) != 0) + tst_brk(TBROK, "Failed to create swapfile"); + } + /* make the file swapfile */ const char *argv[2 + 1];
> On Tue, Jan 23, 2024 at 11:47 PM Petr Vorel <pvorel@suse.cz> wrote: > > > Hi Petr, > > > On Tue, Jan 23, 2024 at 8:37 PM Petr Vorel <pvorel@suse.cz> wrote: > > > > Hi Li, > > > > > Hi Li, > > > > > swapoff01 fails on TMPDIR on btrfs (regardless kernel version): > > > > FYI it works on Tumbleweed with 6.7 kernel. It's broken on some older > > SLES > > > > with 5.14 and all older kernels. I suppose with nocow (fixes I > > suggested > > > > previously) would work as expected (TPASS, or TCONF on kernel < 5.0). > > > You're right. > > > We have to guarantee the swapfile is a contiguous file whatever the FS > > type > > > is. > > > So here making use of make_swapfile() is a hard requirement. > > > And, I don't think the file first with 65536 blocks (in swapoff01) is not > > > necessary. > > Maybe not, but now we test on single swap size. Testing small swap and big > > swap > > was IMHO more testing coverage (various filesystems behave differently on > > different size), but given this would be more important for whole > > .all_filesystems = 1 testing I'm ok with the change. > Ok, that could be achieved by customizing the swap file size later. > It's not very hard. But now I don't want to increase the patchset number > just for more coverage, that will be a burden for release testing work. Hi Li, +1, sure. Kind regards, Petr
diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c index 8c2ce6cd7..b253dbeec 100644 --- a/libs/libltpswap/libswap.c +++ b/libs/libltpswap/libswap.c @@ -4,6 +4,7 @@ * Author: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com> */ +#include <linux/fs.h> #include <errno.h> #define TST_NO_DEFAULT_MAIN @@ -13,6 +14,7 @@ #include "lapi/syscalls.h" static const char *const swap_supported_fs[] = { + "btrfs", "ext2", "ext3", "ext4", @@ -23,6 +25,50 @@ static const char *const swap_supported_fs[] = { NULL }; +static void set_nocow_attr(const char *filename) +{ + int fd; + int attrs; + + tst_res(TINFO, "FS_NOCOW_FL attribute set on %s", filename); + + fd = SAFE_OPEN(filename, O_RDONLY); + + SAFE_IOCTL(fd, FS_IOC_GETFLAGS, &attrs); + + attrs |= FS_NOCOW_FL; + + SAFE_IOCTL(fd, FS_IOC_SETFLAGS, &attrs); + + SAFE_CLOSE(fd); +} + +static int prealloc_contiguous_file(const char *path, size_t bs, size_t bcount) +{ + int fd; + + fd = open(path, O_CREAT|O_WRONLY|O_TRUNC, 0600); + if (fd < 0) + return -1; + + /* Btrfs file need set 'nocow' attribute */ + if (tst_fs_type(path) == TST_BTRFS_MAGIC) + set_nocow_attr(path); + + if (tst_prealloc_size_fd(fd, bs, bcount)) { + close(fd); + unlink(path); + return -1; + } + + if (close(fd) < 0) { + unlink(path); + return -1; + } + + return 0; +} + /* * Make a swap file */ @@ -32,7 +78,7 @@ int make_swapfile(const char *swapfile, int safe) tst_brk(TBROK, "Insufficient disk space to create swap file"); /* create file */ - if (tst_fill_file(swapfile, 0, sysconf(_SC_PAGESIZE), 10) != 0) + if (prealloc_contiguous_file(swapfile, sysconf(_SC_PAGESIZE), 10) != 0) tst_brk(TBROK, "Failed to create swapfile"); /* make the file swapfile */
The improve makes several key updates to the swap file handling in the libswap.c file: It incorporates support for the Btrfs filesystem, which is now recognized as a valid filesystem for swap files. A new function, set_nocow_attr, is added to apply the FS_NOCOW_FL flag to files on Btrfs filesystems. Introduces a new prealloc_contiguous_file function. This method preallocates a contiguous block of space for the swap file during its creation, rather than filling the file with data as was done previously. Modifications to the make_swapfile function are made to utilize prealloc_contiguous_file for creating the swap file, ensuring the file is created with contiguous space on the filesystem. Signed-off-by: Li Wang <liwang@redhat.com> --- libs/libltpswap/libswap.c | 48 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-)