swapon: commit swapfile caches to disk

Message ID 1528806239-14297-1-git-send-email-liwang@redhat.com
State Rejected
Headers show
Series
  • swapon: commit swapfile caches to disk
Related show

Commit Message

Li Wang June 12, 2018, 12:23 p.m.
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(+)

Comments

Cyril Hrubis June 12, 2018, 12:48 p.m. | #1
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.
Jan Stancek June 12, 2018, 12:56 p.m. | #2
----- 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
>
Li Wang June 12, 2018, 1:05 p.m. | #3
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
>
Li Wang June 12, 2018, 1:12 p.m. | #4
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
> >
>
Li Wang June 12, 2018, 1:17 p.m. | #5
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
>
Li Wang June 14, 2018, 6:37 a.m. | #6
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

Patch

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();
 }