linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] loop: Fix lost writes caused by missing flag
@ 2018-02-12 23:05 Ross Zwisler
  2018-02-13 14:54 ` Christoph Hellwig
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Ross Zwisler @ 2018-02-12 23:05 UTC (permalink / raw)
  To: linux-kernel
  Cc: Ross Zwisler, linux-nvdimm, Christoph Hellwig, Al Viro, stable

The following commit:

commit aa4d86163e4e ("block: loop: switch to VFS ITER_BVEC")

replaced __do_lo_send_write(), which used ITER_KVEC iterators, with
lo_write_bvec() which uses ITER_BVEC iterators.  In this change, though,
the WRITE flag was lost:

-       iov_iter_kvec(&from, ITER_KVEC | WRITE, &kvec, 1, len);
+       iov_iter_bvec(&i, ITER_BVEC, bvec, 1, bvec->bv_len);

This flag is necessary for the DAX case because we make decisions based on
whether or not the iterator is a READ or a WRITE in dax_iomap_actor() and
in dax_iomap_rw().

We end up going through this path in configurations where we combine a PMEM
device with 4k sectors, a loopback device and DAX.  The consequence of this
missed flag is that what we intend as a write actually turns into a read in
the DAX code, so no data is ever written.

The very simplest test case is to create a loopback device and try and
write a small string to it, then hexdump a few bytes of the device to see
if the write took.  Without this patch you read back all zeros, with this
you read back the string you wrote.

For XFS this causes us to fail or panic during the following xfstests:

	xfs/074 xfs/078 xfs/216 xfs/217 xfs/250

For ext4 we have a similar issue where writes never happen, but we don't
currently have any xfstests that use loopback and show this issue.

Fix this by restoring the WRITE flag argument to iov_iter_bvec().  This
causes the xfstests to all pass.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Fixes: commit aa4d86163e4e ("block: loop: switch to VFS ITER_BVEC")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: stable@vger.kernel.org
---
 drivers/block/loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d5fe720cf149..89d2ee00cced 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -266,7 +266,7 @@ static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos)
 	struct iov_iter i;
 	ssize_t bw;
 
-	iov_iter_bvec(&i, ITER_BVEC, bvec, 1, bvec->bv_len);
+	iov_iter_bvec(&i, ITER_BVEC | WRITE, bvec, 1, bvec->bv_len);
 
 	file_start_write(file);
 	bw = vfs_iter_write(file, &i, ppos, 0);
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] loop: Fix lost writes caused by missing flag
  2018-02-12 23:05 [PATCH] loop: Fix lost writes caused by missing flag Ross Zwisler
@ 2018-02-13 14:54 ` Christoph Hellwig
  2018-02-13 19:22   ` Ross Zwisler
  2018-02-22  3:21 ` Ming Lei
  2018-03-09  0:20 ` Ross Zwisler
  2 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2018-02-13 14:54 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: linux-kernel, linux-nvdimm, Christoph Hellwig, Al Viro, stable

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Can you wire up your test cases for blktests?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] loop: Fix lost writes caused by missing flag
  2018-02-13 14:54 ` Christoph Hellwig
@ 2018-02-13 19:22   ` Ross Zwisler
  2018-02-13 20:52     ` Dan Williams
  0 siblings, 1 reply; 11+ messages in thread
From: Ross Zwisler @ 2018-02-13 19:22 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Ross Zwisler, linux-kernel, linux-nvdimm, Al Viro, stable

On Tue, Feb 13, 2018 at 03:54:04PM +0100, Christoph Hellwig wrote:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Can you wire up your test cases for blktests?

Is blktests really the right place for this test?  This failure is highly
dependent on the configuration of the filesystem that is holding the file that
we are using for the loopback device.  It doesn't seem like blktests has
support for mount options (dax), etc?

Because of the interaction with the underlying filesystem this seems like a
better fit with xfstests to me, but I don't know if we need to add tests there
because we already have pretty good coverage of loopback device failures.
That's how we found this - this bug causes all these tests to fail:
xfs/074 xfs/078 xfs/216 xfs/217 xfs/250

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] loop: Fix lost writes caused by missing flag
  2018-02-13 19:22   ` Ross Zwisler
@ 2018-02-13 20:52     ` Dan Williams
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Williams @ 2018-02-13 20:52 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: Christoph Hellwig, stable, Linux Kernel Mailing List, Al Viro,
	linux-nvdimm

