[PATCH] brd: fix brd_rw_page() vs copy_to_brd_setup errors

[PATCH] brd: fix brd_rw_page() vs copy_to_brd_setup errors

[Dan Williams]

As is done in zram_rw_page, pmem_rw_page, and btt_rw_page, don't
call page_endio in the error case since do_mpage_readpage and
__mpage_writepage will resubmit on error. Calling page_endio in the
error case leads to double completion.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Matthew Wilcox <mawilcox@microsoft.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Noticed this while looking at unrelated brd code...

 drivers/block/brd.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 104b71c0490d..055255ea131d 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -327,7 +327,13 @@ static int brd_rw_page(struct block_device *bdev, sector_t sector,
 {
 	struct brd_device *brd = bdev->bd_disk->private_data;
 	int err = brd_do_bvec(brd, page, PAGE_SIZE, 0, is_write, sector);
-	page_endio(page, is_write, err);
+
+	/*
+	 * In the error case we expect the upper layer to retry, so we
+	 * can't trigger page_endio yet.
+	 */
+	if (err == 0)
+		page_endio(page, is_write, 0);
 	return err;
 }
 

Re: [PATCH] brd: fix brd_rw_page() vs copy_to_brd_setup errors

[Christoph Hellwig]

On Tue, Jul 25, 2017 at 06:02:29PM -0700, Dan Williams wrote:
> As is done in zram_rw_page, pmem_rw_page, and btt_rw_page, don't
> call page_endio in the error case since do_mpage_readpage and
> __mpage_writepage will resubmit on error. Calling page_endio in the
> error case leads to double completion.
> 
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Matthew Wilcox <mawilcox@microsoft.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Noticed this while looking at unrelated brd code...

And the real question would be:  where would we see any real life impact
of just removing brd_rw_page?

Re: [PATCH] brd: fix brd_rw_page() vs copy_to_brd_setup errors

[Ross Zwisler]

On Wed, Jul 26, 2017 at 01:12:28PM -0700, Christoph Hellwig wrote:
> On Tue, Jul 25, 2017 at 06:02:29PM -0700, Dan Williams wrote:
> > As is done in zram_rw_page, pmem_rw_page, and btt_rw_page, don't
> > call page_endio in the error case since do_mpage_readpage and
> > __mpage_writepage will resubmit on error. Calling page_endio in the
> > error case leads to double completion.
> > 
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Cc: Matthew Wilcox <mawilcox@microsoft.com>
> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> > Noticed this while looking at unrelated brd code...
> 
> And the real question would be:  where would we see any real life impact
> of just removing brd_rw_page?

I've got patches ready that remove rw_page from brd, btt and pmem.  I'll send
out once I'm done regression testing.

Re: [PATCH] brd: fix brd_rw_page() vs copy_to_brd_setup errors

[Dan Williams]

On Wed, Jul 26, 2017 at 2:32 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Wed, Jul 26, 2017 at 01:12:28PM -0700, Christoph Hellwig wrote:
>> On Tue, Jul 25, 2017 at 06:02:29PM -0700, Dan Williams wrote:
>> > As is done in zram_rw_page, pmem_rw_page, and btt_rw_page, don't
>> > call page_endio in the error case since do_mpage_readpage and
>> > __mpage_writepage will resubmit on error. Calling page_endio in the
>> > error case leads to double completion.
>> >
>> > Cc: Jens Axboe <axboe@kernel.dk>
>> > Cc: Matthew Wilcox <mawilcox@microsoft.com>
>> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> > ---
>> > Noticed this while looking at unrelated brd code...
>>
>> And the real question would be:  where would we see any real life impact
>> of just removing brd_rw_page?
>
> I've got patches ready that remove rw_page from brd, btt and pmem.  I'll send
> out once I'm done regression testing.

That would leave zram_rw_page(), is there a compelling reason to keep
that and the related infrastructure?