Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754501AbcDTNsR (ORCPT ); Wed, 20 Apr 2016 09:48:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53544 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752806AbcDTNsP (ORCPT ); Wed, 20 Apr 2016 09:48:15 -0400 From: Vitaly Kuznetsov To: Ming Lei Cc: Keith Busch , linux-block@vger.kernel.org, Linux Kernel Mailing List , Jens Axboe , Dan Williams , "Martin K. Petersen" , Sagi Grimberg , Mike Snitzer , "K. Y. Srinivasan" , Cathy Avery Subject: Re: [PATCH RFC] block: fix bio merge checks when virt_boundary is set References: <1458055076-2175-1-git-send-email-vkuznets@redhat.com> <87oaae4cej.fsf@vitty.brq.redhat.com> <20160316223804.GA6217@localhost.lm.intel.com> <87egb94agz.fsf@vitty.brq.redhat.com> <20160317163946.GC6217@localhost.lm.intel.com> Date: Wed, 20 Apr 2016 15:48:10 +0200 In-Reply-To: (Ming Lei's message of "Wed, 30 Mar 2016 21:07:19 +0800") Message-ID: <87shyg1jdx.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2647 Lines: 75 Ming Lei writes: > On Fri, Mar 18, 2016 at 10:59 AM, Ming Lei wrote: >> On Fri, Mar 18, 2016 at 12:39 AM, Keith Busch wrote: >>> On Thu, Mar 17, 2016 at 12:20:28PM +0100, Vitaly Kuznetsov wrote: >>>> Keith Busch writes: >>>> > been combined. In any case, I think you can get what you're after just >>>> > by moving the gap check after BIOVEC_PHYS_MERGABLE. Does the following >>>> > look ok to you? >>>> > >>>> >>>> Thanks, it does. >>> >>> Cool, thanks for confirming. >>> >>>> Will you send it or would you like me to do that with your Suggested-by? >>> >>> I'm not confident yet this doesn't break anything, particularly since >>> we moved the gap check after the length check. Just wanted to confirm >>> the concept addressed your concern, but still need to take a closer look >>> and test before submitting. >> >> IMO, the change on blk_bio_segment_split() is correct, because actually it >> is a sg gap and the check should have been done between segments >> instead of bvecs. So it is reasonable to move the check just before populating >> a new segment. > > Thinking of the 1st part change further, looks it is just correct in concept, > but wrong from current implementation. Because of bios/reqs merge, > blk_rq_map_sg() may end one segment in any bvec in theroy, so I guess > that is why each non-1st bvec need the check to make sure no sg gap. > Looks a very crazy limit, :-) > >> >> But for the 2nd change in bio_will_gap(), which should fix Vitaly's problem, I >> am still not sure if it is completely correct. bio_will_gap() is used >> to check if two >> bios may be merged. Suppose two bios are continues physically, the last bvec >> in 1st bio and the first bvec in 2nd bio might not be in one same segment >> because of segment size limit. > > How about the attached patch? > I just wanted to revive the discussion as the issue persists. I re-tested your patch against 4.6-rc4 and it efficiently solves the issue. pre-patch: # time mkfs.ntfs /dev/sdb1 Cluster size has been automatically set to 4096 bytes. Initializing device with zeroes: 100% - Done. Creating NTFS volume structures. mkntfs completed successfully. Have a nice day. real8m10.977s user0m0.115s sys0m12.672s post-patch: # time mkfs.ntfs /dev/sdb1 Cluster size has been automatically set to 4096 bytes. Initializing device with zeroes: 100% - Done. Creating NTFS volume structures. mkntfs completed successfully. Have a nice day. real0m42.430s user0m0.171s sys0m7.675s Will you send this patch? Please let me know if I can further assist. Thanks! -- Vitaly