On Tue, Feb 13, 2018 at 11:22 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Tue, Feb 13, 2018 at 03:54:04PM +0100, Christoph Hellwig wrote:
>> Looks good:
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>
>> Can you wire up your test cases for blktests?
>
> Is blktests really the right place for this test?  This failure is highly
> dependent on the configuration of the filesystem that is holding the file that
> we are using for the loopback device.  It doesn't seem like blktests has
> support for mount options (dax), etc?
>
> Because of the interaction with the underlying filesystem this seems like a
> better fit with xfstests to me, but I don't know if we need to add tests there
> because we already have pretty good coverage of loopback device failures.
> That's how we found this - this bug causes all these tests to fail:
> xfs/074 xfs/078 xfs/216 xfs/217 xfs/250

The problem is that those tests don't configure the device in 4K
sector mode, so we're still missing a regression test. That seems to
be where blktests can come into play.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] loop: Fix lost writes caused by missing flag
  2018-02-12 23:05 [PATCH] loop: Fix lost writes caused by missing flag Ross Zwisler
  2018-02-13 14:54 ` Christoph Hellwig
@ 2018-02-22  3:21 ` Ming Lei
  2018-03-09  0:20 ` Ross Zwisler
  2 siblings, 0 replies; 11+ messages in thread
From: Ming Lei @ 2018-02-22  3:21 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: Linux Kernel Mailing List, linux-nvdimm, Christoph Hellwig,
	Al Viro, stable

On Tue, Feb 13, 2018 at 7:05 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> The following commit:
>
> commit aa4d86163e4e ("block: loop: switch to VFS ITER_BVEC")
>
> replaced __do_lo_send_write(), which used ITER_KVEC iterators, with
> lo_write_bvec() which uses ITER_BVEC iterators.  In this change, though,
> the WRITE flag was lost:
>
> -       iov_iter_kvec(&from, ITER_KVEC | WRITE, &kvec, 1, len);
> +       iov_iter_bvec(&i, ITER_BVEC, bvec, 1, bvec->bv_len);
>
> This flag is necessary for the DAX case because we make decisions based on
> whether or not the iterator is a READ or a WRITE in dax_iomap_actor() and
> in dax_iomap_rw().
>
> We end up going through this path in configurations where we combine a PMEM
> device with 4k sectors, a loopback device and DAX.  The consequence of this
> missed flag is that what we intend as a write actually turns into a read in
> the DAX code, so no data is ever written.
>
> The very simplest test case is to create a loopback device and try and
> write a small string to it, then hexdump a few bytes of the device to see
> if the write took.  Without this patch you read back all zeros, with this
> you read back the string you wrote.
>
> For XFS this causes us to fail or panic during the following xfstests:
>
>         xfs/074 xfs/078 xfs/216 xfs/217 xfs/250
>
> For ext4 we have a similar issue where writes never happen, but we don't
> currently have any xfstests that use loopback and show this issue.
>
> Fix this by restoring the WRITE flag argument to iov_iter_bvec().  This
> causes the xfstests to all pass.
>
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Fixes: commit aa4d86163e4e ("block: loop: switch to VFS ITER_BVEC")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: stable@vger.kernel.org
> ---
>  drivers/block/loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index d5fe720cf149..89d2ee00cced 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -266,7 +266,7 @@ static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos)
>         struct iov_iter i;
>         ssize_t bw;
>
> -       iov_iter_bvec(&i, ITER_BVEC, bvec, 1, bvec->bv_len);
> +       iov_iter_bvec(&i, ITER_BVEC | WRITE, bvec, 1, bvec->bv_len);
>
>         file_start_write(file);
>         bw = vfs_iter_write(file, &i, ppos, 0);
> --
> 2.14.3
>

Reviewed-by: Ming Lei <ming.lei@redhat.com>


-- 
Ming Lei

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] loop: Fix lost writes caused by missing flag
  2018-02-12 23:05 [PATCH] loop: Fix lost writes caused by missing flag Ross Zwisler
  2018-02-13 14:54 ` Christoph Hellwig
  2018-02-22  3:21 ` Ming Lei
@ 2018-03-09  0:20 ` Ross Zwisler
  2018-03-09 15:38   ` Jens Axboe
  2 siblings, 1 reply; 11+ messages in thread
From: Ross Zwisler @ 2018-03-09  0:20 UTC (permalink / raw)
  To: Ross Zwisler, Jens Axboe
  Cc: linux-kernel, linux-nvdimm, Christoph Hellwig, Al Viro, stable

This has gotten Reviewed-by tags from Christoph and Ming Lei.

Al, are you the right person to merge this?  Or is the correct person Jens,
whom I accidentally didn't include when I sent this out?

Just wanted to make sure this got merged, and to see whether it was targeting
v4.16 or v4.17.

Thanks,
- Ross

On Mon, Feb 12, 2018 at 04:05:58PM -0700, Ross Zwisler wrote:
> The following commit:
> 
> commit aa4d86163e4e ("block: loop: switch to VFS ITER_BVEC")
> 
> replaced __do_lo_send_write(), which used ITER_KVEC iterators, with
> lo_write_bvec() which uses ITER_BVEC iterators.  In this change, though,
> the WRITE flag was lost:
> 
> -       iov_iter_kvec(&from, ITER_KVEC | WRITE, &kvec, 1, len);
> +       iov_iter_bvec(&i, ITER_BVEC, bvec, 1, bvec->bv_len);
> 
> This flag is necessary for the DAX case because we make decisions based on
> whether or not the iterator is a READ or a WRITE in dax_iomap_actor() and
> in dax_iomap_rw().
> 
> We end up going through this path in configurations where we combine a PMEM
> device with 4k sectors, a loopback device and DAX.  The consequence of this
> missed flag is that what we intend as a write actually turns into a read in
> the DAX code, so no data is ever written.
> 
> The very simplest test case is to create a loopback device and try and
> write a small string to it, then hexdump a few bytes of the device to see
> if the write took.  Without this patch you read back all zeros, with this
> you read back the string you wrote.
> 
> For XFS this causes us to fail or panic during the following xfstests:
> 
> 	xfs/074 xfs/078 xfs/216 xfs/217 xfs/250
> 
> For ext4 we have a similar issue where writes never happen, but we don't
> currently have any xfstests that use loopback and show this issue.
> 
> Fix this by restoring the WRITE flag argument to iov_iter_bvec().  This
> causes the xfstests to all pass.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> Fixes: commit aa4d86163e4e ("block: loop: switch to VFS ITER_BVEC")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: stable@vger.kernel.org
> ---
>  drivers/block/loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index d5fe720cf149..89d2ee00cced 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -266,7 +266,7 @@ static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos)
>  	struct iov_iter i;
>  	ssize_t bw;
>  
> -	iov_iter_bvec(&i, ITER_BVEC, bvec, 1, bvec->bv_len);
> +	iov_iter_bvec(&i, ITER_BVEC | WRITE, bvec, 1, bvec->bv_len);
>  
>  	file_start_write(file);
>  	bw = vfs_iter_write(file, &i, ppos, 0);
> -- 
> 2.14.3
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] loop: Fix lost writes caused by missing flag
  2018-03-09  0:20 ` Ross Zwisler
