| Message ID | 1528806239-14297-1-git-send-email-liwang@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | swapon: commit swapfile caches to disk | expand |
Hi! > Testcase include swapon(8) fails on mainline kernel-v4.17+ as: > swapon01 1 TFAIL : swapon01.c:47: Failed to turn on swapfile: TEST_ERRNO=EINVAL(22): Invalid argument > swapon02 4 TFAIL : swapon02.c:91: swapon(2) failed to produce expected error: 16, errno: EBUSY and got 22. > swapon03 1 TFAIL : swapon03.c:243: Failed to setup swaps > swapoff01 1 TBROK : swapoff01.c:64: Failed to turn on the swap file, skipping test iteration 4.17+ means that this is yet to be release kernel, right? In that situation we wait if the change will survive upon the official release before changing the LTP test. > diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c b/testcases/kernel/syscalls/swapoff/swapoff01.c > index a63e661..b587b0d 100644 > --- a/testcases/kernel/syscalls/swapoff/swapoff01.c > +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c > @@ -105,6 +105,8 @@ static void setup(void) > > if (system("mkswap swapfile01 > tmpfile 2>&1") != 0) > tst_brkm(TBROK, cleanup, "Failed to make swapfile"); > + > + sync(); Can we rather use fsync() and/or fdatasync()? I would like the avoid the system wide sync() here since that may slow down the test unnecessarily.
----- Original Message ----- > Testcase include swapon(8) fails on mainline kernel-v4.17+ as: > swapon01 1 TFAIL : swapon01.c:47: Failed to turn on swapfile: > TEST_ERRNO=EINVAL(22): Invalid argument > swapon02 4 TFAIL : swapon02.c:91: swapon(2) failed to produce > expected error: 16, errno: EBUSY and got 22. > swapon03 1 TFAIL : swapon03.c:243: Failed to setup swaps > swapoff01 1 TBROK : swapoff01.c:64: Failed to turn on the swap file, > skipping test iteration > > And dmesg shows that: > [ 128.698981] swapon: file is not committed > > The error located in IOMAP_F_DIRTY(linux/fs/iomap.c) checking, which means > this swapfile have uncommitted metadata in caches. After adding sync() to > the end of all mkswap opertaion, these errors were gone. > > linux/fs/iomap.c: > static loff_t iomap_swapfile_activate_actor( > struct inode *inode, > loff_t pos, > loff_t count, > void *data, > struct iomap *iomap) > { > struct iomap_swapfile_info *isi = data; > int error; > ... > > /* No uncommitted metadata or shared blocks. */ > if (iomap->flags & IOMAP_F_DIRTY) { > pr_err("swapon: file is not committed\n"); > return -EINVAL; > } > ... > } > } > Signed-off-by: Li Wang <liwang@redhat.com> > --- > > Notes: > The new changes about swapfile activation function were merged in > kernel-4.17 > recently, so we didn't hit this failures before. Comment/code in mkswap, that has been present for years: /* * A subsequent swapon() will fail if the signature * is not actually on disk. (This is a kernel bug.) * The fsync() in close_fd() will take care of writing. */ I'm assuming your mkswap is recent enough. Any idea why fsync() in mkswap is no longer enough? Regards, Jan > > testcases/kernel/syscalls/swapoff/swapoff01.c | 2 ++ > testcases/kernel/syscalls/swapoff/swapoff02.c | 2 ++ > testcases/kernel/syscalls/swapon/libswapon.c | 2 ++ > 3 files changed, 6 insertions(+) > > diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c > b/testcases/kernel/syscalls/swapoff/swapoff01.c > index a63e661..b587b0d 100644 > --- a/testcases/kernel/syscalls/swapoff/swapoff01.c > +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c > @@ -105,6 +105,8 @@ static void setup(void) > > if (system("mkswap swapfile01 > tmpfile 2>&1") != 0) > tst_brkm(TBROK, cleanup, "Failed to make swapfile"); > + > + sync(); > } > > static void cleanup(void) > diff --git a/testcases/kernel/syscalls/swapoff/swapoff02.c > b/testcases/kernel/syscalls/swapoff/swapoff02.c > index b5c6312..d8c5889 100644 > --- a/testcases/kernel/syscalls/swapoff/swapoff02.c > +++ b/testcases/kernel/syscalls/swapoff/swapoff02.c > @@ -154,6 +154,8 @@ static void setup(void) > > if (tst_fill_file("./swapfile01", 0x00, 1024, 1)) > tst_brkm(TBROK, cleanup, "Failed to create swapfile"); > + > + sync(); > } > > static void cleanup(void) > diff --git a/testcases/kernel/syscalls/swapon/libswapon.c > b/testcases/kernel/syscalls/swapon/libswapon.c > index cf6a988..49fa5ed 100644 > --- a/testcases/kernel/syscalls/swapon/libswapon.c > +++ b/testcases/kernel/syscalls/swapon/libswapon.c > @@ -46,4 +46,6 @@ void make_swapfile(void (cleanup)(void), const char > *swapfile) > argv[2] = NULL; > > tst_run_cmd(cleanup, argv, "/dev/null", "/dev/null", 0); > + > + sync(); > } > -- > 1.9.3 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp >
On Tue, Jun 12, 2018 at 8:48 PM, Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > Testcase include swapon(8) fails on mainline kernel-v4.17+ as: > > swapon01 1 TFAIL : swapon01.c:47: Failed to turn on swapfile: > TEST_ERRNO=EINVAL(22): Invalid argument > > swapon02 4 TFAIL : swapon02.c:91: swapon(2) failed to produce > expected error: 16, errno: EBUSY and got 22. > > swapon03 1 TFAIL : swapon03.c:243: Failed to setup swaps > > swapoff01 1 TBROK : swapoff01.c:64: Failed to turn on the swap > file, skipping test iteration > > 4.17+ means that this is yet to be release kernel, right? In that > situation we wait if the change will survive upon the official release > before changing the LTP test. > You are right, I'm OK to wait. > > > diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c > b/testcases/kernel/syscalls/swapoff/swapoff01.c > > index a63e661..b587b0d 100644 > > --- a/testcases/kernel/syscalls/swapoff/swapoff01.c > > +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c > > @@ -105,6 +105,8 @@ static void setup(void) > > > > if (system("mkswap swapfile01 > tmpfile 2>&1") != 0) > > tst_brkm(TBROK, cleanup, "Failed to make swapfile"); > > + > > + sync(); > > Can we rather use fsync() and/or fdatasync()? I would like the avoid the > system wide sync() here since that may slow down the test unnecessarily. > > Actually I tried to add fsync() and syncfs() in tst_fill_file.c first, but it does NOT works for me. After tracking more, I found that we have to sync up caches in front of the command `mkswap swapfile`. > -- > Cyril Hrubis > chrubis@suse.cz >
On Tue, Jun 12, 2018 at 8:56 PM, Jan Stancek <jstancek@redhat.com> wrote: > > ----- Original Message ----- > > Testcase include swapon(8) fails on mainline kernel-v4.17+ as: > > swapon01 1 TFAIL : swapon01.c:47: Failed to turn on swapfile: > > TEST_ERRNO=EINVAL(22): Invalid argument > > swapon02 4 TFAIL : swapon02.c:91: swapon(2) failed to produce > > expected error: 16, errno: EBUSY and got 22. > > swapon03 1 TFAIL : swapon03.c:243: Failed to setup swaps > > swapoff01 1 TBROK : swapoff01.c:64: Failed to turn on the swap > file, > > skipping test iteration > > > > And dmesg shows that: > > [ 128.698981] swapon: file is not committed > > > > The error located in IOMAP_F_DIRTY(linux/fs/iomap.c) checking, which > means > > this swapfile have uncommitted metadata in caches. After adding sync() to > > the end of all mkswap opertaion, these errors were gone. > > > > linux/fs/iomap.c: > > static loff_t iomap_swapfile_activate_actor( > > struct inode *inode, > > loff_t pos, > > loff_t count, > > void *data, > > struct iomap *iomap) > > { > > struct iomap_swapfile_info *isi = data; > > int error; > > ... > > > > /* No uncommitted metadata or shared blocks. */ > > if (iomap->flags & IOMAP_F_DIRTY) { > > pr_err("swapon: file is not committed\n"); > > return -EINVAL; > > } > > ... > > } > > } > > Signed-off-by: Li Wang <liwang@redhat.com> > > --- > > > > Notes: > > The new changes about swapfile activation function were merged in > > kernel-4.17 > > recently, so we didn't hit this failures before. > > Comment/code in mkswap, that has been present for years: > /* > * A subsequent swapon() will fail if the signature > * is not actually on disk. (This is a kernel bug.) > * The fsync() in close_fd() will take care of writing. > */ > > I'm assuming your mkswap is recent enough. Any idea why fsync() > in mkswap is no longer enough? > Good catch, Jan. It sounds like a mkswap/fsync issue. Both of my mkswap failed to sync that, let me re-check it later. # rpm -qf /usr/sbin/mkswap util-linux-2.23.2-43.el7.x86_64 # rpm -qf /usr/sbin/mkswap util-linux-2.23.2-52.el7.x86_64 > > Regards, > Jan > > > > > testcases/kernel/syscalls/swapoff/swapoff01.c | 2 ++ > > testcases/kernel/syscalls/swapoff/swapoff02.c | 2 ++ > > testcases/kernel/syscalls/swapon/libswapon.c | 2 ++ > > 3 files changed, 6 insertions(+) > > > > diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c > > b/testcases/kernel/syscalls/swapoff/swapoff01.c > > index a63e661..b587b0d 100644 > > --- a/testcases/kernel/syscalls/swapoff/swapoff01.c > > +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c > > @@ -105,6 +105,8 @@ static void setup(void) > > > > if (system("mkswap swapfile01 > tmpfile 2>&1") != 0) > > tst_brkm(TBROK, cleanup, "Failed to make swapfile"); > > + > > + sync(); > > } > > > > static void cleanup(void) > > diff --git a/testcases/kernel/syscalls/swapoff/swapoff02.c > > b/testcases/kernel/syscalls/swapoff/swapoff02.c > > index b5c6312..d8c5889 100644 > > --- a/testcases/kernel/syscalls/swapoff/swapoff02.c > > +++ b/testcases/kernel/syscalls/swapoff/swapoff02.c > > @@ -154,6 +154,8 @@ static void setup(void) > > > > if (tst_fill_file("./swapfile01", 0x00, 1024, 1)) > > tst_brkm(TBROK, cleanup, "Failed to create swapfile"); > > + > > + sync(); > > } > > > > static void cleanup(void) > > diff --git a/testcases/kernel/syscalls/swapon/libswapon.c > > b/testcases/kernel/syscalls/swapon/libswapon.c > > index cf6a988..49fa5ed 100644 > > --- a/testcases/kernel/syscalls/swapon/libswapon.c > > +++ b/testcases/kernel/syscalls/swapon/libswapon.c > > @@ -46,4 +46,6 @@ void make_swapfile(void (cleanup)(void), const char > > *swapfile) > > argv[2] = NULL; > > > > tst_run_cmd(cleanup, argv, "/dev/null", "/dev/null", 0); > > + > > + sync(); > > } > > -- > > 1.9.3 > > > > > > -- > > Mailing list info: https://lists.linux.it/listinfo/ltp > > >
On Tue, Jun 12, 2018 at 9:05 PM, Li Wang <liwang@redhat.com> wrote: > > > On Tue, Jun 12, 2018 at 8:48 PM, Cyril Hrubis <chrubis@suse.cz> wrote: > >> Hi! >> > Testcase include swapon(8) fails on mainline kernel-v4.17+ as: >> > swapon01 1 TFAIL : swapon01.c:47: Failed to turn on swapfile: >> TEST_ERRNO=EINVAL(22): Invalid argument >> > swapon02 4 TFAIL : swapon02.c:91: swapon(2) failed to produce >> expected error: 16, errno: EBUSY and got 22. >> > swapon03 1 TFAIL : swapon03.c:243: Failed to setup swaps >> > swapoff01 1 TBROK : swapoff01.c:64: Failed to turn on the swap >> file, skipping test iteration >> >> 4.17+ means that this is yet to be release kernel, right? In that >> situation we wait if the change will survive upon the official release >> before changing the LTP test. >> > > You are right, I'm OK to wait. > > > >> >> > diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c >> b/testcases/kernel/syscalls/swapoff/swapoff01.c >> > index a63e661..b587b0d 100644 >> > --- a/testcases/kernel/syscalls/swapoff/swapoff01.c >> > +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c >> > @@ -105,6 +105,8 @@ static void setup(void) >> > >> > if (system("mkswap swapfile01 > tmpfile 2>&1") != 0) >> > tst_brkm(TBROK, cleanup, "Failed to make swapfile"); >> > + >> > + sync(); >> >> Can we rather use fsync() and/or fdatasync()? I would like the avoid the >> system wide sync() here since that may slow down the test unnecessarily. >> >> > Actually I tried to add fsync() and syncfs() in tst_fill_file.c first, > but it does NOT works for me. After tracking more, I found that we have to > sync up caches in front of the command `mkswap swapfile`. > Oops, this is wrong description, not in front of that. Should be done sync again after perform command `mkswap swapfile`. > > >> -- >> Cyril Hrubis >> chrubis@suse.cz >> > > > > -- > Regards, > Li Wang >
On Tue, Jun 12, 2018 at 9:12 PM, Li Wang <liwang@redhat.com> wrote: > > > > On Tue, Jun 12, 2018 at 8:56 PM, Jan Stancek <jstancek@redhat.com> wrote: >> >> >> ----- Original Message ----- >> > Testcase include swapon(8) fails on mainline kernel-v4.17+ as: >> > swapon01 1 TFAIL : swapon01.c:47: Failed to turn on swapfile: >> > TEST_ERRNO=EINVAL(22): Invalid argument >> > swapon02 4 TFAIL : swapon02.c:91: swapon(2) failed to produce >> > expected error: 16, errno: EBUSY and got 22. >> > swapon03 1 TFAIL : swapon03.c:243: Failed to setup swaps >> > swapoff01 1 TBROK : swapoff01.c:64: Failed to turn on the swap file, >> > skipping test iteration >> > >> > And dmesg shows that: >> > [ 128.698981] swapon: file is not committed >> > >> > The error located in IOMAP_F_DIRTY(linux/fs/iomap.c) checking, which means >> > this swapfile have uncommitted metadata in caches. After adding sync() to >> > the end of all mkswap opertaion, these errors were gone. >> > >> > linux/fs/iomap.c: >> > static loff_t iomap_swapfile_activate_actor( >> > struct inode *inode, >> > loff_t pos, >> > loff_t count, >> > void *data, >> > struct iomap *iomap) >> > { >> > struct iomap_swapfile_info *isi = data; >> > int error; >> > ... >> > >> > /* No uncommitted metadata or shared blocks. */ >> > if (iomap->flags & IOMAP_F_DIRTY) { >> > pr_err("swapon: file is not committed\n"); >> > return -EINVAL; >> > } >> > ... >> > } >> > } >> > Signed-off-by: Li Wang <liwang@redhat.com> >> > --- >> > >> > Notes: >> > The new changes about swapfile activation function were merged in >> > kernel-4.17 >> > recently, so we didn't hit this failures before. >> >> Comment/code in mkswap, that has been present for years: >> /* >> * A subsequent swapon() will fail if the signature >> * is not actually on disk. (This is a kernel bug.) >> * The fsync() in close_fd() will take care of writing. >> */ >> >> I'm assuming your mkswap is recent enough. Any idea why fsync() >> in mkswap is no longer enough? > > > > Good catch, Jan. It sounds like a mkswap/fsync issue. I looked into this issue today. It seems like a new kernel bug which has been fixed by commit 117a148ffe0 (iomap: fsync swap files before iterating mappings) in linus tree. ------------------ Simple steps to reproduce that: # dd if=/dev/zero of=swapfile01 bs=2048 count=1024 1024+0 records in 1024+0 records out 2097152 bytes (2.1 MB) copied, 0.00408662 s, 513 MB/s # mkswap swapfile01 Setting up swapspace version 1, size = 2044 KiB no label, UUID=17830401-9cf2-4143-88a8-5a89c0b2d01d # chmod 600 swapfile01 # swapon swapfile01 swapon: swapfile01: swapon failed: Invalid argument Checking dmesg and get the root cause is swapfile01 have uncommitted metadata in caches: [ 128.698981] swapon: file is not committed Then, I tracking the __x64_sys_swapon function calltrace on my bad kernel: xfs_iomap_swapfile_activate [xfs]() { iomap_swapfile_activate() { filemap_write_and_wait() { <===== [1] mapping_needs_writeback(); __filemap_fdatawrite_range() { _raw_spin_lock(); wbc_attach_and_unlock_inode(); do_writepages() { xfs_vm_writepages [xfs]() { _raw_spin_lock(); write_cache_pages() { tag_pages_for_writeback() { _raw_spin_lock_irq(); } pagevec_lookup_range_tag() { find_get_pages_range_tag(); } } } } wbc_detach_inode(); } Noticed [1] was being replaced by "ret = vfs_fsync(swap_file, 1);" in the kernel commit 117a148ff. And this change works fine to me. But I still have a query as Jan mentioned that, the command mkswap has already done the write-back action via fsync() in close_fd(). Why we have to do this fsync() again in kernel side and then have effect to swapfile? $ tail util-linux/disk-utils/mkswap.c #endif /* * A subsequent swapon() will fail if the signature * is not actually on disk. (This is a kernel bug.) * The fsync() in close_fd() will take care of writing. */ if (close_fd(ctl.fd) != 0) err(EXIT_FAILURE, _("write failed")); return EXIT_SUCCESS; } $ tail -12 util-linux/disk-utils/include/closestream.h static inline int close_fd(int fd) { const int fsync_fail = (fsync(fd) != 0); const int close_fail = (close(fd) != 0); if (fsync_fail || close_fail) return EOF; return 0; } #endif /* UTIL_LINUX_CLOSESTREAM_H */ -------------- Anyway, this is not a LTP issue, so plz ignore my previous patch. Thanks, Li Wang
diff --git a/testcases/kernel/syscalls/swapoff/swapoff01.c b/testcases/kernel/syscalls/swapoff/swapoff01.c index a63e661..b587b0d 100644 --- a/testcases/kernel/syscalls/swapoff/swapoff01.c +++ b/testcases/kernel/syscalls/swapoff/swapoff01.c @@ -105,6 +105,8 @@ static void setup(void) if (system("mkswap swapfile01 > tmpfile 2>&1") != 0) tst_brkm(TBROK, cleanup, "Failed to make swapfile"); + + sync(); } static void cleanup(void) diff --git a/testcases/kernel/syscalls/swapoff/swapoff02.c b/testcases/kernel/syscalls/swapoff/swapoff02.c index b5c6312..d8c5889 100644 --- a/testcases/kernel/syscalls/swapoff/swapoff02.c +++ b/testcases/kernel/syscalls/swapoff/swapoff02.c @@ -154,6 +154,8 @@ static void setup(void) if (tst_fill_file("./swapfile01", 0x00, 1024, 1)) tst_brkm(TBROK, cleanup, "Failed to create swapfile"); + + sync(); } static void cleanup(void) diff --git a/testcases/kernel/syscalls/swapon/libswapon.c b/testcases/kernel/syscalls/swapon/libswapon.c index cf6a988..49fa5ed 100644 --- a/testcases/kernel/syscalls/swapon/libswapon.c +++ b/testcases/kernel/syscalls/swapon/libswapon.c @@ -46,4 +46,6 @@ void make_swapfile(void (cleanup)(void), const char *swapfile) argv[2] = NULL; tst_run_cmd(cleanup, argv, "/dev/null", "/dev/null", 0); + + sync(); }
Testcase include swapon(8) fails on mainline kernel-v4.17+ as: swapon01 1 TFAIL : swapon01.c:47: Failed to turn on swapfile: TEST_ERRNO=EINVAL(22): Invalid argument swapon02 4 TFAIL : swapon02.c:91: swapon(2) failed to produce expected error: 16, errno: EBUSY and got 22. swapon03 1 TFAIL : swapon03.c:243: Failed to setup swaps swapoff01 1 TBROK : swapoff01.c:64: Failed to turn on the swap file, skipping test iteration And dmesg shows that: [ 128.698981] swapon: file is not committed The error located in IOMAP_F_DIRTY(linux/fs/iomap.c) checking, which means this swapfile have uncommitted metadata in caches. After adding sync() to the end of all mkswap opertaion, these errors were gone. linux/fs/iomap.c: static loff_t iomap_swapfile_activate_actor( struct inode *inode, loff_t pos, loff_t count, void *data, struct iomap *iomap) { struct iomap_swapfile_info *isi = data; int error; ... /* No uncommitted metadata or shared blocks. */ if (iomap->flags & IOMAP_F_DIRTY) { pr_err("swapon: file is not committed\n"); return -EINVAL; } ... } } Signed-off-by: Li Wang <liwang@redhat.com> --- Notes: The new changes about swapfile activation function were merged in kernel-4.17 recently, so we didn't hit this failures before. testcases/kernel/syscalls/swapoff/swapoff01.c | 2 ++ testcases/kernel/syscalls/swapoff/swapoff02.c | 2 ++ testcases/kernel/syscalls/swapon/libswapon.c | 2 ++ 3 files changed, 6 insertions(+)