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: Sat, 1 Aug 2015 12:33:57 -0400 Message-ID: <20150801163356.GA21478@redhat.com> References: <1436168690-32102-1-git-send-email-mlin@kernel.org> <20150731192337.GA8907@redhat.com> <20150731213831.GA16464@redhat.com> <1438412290.26596.14.camel@hasee> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1438412290.26596.14.camel@hasee> Sender: linux-kernel-owner@vger.kernel.org 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 List-Id: dm-devel.ids On Sat, Aug 01 2015 at 2:58am -0400, Ming Lin wrote: > On Fri, 2015-07-31 at 17:38 -0400, Mike Snitzer wrote: > > > > 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. > > It's caused by patch 4. > When discard size >=4G, the bio->bi_iter.bi_size overflows. Thanks for tracking this down! > Below is the new patch. > > Christoph, > Could you also help to review it? > > Now we still do "misaligned" check in blkdev_issue_discard(). > So the same code in blk_bio_discard_split() was removed. But I don't agree with this approach. One of the most meaningful benefits of late bio splitting is the upper layers shouldn't _need_ to depend on the intermediate devices' queue_limits being stacked properly. Your solution to mix discard granularity/alignment checks at the upper layer(s) but then split based on max_discard_sectors at the lower layer defeats that benefit for discards. This will translate to all intermediate layers that might split discards needing to worry about granularity/alignment too (e.g. how dm-thinp will have to care because it must generate discard mappings with associated bios based on how blocks were mapped to thinp). Also, it is unfortunate that IO that doesn't have a payload is being artificially split simply because bio->bi_iter.bi_size is 32bits. Mike From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Sat, 1 Aug 2015 12:33:57 -0400 From: Mike Snitzer To: Ming Lin Message-ID: <20150801163356.GA21478@redhat.com> References: <1436168690-32102-1-git-send-email-mlin@kernel.org> <20150731192337.GA8907@redhat.com> <20150731213831.GA16464@redhat.com> <1438412290.26596.14.camel@hasee> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1438412290.26596.14.camel@hasee> Cc: Neil Brown , Ming Lei , dm-devel@redhat.com, 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@lists.linbit.com Subject: Re: [Drbd-dev] [PATCH v5 01/11] block: make generic_make_request handle arbitrarily sized bios List-Id: "*Coordination* of development, patches, contributions -- *Questions* \(even to developers\) go to drbd-user, please." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Sat, Aug 01 2015 at 2:58am -0400, Ming Lin wrote: > On Fri, 2015-07-31 at 17:38 -0400, Mike Snitzer wrote: > > > > 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. > > It's caused by patch 4. > When discard size >=4G, the bio->bi_iter.bi_size overflows. Thanks for tracking this down! > Below is the new patch. > > Christoph, > Could you also help to review it? > > Now we still do "misaligned" check in blkdev_issue_discard(). > So the same code in blk_bio_discard_split() was removed. But I don't agree with this approach. One of the most meaningful benefits of late bio splitting is the upper layers shouldn't _need_ to depend on the intermediate devices' queue_limits being stacked properly. Your solution to mix discard granularity/alignment checks at the upper layer(s) but then split based on max_discard_sectors at the lower layer defeats that benefit for discards. This will translate to all intermediate layers that might split discards needing to worry about granularity/alignment too (e.g. how dm-thinp will have to care because it must generate discard mappings with associated bios based on how blocks were mapped to thinp). Also, it is unfortunate that IO that doesn't have a payload is being artificially split simply because bio->bi_iter.bi_size is 32bits. Mike