@ 2018-03-09 15:38   ` Jens Axboe
  2018-03-09 15:38     ` Jens Axboe
  0 siblings, 1 reply; 11+ messages in thread
From: Jens Axboe @ 2018-03-09 15:38 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: linux-kernel, linux-nvdimm, Christoph Hellwig, Al Viro, stable

On 3/8/18 5:20 PM, Ross Zwisler wrote:
> This has gotten Reviewed-by tags from Christoph and Ming Lei.
> 
> Al, are you the right person to merge this?  Or is the correct person Jens,
> whom I accidentally didn't include when I sent this out?
> 
> Just wanted to make sure this got merged, and to see whether it was targeting
> v4.16 or v4.17.

I'm the right guy, but I don't see patches if I'm not cc'ed on them...
I have queued this up for 4.16, thanks.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] loop: Fix lost writes caused by missing flag
  2018-03-09 15:38   ` Jens Axboe
@ 2018-03-09 15:38     ` Jens Axboe
  2018-03-09 16:35       ` Ross Zwisler
  2018-03-09 16:38       ` [PATCH] MAINTAINERS: add coverage for drivers/block Ross Zwisler
  0 siblings, 2 replies; 11+ messages in thread
From: Jens Axboe @ 2018-03-09 15:38 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: linux-kernel, linux-nvdimm, Christoph Hellwig, Al Viro, stable

On 3/9/18 8:38 AM, Jens Axboe wrote:
> On 3/8/18 5:20 PM, Ross Zwisler wrote:
>> This has gotten Reviewed-by tags from Christoph and Ming Lei.
>>
>> Al, are you the right person to merge this?  Or is the correct person Jens,
>> whom I accidentally didn't include when I sent this out?
>>
>> Just wanted to make sure this got merged, and to see whether it was targeting
>> v4.16 or v4.17.
> 
> I'm the right guy, but I don't see patches if I'm not cc'ed on them...
> I have queued this up for 4.16, thanks.

I do see patches sent to linux-block as well, but you didn't CC that one
either.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] loop: Fix lost writes caused by missing flag
  2018-03-09 15:38     ` Jens Axboe
@ 2018-03-09 16:35       ` Ross Zwisler
  2018-03-09 17:19         ` Jens Axboe
  2018-03-09 16:38       ` [PATCH] MAINTAINERS: add coverage for drivers/block Ross Zwisler
  1 sibling, 1 reply; 11+ messages in thread
