| Message ID | 150103091559.7874.16803761117298861951.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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.
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?
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; }
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(-)