| Message ID | 20240122072948.2568801-4-liwang@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/4] libswap: add known swap supported fs check | expand |
Hi Li, > The patch aims to ensure swap files on Btrfs filesystems are created > with the appropriate FS_NOCOW_FL attribute, which is necessary to > disable CoW (Copy-on-Write) for swap files, perthe btrfs(5) manual page. > This change is gated behind a kernel version check to ensure compatibility > with the system's capabilities. > Signed-off-by: Li Wang <liwang@redhat.com> > --- > libs/libltpswap/libswap.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > index 623f2fb3c..0b1d9a227 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 > @@ -23,11 +24,37 @@ static const char *const swap_supported_fs[] = { > NULL > }; > +static void set_nocow_attr(const char *filename) > +{ > + int fd; > + int attrs; > + > + fd = SAFE_OPEN(filename, O_RDONLY); > + > + if (ioctl(fd, FS_IOC_GETFLAGS, &attrs) == -1) { > + tst_res(TFAIL | TERRNO, "Error getting attributes"); I guess we don't want to break all testing due ioctl failure, right? Otherwise I would use SAFE_IOCTL(). > + close(fd); > + return; > + } > + > + attrs |= FS_NOCOW_FL; > + > + if (ioctl(fd, FS_IOC_SETFLAGS, &attrs) == -1) > + tst_res(TFAIL | TERRNO, "Error setting FS_NOCOW_FL attribute"); And here as well. > + else > + tst_res(TINFO, "FS_NOCOW_FL attribute set on %s\n", filename); This is redundant new line, please remove \n before merging. nit: make check reports various of formatting issues (on master there are only two). Would you dare to fix them? (some of them are added in the first commit). make check-libswap CHECK libs/libltpswap/libswap.c libswap.c:75: WARNING: Missing a blank line after declarations libswap.c:101: WARNING: please, no spaces at the start of a line libswap.c:102: WARNING: Missing a blank line after declarations libswap.c:102: WARNING: please, no spaces at the start of a line libswap.c:102: WARNING: suspect code indent for conditional statements (7, 15) libswap.c:103: ERROR: code indent should use tabs where possible libswap.c:103: WARNING: please, no spaces at the start of a line libswap.c:103: WARNING: suspect code indent for conditional statements (15, 23) libswap.c:104: ERROR: code indent should use tabs where possible libswap.c:104: WARNING: please, no spaces at the start of a line libswap.c:105: ERROR: code indent should use tabs where possible libswap.c:105: WARNING: please, no spaces at the start of a line libswap.c:105: WARNING: suspect code indent for conditional statements (15, 23) libswap.c:106: ERROR: code indent should use tabs where possible libswap.c:106: WARNING: please, no spaces at the start of a line libswap.c:107: WARNING: please, no spaces at the start of a line > + > + close(fd); > +} > + > /* > * Make a swap file > */ > int make_swapfile(const char *swapfile, int safe) > { > + long fs_type = tst_fs_type(swapfile); > + const char *fstype = tst_fs_type_name(fs_type); > + > if (!tst_fs_has_free(".", sysconf(_SC_PAGESIZE) * 10, TST_BYTES)) > tst_brk(TBROK, "Insufficient disk space to create swap file"); > @@ -35,6 +62,14 @@ int make_swapfile(const char *swapfile, int safe) > if (tst_fill_file(swapfile, 0, sysconf(_SC_PAGESIZE), 10) != 0) > tst_brk(TBROK, "Failed to create swapfile"); > + /* Btrfs file need set 'nocow' attribute */ > + if (strcmp(fstype, "btrfs") == 0) { Maybe using just fs_type (long), i.e. avoid conversion to char * via tst_fs_type_name()? Or am I missing something? long fs_type = tst_fs_type(swapfile); ... if (fs_type == TST_BTRFS_MAGIC) { Kind regards, Petr > + if (tst_kvercmp(5, 0, 0) > 0) > + set_nocow_attr(swapfile); > + else > + tst_brk(TCONF, "Swapfile on %s not implemented", fstype); > + } > + > /* make the file swapfile */ > const char *argv[2 + 1]; > argv[0] = "mkswap";
Hi Li, testing the patchset on SLES, at least swapon01 reports some results on systems with TMPDIR on tmpfs or btrfs. Therefore I agree it'd be good to use the same approach for all swapon* and swapoff* tests. I would be ok to get it to the release (generally patchset LGTM), but depends on your and Cyril's time (no problem to postpone it). Kind regards, Petr
Hi Petr, On Mon, Jan 22, 2024 at 4:40 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > > The patch aims to ensure swap files on Btrfs filesystems are created > > with the appropriate FS_NOCOW_FL attribute, which is necessary to > > disable CoW (Copy-on-Write) for swap files, perthe btrfs(5) manual page. > > This change is gated behind a kernel version check to ensure > compatibility > > with the system's capabilities. > > > Signed-off-by: Li Wang <liwang@redhat.com> > > --- > > libs/libltpswap/libswap.c | 35 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > > index 623f2fb3c..0b1d9a227 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 > > @@ -23,11 +24,37 @@ static const char *const swap_supported_fs[] = { > > NULL > > }; > > > +static void set_nocow_attr(const char *filename) > > +{ > > + int fd; > > + int attrs; > > + > > + fd = SAFE_OPEN(filename, O_RDONLY); > > + > > + if (ioctl(fd, FS_IOC_GETFLAGS, &attrs) == -1) { > > + tst_res(TFAIL | TERRNO, "Error getting attributes"); > I guess we don't want to break all testing due ioctl failure, right? > Otherwise I would use SAFE_IOCTL(). > +1, thanks. > > + close(fd); > > + return; > > + } > > + > > + attrs |= FS_NOCOW_FL; > > + > > + if (ioctl(fd, FS_IOC_SETFLAGS, &attrs) == -1) > > + tst_res(TFAIL | TERRNO, "Error setting FS_NOCOW_FL > attribute"); > And here as well. > +1 > > + else > > + tst_res(TINFO, "FS_NOCOW_FL attribute set on %s\n", > filename); > This is redundant new line, please remove \n before merging. > +1, I would send a V2 for 4/4, because I found additional issue that the "btrfs" should be added in swap_supported_fs[] as well. > nit: make check reports various of formatting issues (on master there are > only > two). Would you dare to fix them? (some of them are added in the first > commit). > +1 > make check-libswap > CHECK libs/libltpswap/libswap.c > libswap.c:75: WARNING: Missing a blank line after declarations > libswap.c:101: WARNING: please, no spaces at the start of a line > libswap.c:102: WARNING: Missing a blank line after declarations > libswap.c:102: WARNING: please, no spaces at the start of a line > libswap.c:102: WARNING: suspect code indent for conditional statements (7, > 15) > libswap.c:103: ERROR: code indent should use tabs where possible > libswap.c:103: WARNING: please, no spaces at the start of a line > libswap.c:103: WARNING: suspect code indent for conditional statements > (15, 23) > libswap.c:104: ERROR: code indent should use tabs where possible > libswap.c:104: WARNING: please, no spaces at the start of a line > libswap.c:105: ERROR: code indent should use tabs where possible > libswap.c:105: WARNING: please, no spaces at the start of a line > libswap.c:105: WARNING: suspect code indent for conditional statements > (15, 23) > libswap.c:106: ERROR: code indent should use tabs where possible > libswap.c:106: WARNING: please, no spaces at the start of a line > libswap.c:107: WARNING: please, no spaces at the start of a line > > > > + > > + close(fd); > > +} > > + > > /* > > * Make a swap file > > */ > > int make_swapfile(const char *swapfile, int safe) > > { > > + long fs_type = tst_fs_type(swapfile); > > + const char *fstype = tst_fs_type_name(fs_type); > > + > > if (!tst_fs_has_free(".", sysconf(_SC_PAGESIZE) * 10, TST_BYTES)) > > tst_brk(TBROK, "Insufficient disk space to create swap > file"); > > > @@ -35,6 +62,14 @@ int make_swapfile(const char *swapfile, int safe) > > if (tst_fill_file(swapfile, 0, sysconf(_SC_PAGESIZE), 10) != 0) > > tst_brk(TBROK, "Failed to create swapfile"); > > > + /* Btrfs file need set 'nocow' attribute */ > > + if (strcmp(fstype, "btrfs") == 0) { > > Maybe using just fs_type (long), i.e. avoid conversion to char * via > tst_fs_type_name()? Or am I missing something? > > long fs_type = tst_fs_type(swapfile); > ... > if (fs_type == TST_BTRFS_MAGIC) { > Good point. > > Kind regards, > Petr > > > + if (tst_kvercmp(5, 0, 0) > 0) > > + set_nocow_attr(swapfile); > > + else > > + tst_brk(TCONF, "Swapfile on %s not implemented", > fstype); > > + } > > + > > /* make the file swapfile */ > > const char *argv[2 + 1]; > > argv[0] = "mkswap"; > >
On Mon, Jan 22, 2024 at 4:47 PM Petr Vorel <pvorel@suse.cz> wrote: > Hi Li, > > testing the patchset on SLES, at least swapon01 reports some results > on systems with TMPDIR on tmpfs or btrfs. > > Therefore I agree it'd be good to use the same approach for all swapon* and > swapoff* tests. > > I would be ok to get it to the release (generally patchset LGTM), but > depends on > your and Cyril's time (no problem to postpone it). > Thanks a lot. I can send out the V2 by the end of today.
diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c index 623f2fb3c..0b1d9a227 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 @@ -23,11 +24,37 @@ static const char *const swap_supported_fs[] = { NULL }; +static void set_nocow_attr(const char *filename) +{ + int fd; + int attrs; + + fd = SAFE_OPEN(filename, O_RDONLY); + + if (ioctl(fd, FS_IOC_GETFLAGS, &attrs) == -1) { + tst_res(TFAIL | TERRNO, "Error getting attributes"); + close(fd); + return; + } + + attrs |= FS_NOCOW_FL; + + if (ioctl(fd, FS_IOC_SETFLAGS, &attrs) == -1) + tst_res(TFAIL | TERRNO, "Error setting FS_NOCOW_FL attribute"); + else + tst_res(TINFO, "FS_NOCOW_FL attribute set on %s\n", filename); + + close(fd); +} + /* * Make a swap file */ int make_swapfile(const char *swapfile, int safe) { + long fs_type = tst_fs_type(swapfile); + const char *fstype = tst_fs_type_name(fs_type); + if (!tst_fs_has_free(".", sysconf(_SC_PAGESIZE) * 10, TST_BYTES)) tst_brk(TBROK, "Insufficient disk space to create swap file"); @@ -35,6 +62,14 @@ int make_swapfile(const char *swapfile, int safe) if (tst_fill_file(swapfile, 0, sysconf(_SC_PAGESIZE), 10) != 0) tst_brk(TBROK, "Failed to create swapfile"); + /* Btrfs file need set 'nocow' attribute */ + if (strcmp(fstype, "btrfs") == 0) { + if (tst_kvercmp(5, 0, 0) > 0) + set_nocow_attr(swapfile); + else + tst_brk(TCONF, "Swapfile on %s not implemented", fstype); + } + /* make the file swapfile */ const char *argv[2 + 1]; argv[0] = "mkswap";
The patch aims to ensure swap files on Btrfs filesystems are created with the appropriate FS_NOCOW_FL attribute, which is necessary to disable CoW (Copy-on-Write) for swap files, perthe btrfs(5) manual page. This change is gated behind a kernel version check to ensure compatibility with the system's capabilities. Signed-off-by: Li Wang <liwang@redhat.com> --- libs/libltpswap/libswap.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)