From: Ross Zwisler @ 2018-03-09 16:35 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Ross Zwisler, linux-kernel, linux-nvdimm, Christoph Hellwig,
	Al Viro, stable

On Fri, Mar 09, 2018 at 08:38:57AM -0700, Jens Axboe wrote:
> On 3/9/18 8:38 AM, Jens Axboe wrote:
> > On 3/8/18 5:20 PM, Ross Zwisler wrote:
> >> This has gotten Reviewed-by tags from Christoph and Ming Lei.
> >>
> >> Al, are you the right person to merge this?  Or is the correct person Jens,
> >> whom I accidentally didn't include when I sent this out?
> >>
> >> Just wanted to make sure this got merged, and to see whether it was targeting
> >> v4.16 or v4.17.
> > 
> > I'm the right guy, but I don't see patches if I'm not cc'ed on them...
> > I have queued this up for 4.16, thanks.
> 
> I do see patches sent to linux-block as well, but you didn't CC that one
> either.

I figure out who to CC on my patches by using scripts/get_maintainer.pl, and
it didn't know about you or linux-block because drivers/block wasn't covered
in MAINTAINERS.  I'll send a patch to fix this.

As it was I just CC'd the folks involved in the original patch, and that one
went up through Al.

Thanks for picking this up.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] MAINTAINERS: add coverage for drivers/block
  2018-03-09 15:38     ` Jens Axboe
  2018-03-09 16:35       ` Ross Zwisler
@ 2018-03-09 16:38       ` Ross Zwisler
  1 sibling, 0 replies; 11+ messages in thread
From: Ross Zwisler @ 2018-03-09 16:38 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel, linux-nvdimm, Christoph Hellwig, Al Viro
  Cc: Ross Zwisler

To help folks like me that use scripts/get_maintainer.pl.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4623caf8d72d..7ff83f4c9aeb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2685,6 +2685,7 @@ L:	linux-block@vger.kernel.org
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git
 S:	Maintained
 F:	block/
+F:	drivers/block/
 F:	kernel/trace/blktrace.c
 F:	lib/sbitmap.c
 
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] loop: Fix lost writes caused by missing flag
  2018-03-09 16:35       ` Ross Zwisler
@ 2018-03-09 17:19         ` Jens Axboe
  0 siblings, 0 replies; 11+ messages in thread
From: Jens Axboe @ 2018-03-09 17:19 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: linux-kernel, linux-nvdimm, Christoph Hellwig, Al Viro, stable

On 3/9/18 9:35 AM, Ross Zwisler wrote:
> On Fri, Mar 09, 2018 at 08:38:57AM -0700, Jens Axboe wrote:
>> On 3/9/18 8:38 AM, Jens Axboe wrote:
>>> On 3/8/18 5:20 PM, Ross Zwisler wrote:
>>>> This has gotten Reviewed-by tags from Christoph and Ming Lei.
>>>>
>>>> Al, are you the right person to merge this?  Or is the correct person Jens,
>>>> whom I accidentally didn't include when I sent this out?
>>>>
>>>> Just wanted to make sure this got merged, and to see whether it was targeting
>>>> v4.16 or v4.17.
>>>
>>> I'm the right guy, but I don't see patches if I'm not cc'ed on them...
>>> I have queued this up for 4.16, thanks.
>>
>> I do see patches sent to linux-block aswell, but you didn't CC that one
>> either.
> 
> I figure out who to CC on my patches by using scripts/get_maintainer.pl, and
> it didn't know about you or linux-block because drivers/block wasn't covered
> in MAINTAINERS.  I'll send a patch to fix this.

I'm the first person for a check on drivers/block/ or
drivers/block/loop.c, though...

> As it was I just CC'd the folks involved in the original patch, and that one
> went up through Al.
> 
> Thanks for picking this up.

No problem, glad it worked out.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-03-09 17:19 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-12 23:05 [PATCH] loop: Fix lost writes caused by missing flag Ross Zwisler
2018-02-13 14:54 ` Christoph Hellwig
2018-02-13 19:22   ` Ross Zwisler
2018-02-13 20:52     ` Dan Williams
2018-02-22  3:21 ` Ming Lei
2018-03-09  0:20 ` Ross Zwisler
2018-03-09 15:38   ` Jens Axboe
2018-03-09 15:38     ` Jens Axboe
2018-03-09 16:35       ` Ross Zwisler
2018-03-09 17:19         ` Jens Axboe
2018-03-09 16:38       ` [PATCH] MAINTAINERS: add coverage for drivers/block Ross Zwisler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).