From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1947887AbbGaVih (ORCPT ); Fri, 31 Jul 2015 17:38:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57731 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754355AbbGaVif (ORCPT ); Fri, 31 Jul 2015 17:38:35 -0400 Date: Fri, 31 Jul 2015 17:38:32 -0400 From: Mike Snitzer To: Ming Lin Cc: lkml , Christoph Hellwig , Jens Axboe , Kent Overstreet , Dongsu Park , Christoph Hellwig , Al Viro , Ming Lei , Neil Brown , Alasdair Kergon , dm-devel@redhat.com, Lars Ellenberg , drbd-user@lists.linbit.com, Jiri Kosina , Geoff Levand , Jim Paris , Joshua Morris , Philip Kelleher , Minchan Kim , Nitin Gupta , Oleg Drokin , Andreas Dilger , Ming Lin Subject: Re: [PATCH v5 01/11] block: make generic_make_request handle arbitrarily sized bios Message-ID: <20150731213831.GA16464@redhat.com> References: <1436168690-32102-1-git-send-email-mlin@kernel.org> <20150731192337.GA8907@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 31 2015 at 5:19pm -0400, Ming Lin wrote: > On Fri, Jul 31, 2015 at 12:23 PM, Mike Snitzer wrote: > > On Mon, Jul 06 2015 at 3:44P -0400, > > Ming Lin wrote: > > > >> From: Kent Overstreet > >> > >> The way the block layer is currently written, it goes to great lengths > >> to avoid having to split bios; upper layer code (such as bio_add_page()) > >> checks what the underlying device can handle and tries to always create > >> bios that don't need to be split. > >> > >> But this approach becomes unwieldy and eventually breaks down with > >> stacked devices and devices with dynamic limits, and it adds a lot of > >> complexity. If the block layer could split bios as needed, we could > >> eliminate a lot of complexity elsewhere - particularly in stacked > >> drivers. Code that creates bios can then create whatever size bios are > >> convenient, and more importantly stacked drivers don't have to deal with > >> both their own bio size limitations and the limitations of the > >> (potentially multiple) devices underneath them. In the future this will > >> let us delete merge_bvec_fn and a bunch of other code. > >> > >> We do this by adding calls to blk_queue_split() to the various > >> make_request functions that need it - a few can already handle arbitrary > >> size bios. Note that we add the call _after_ any call to > >> blk_queue_bounce(); this means that blk_queue_split() and > >> blk_recalc_rq_segments() don't need to be concerned with bouncing > >> affecting segment merging. > >> > >> Some make_request_fn() callbacks were simple enough to audit and verify > >> they don't need blk_queue_split() calls. The skipped ones are: > >> > >> * nfhd_make_request (arch/m68k/emu/nfblock.c) > >> * axon_ram_make_request (arch/powerpc/sysdev/axonram.c) > >> * simdisk_make_request (arch/xtensa/platforms/iss/simdisk.c) > >> * brd_make_request (ramdisk - drivers/block/brd.c) > >> * mtip_submit_request (drivers/block/mtip32xx/mtip32xx.c) > >> * loop_make_request > >> * null_queue_bio > >> * bcache's make_request fns > >> > >> Some others are almost certainly safe to remove now, but will be left > >> for future patches. > >> > >> Cc: Jens Axboe > >> Cc: Christoph Hellwig > >> Cc: Al Viro > >> Cc: Ming Lei > >> Cc: Neil Brown > >> Cc: Alasdair Kergon > >> Cc: Mike Snitzer > >> Cc: dm-devel@redhat.com > >> Cc: Lars Ellenberg > >> Cc: drbd-user@lists.linbit.com > >> Cc: Jiri Kosina > >> Cc: Geoff Levand > >> Cc: Jim Paris > >> Cc: Joshua Morris > >> Cc: Philip Kelleher > >> Cc: Minchan Kim > >> Cc: Nitin Gupta > >> Cc: Oleg Drokin > >> Cc: Andreas Dilger > >> Acked-by: NeilBrown (for the 'md/md.c' bits) > >> Signed-off-by: Kent Overstreet > >> [dpark: skip more mq-based drivers, resolve merge conflicts, etc.] > >> Signed-off-by: Dongsu Park > >> Signed-off-by: Ming Lin > > ... > >> diff --git a/block/blk-merge.c b/block/blk-merge.c > >> index 30a0d9f..3707f30 100644 > >> --- a/block/blk-merge.c > >> +++ b/block/blk-merge.c > >> @@ -9,12 +9,158 @@ > >> > >> #include "blk.h" > >> > >> +static struct bio *blk_bio_discard_split(struct request_queue *q, > >> + struct bio *bio, > >> + struct bio_set *bs) > >> +{ > >> + unsigned int max_discard_sectors, granularity; > >> + int alignment; > >> + sector_t tmp; > >> + unsigned split_sectors; > >> + > >> + /* Zero-sector (unknown) and one-sector granularities are the same. */ > >> + granularity = max(q->limits.discard_granularity >> 9, 1U); > >> + > >> + max_discard_sectors = min(q->limits.max_discard_sectors, UINT_MAX >> 9); > >> + max_discard_sectors -= max_discard_sectors % granularity; > >> + > >> + if (unlikely(!max_discard_sectors)) { > >> + /* XXX: warn */ > >> + return NULL; > >> + } > >> + > >> + if (bio_sectors(bio) <= max_discard_sectors) > >> + return NULL; > >> + > >> + split_sectors = max_discard_sectors; > >> + > >> + /* > >> + * If the next starting sector would be misaligned, stop the discard at > >> + * the previous aligned sector. > >> + */ > >> + alignment = (q->limits.discard_alignment >> 9) % granularity; > >> + > >> + tmp = bio->bi_iter.bi_sector + split_sectors - alignment; > >> + tmp = sector_div(tmp, granularity); > >> + > >> + if (split_sectors > tmp) > >> + split_sectors -= tmp; > >> + > >> + return bio_split(bio, split_sectors, GFP_NOIO, bs); > >> +} > > > > This code to stop the discard at the previous aligned sector could be > > the reason why I have 2 device-mapper-test-suite tests in the > > 'thin-provisioning' testsuite failing due to this patchset: > > I'm setting up the testsuite to debug. OK, once setup, to run the 2 tests in question directly you'd do something like: dmtest run --suite thin-provisioning -n discard_a_fragmented_device dmtest run --suite thin-provisioning -n discard_fully_provisioned_device_benchmark Again, these tests pass without this patchset. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Snitzer Subject: Re: [PATCH v5 01/11] block: make generic_make_request handle arbitrarily sized bios Date: Fri, 31 Jul 2015 17:38:32 -0400 Message-ID: <20150731213831.GA16464@redhat.com> References: <1436168690-32102-1-git-send-email-mlin@kernel.org> <20150731192337.GA8907@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: drbd-dev-bounces-cunTk1MwBs8qoQakbn7OcQ@public.gmane.org Errors-To: drbd-dev-bounces-cunTk1MwBs8qoQakbn7OcQ@public.gmane.org To: Ming Lin Cc: Neil Brown , Ming Lei , dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, Christoph Hellwig , Alasdair Kergon , Lars Ellenberg , Philip Kelleher , Joshua Morris , Christoph Hellwig , Kent Overstreet , Nitin Gupta , Ming Lin , Oleg Drokin , Al Viro , Jens Axboe , Andreas Dilger , Geoff Levand , Jiri Kosina , lkml , Jim Paris , Minchan Kim , Dongsu Park , drbd-user-cunTk1MwBs8qoQakbn7OcQ@public.gmane.org List-Id: dm-devel.ids On Fri, Jul 31 2015 at 5:19pm -0400, Ming Lin wrote: > On Fri, Jul 31, 2015 at 12:23 PM, Mike Snitzer wrote: > > On Mon, Jul 06 2015 at 3:44P -0400, > > Ming Lin wrote: > > > >> From: Kent Overstreet > >> > >> The way the block layer is currently written, it goes to great lengths > >> to avoid having to split bios; upper layer code (such as bio_add_page()) > >> checks what the underlying device can handle and tries to always create > >> bios that don't need to be split. > >> > >> But this approach becomes unwieldy and eventually breaks down with > >> stacked devices and devices with dynamic limits, and it adds a lot of > >> complexity. If the block layer could split bios as needed, we could > >> eliminate a lot of complexity elsewhere - particularly in stacked > >> drivers. Code that creates bios can then create whatever size bios are > >> convenient, and more importantly stacked drivers don't have to deal with > >> both their own bio size limitations and the limitations of the > >> (potentially multiple) devices underneath them. In the future this will > >> let us delete merge_bvec_fn and a bunch of other code. > >> > >> We do this by adding calls to blk_queue_split() to the various > >> make_request functions that need it - a few can already handle arbitrary > >> size bios. Note that we add the call _after_ any call to > >> blk_queue_bounce(); this means that blk_queue_split() and > >> blk_recalc_rq_segments() don't need to be concerned with bouncing > >> affecting segment merging. > >> > >> Some make_request_fn() callbacks were simple enough to audit and verify > >> they don't need blk_queue_split() calls. The skipped ones are: > >> > >> * nfhd_make_request (arch/m68k/emu/nfblock.c) > >> * axon_ram_make_request (arch/powerpc/sysdev/axonram.c) > >> * simdisk_make_request (arch/xtensa/platforms/iss/simdisk.c) > >> * brd_make_request (ramdisk - drivers/block/brd.c) > >> * mtip_submit_request (drivers/block/mtip32xx/mtip32xx.c) > >> * loop_make_request > >> * null_queue_bio > >> * bcache's make_request fns > >> > >> Some others are almost certainly safe to remove now, but will be left > >> for future patches. > >> > >> Cc: Jens Axboe > >> Cc: Christoph Hellwig > >> Cc: Al Viro > >> Cc: Ming Lei > >> Cc: Neil Brown > >> Cc: Alasdair Kergon > >> Cc: Mike Snitzer > >> Cc: dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org > >> Cc: Lars Ellenberg > >> Cc: drbd-user-cunTk1MwBs8qoQakbn7OcQ@public.gmane.org > >> Cc: Jiri Kosina > >> Cc: Geoff Levand > >> Cc: Jim Paris > >> Cc: Joshua Morris > >> Cc: Philip Kelleher > >> Cc: Minchan Kim > >> Cc: Nitin Gupta > >> Cc: Oleg Drokin > >> Cc: Andreas Dilger > >> Acked-by: NeilBrown (for the 'md/md.c' bits) > >> Signed-off-by: Kent Overstreet > >> [dpark: skip more mq-based drivers, resolve merge conflicts, etc.] > >> Signed-off-by: Dongsu Park > >> Signed-off-by: Ming Lin > > ... > >> diff --git a/block/blk-merge.c b/block/blk-merge.c > >> index 30a0d9f..3707f30 100644 > >> --- a/block/blk-merge.c > >> +++ b/block/blk-merge.c > >> @@ -9,12 +9,158 @@ > >> > >> #include "blk.h" > >> > >> +static struct bio *blk_bio_discard_split(struct request_queue *q, > >> + struct bio *bio, > >> + struct bio_set *bs) > >> +{ > >> + unsigned int max_discard_sectors, granularity; > >> + int alignment; > >> + sector_t tmp; > >> + unsigned split_sectors; > >> + > >> + /* Zero-sector (unknown) and one-sector granularities are the same. */ > >> + granularity = max(q->limits.discard_granularity >> 9, 1U); > >> + > >> + max_discard_sectors = min(q->limits.max_discard_sectors, UINT_MAX >> 9); > >> + max_discard_sectors -= max_discard_sectors % granularity; > >> + > >> + if (unlikely(!max_discard_sectors)) { > >> + /* XXX: warn */ > >> + return NULL; > >> + } > >> + > >> + if (bio_sectors(bio) <= max_discard_sectors) > >> + return NULL; > >> + > >> + split_sectors = max_discard_sectors; > >> + > >> + /* > >> + * If the next starting sector would be misaligned, stop the discard at > >> + * the previous aligned sector. > >> + */ > >> + alignment = (q->limits.discard_alignment >> 9) % granularity; > >> + > >> + tmp = bio->bi_iter.bi_sector + split_sectors - alignment; > >> + tmp = sector_div(tmp, granularity); > >> + > >> + if (split_sectors > tmp) > >> + split_sectors -= tmp; > >> + > >> + return bio_split(bio, split_sectors, GFP_NOIO, bs); > >> +} > > > > This code to stop the discard at the previous aligned sector could be > > the reason why I have 2 device-mapper-test-suite tests in the > > 'thin-provisioning' testsuite failing due to this patchset: > > I'm setting up the testsuite to debug. OK, once setup, to run the 2 tests in question directly you'd do something like: dmtest run --suite thin-provisioning -n discard_a_fragmented_device dmtest run --suite thin-provisioning -n discard_fully_provisioned_device_benchmark Again, these tests pass without this patchset.