From patchwork Thu Jun 14 20:08:09 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Snitzer X-Patchwork-Id: 10465225 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id C813F603EE for ; Thu, 14 Jun 2018 20:08:21 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6990D28517 for ; Thu, 14 Jun 2018 20:08:21 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 59C8528531; Thu, 14 Jun 2018 20:08:21 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EBD8928C0C for ; Thu, 14 Jun 2018 20:08:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755280AbeFNUIU (ORCPT ); Thu, 14 Jun 2018 16:08:20 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:47262 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755265AbeFNUIT (ORCPT ); Thu, 14 Jun 2018 16:08:19 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4DF1B407541D; Thu, 14 Jun 2018 20:08:19 +0000 (UTC) Received: from localhost (unknown [10.16.197.123]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B916D23148; Thu, 14 Jun 2018 20:08:14 +0000 (UTC) Date: Thu, 14 Jun 2018 16:08:09 -0400 From: Mike Snitzer To: Christoph Hellwig , NeilBrown Cc: linux-block@vger.kernel.org, dm-devel@redhat.com, Ming Lei Subject: Re: why does __split_and_process_bio use bio_clone_bioset? Message-ID: <20180614200808.GA46373@redhat.com> References: <20180614081947.GA23375@infradead.org> <20180614181245.GA1161@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180614181245.GA1161@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 14 Jun 2018 20:08:19 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 14 Jun 2018 20:08:19 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'msnitzer@redhat.com' RCPT:'' Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jun 14 2018 at 2:12P -0400, Mike Snitzer wrote: > On Thu, Jun 14 2018 at 4:19am -0400, > Christoph Hellwig wrote: > > > Hi Neil, > > > > In commit 18a25da8 ("dm: ensure bio submission follows a depth-first > > tree walk") you've added a call to bio_clone_bioset to > > __split_and_process_bio. Unlike all other bio splitting code this > > actually allocates a new bio_vec array instead of just splitting the bio > > and the iterator. I can't actually find a good reason for that either > > in a cursory review of the code, the commit or the comments. > > > > Do you remember why this can't just use bio_clone_fast? > > Your question caused me to revisit this code and it is suspect for a > couple reasons: > > 1) I'm also not seeing why we need bio_clone_bioset() The patch below seems to work fine (given quick testing).. It also has a side-effect of not breaking integrity support (which commit 18a25da8 appears to do because it isn't accounting for any of the integrity stuff bio_split, or dm.c:clone_bio, does). FYI, my other concerns in my my previous reply were unfounded and due to misreading the existing code. Neil, please still feel free to have a look at this to see if you can recall why you used bio_clone_bioset(). If in the end you agree that the following patch is fine please let me know and I'll get a proper fix staged. Thanks, Mike diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 20a8d63..dfb4783 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1582,10 +1582,9 @@ static blk_qc_t __split_and_process_bio(struct mapped_device *md, * the usage of io->orig_bio in dm_remap_zone_report() * won't be affected by this reassignment. */ - struct bio *b = bio_clone_bioset(bio, GFP_NOIO, - &md->queue->bio_split); + struct bio *b = bio_split(bio, bio_sectors(bio) - ci.sector_count, + GFP_NOIO, &md->queue->bio_split); ci.io->orig_bio = b; - bio_advance(bio, (bio_sectors(bio) - ci.sector_count) << 9); bio_chain(b, bio); ret = generic_make_request(bio); break;