From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756701AbbGaWCa (ORCPT ); Fri, 31 Jul 2015 18:02:30 -0400 Received: from mail.kernel.org ([198.145.29.136]:33333 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753136AbbGaWC3 (ORCPT ); Fri, 31 Jul 2015 18:02:29 -0400 Message-ID: <1438380135.7974.4.camel@ssi> Subject: Re: [PATCH v5 01/11] block: make generic_make_request handle arbitrarily sized bios From: Ming Lin To: Mike Snitzer Cc: lkml Date: Fri, 31 Jul 2015 15:02:15 -0700 In-Reply-To: <20150731213831.GA16464@redhat.com> References: <1436168690-32102-1-git-send-email-mlin@kernel.org> <20150731192337.GA8907@redhat.com> <20150731213831.GA16464@redhat.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2015-07-31 at 17:38 -0400, Mike Snitzer wrote: > 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. (trim CC list) I followed https://github.com/jthornber/device-mapper-test-suite https://github.com/jthornber/thin-provisioning-tools to setup. $ grep CONFIG_DM_ .config CONFIG_DM_MQ_DEFAULT=y CONFIG_DM_DEBUG=y CONFIG_DM_BUFIO=y CONFIG_DM_BIO_PRISON=y CONFIG_DM_PERSISTENT_DATA=y # CONFIG_DM_DEBUG_BLOCK_STACK_TRACING is not set CONFIG_DM_CRYPT=y CONFIG_DM_SNAPSHOT=y CONFIG_DM_THIN_PROVISIONING=y CONFIG_DM_CACHE=y CONFIG_DM_CACHE_MQ=y CONFIG_DM_CACHE_SMQ=y CONFIG_DM_CACHE_CLEANER=y CONFIG_DM_ERA=y CONFIG_DM_MIRROR=y CONFIG_DM_LOG_USERSPACE=y CONFIG_DM_RAID=y CONFIG_DM_ZERO=y CONFIG_DM_MULTIPATH=y CONFIG_DM_MULTIPATH_QL=y CONFIG_DM_MULTIPATH_ST=y CONFIG_DM_DELAY=y CONFIG_DM_UEVENT=y CONFIG_DM_FLAKEY=y CONFIG_DM_VERITY=y CONFIG_DM_SWITCH=y CONFIG_DM_LOG_WRITES=y # cat .dmtest/config profile :ssd do metadata_dev '/dev/vda' data_dev '/dev/vdb' end # # profile :spindle do # metadata_dev '/dev/vdd' # data_dev '/dev/vde' # end # # profile :mix do # metadata_dev '/dev/vdb' # data_dev '/dev/vde' # end # # default_profile :ssd # default_test_scale :normal root@bee:~/device-mapper-test-suite# dmtest run --suite thin-provisioning -n discard_a_fragmented_device Loaded suite thin-provisioning Started DiscardQuickTests discard_a_fragmented_device...restored metadata FAIL Finished in 1.958143844 seconds. 1) Error: test_discard_a_fragmented_device(DiscardQuickTests): ProcessControl::ExitError: command failed: dmsetup load test-dev-307057 /tmp/dm-table20150731-3386-1rncpsb /root/device-mapper-test-suite/lib/dmtest/process.rb:96:in `wait' /root/device-mapper-test-suite/lib/dmtest/process.rb:111:in `really_run' /root/device-mapper-test-suite/lib/dmtest/process.rb:116:in `run_' /root/device-mapper-test-suite/lib/dmtest/process.rb:138:in `run' /root/device-mapper-test-suite/lib/dmtest/device-mapper/interface.rb:16:in `block in load' /root/device-mapper-test-suite/lib/dmtest/utils.rb:94:in `with_temp_file' /root/device-mapper-test-suite/lib/dmtest/device-mapper/interface.rb:12:in `load' /root/device-mapper-test-suite/lib/dmtest/device-mapper/dev.rb:19:in `load' /root/device-mapper-test-suite/lib/dmtest/device-mapper/lexical_operators.rb:53:in `block in create' /root/device-mapper-test-suite/lib/dmtest/prelude.rb:39:in `protect_' /root/device-mapper-test-suite/lib/dmtest/device-mapper/lexical_operators.rb:47:in `create' /root/device-mapper-test-suite/lib/dmtest/device-mapper/lexical_operators.rb:10:in `with_dev' /root/device-mapper-test-suite/lib/dmtest/thinp-mixin.rb:147:in `with_fake_discard' /root/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:109:in `with_discardable_pool' /root/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:253:in `test_discard_a_fragmented_device' 1 tests, 0 assertions, 0 failures, 1 errors Tests failed root@bee:~/device-mapper-test-suite# dmesg [ 83.824523] device-mapper: table: 249:0: fake-discard: unknown target type [ 83.827539] device-mapper: ioctl: error adding target to table Seems my setup has problem. Could you help to take a look?