From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:42766 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750705AbeAaFZQ (ORCPT ); Wed, 31 Jan 2018 00:25:16 -0500 Date: Wed, 31 Jan 2018 13:24:58 +0800 From: Ming Lei To: Jiri Palecek Cc: Ming Lei , linux-block , Jens Axboe , Christoph Hellwig Subject: Re: [PATCH] Use bio_endio instead of bio_put in error path of blk_rq_append_bio Message-ID: <20180131052448.GB9985@ming.t460p> References: <20171218074044.1369-3-ming.lei@redhat.com> <87o9lcp1aq.fsf@debian.i-did-not-set--mail-host-address--so-tickle-me> <2c6a3916-57b8-20cb-f081-bb1b907a1a8a@web.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 In-Reply-To: <2c6a3916-57b8-20cb-f081-bb1b907a1a8a@web.de> Sender: linux-block-owner@vger.kernel.org List-Id: linux-block@vger.kernel.org On Tue, Jan 30, 2018 at 04:24:14PM +0100, Jiri Palecek wrote: > > On 1/30/18 1:53 PM, Ming Lei wrote: > > On Thu, Jan 25, 2018 at 9:58 PM, Jiří Paleček wrote: > > > Avoids page leak from bounced requests > > > --- > > > block/blk-map.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/block/blk-map.c b/block/blk-map.c > > > index d3a94719f03f..702d68166689 100644 > > > --- a/block/blk-map.c > > > +++ b/block/blk-map.c > > > @@ -26,7 +26,8 @@ int blk_rq_append_bio(struct request *rq, struct bio **bio) > > > } else { > > > if (!ll_back_merge_fn(rq->q, rq, *bio)) { > > > if (orig_bio != *bio) { > > > - bio_put(*bio); > > > + bio_inc_remaining(orig_bio); > > > + bio_endio(*bio); > > 'bio_inc_remaining(orig_bio);' shouldn't be needed since we don't chain bounced > > bio, otherwise this patch is fine. > > I believe it is needed or at least desirable. The situation when the request > bounced is like this > > bio (bounced) . bi_private ---> orig_bio > > and at the end of bounce_end_io, bio_endio(bio->bi_private) [which is > bio_endio(orig_bio) in our case] is called. That doesn't have any effect on > __blk_rq_map_user_iov; its bios have .bi_end_io==0, therefore one call more > or less doesn't matter. However, for other callers, like osd_initiator.c, > this is not the case. They pass bios which have bi_end_io, and might be > surprised if this was called unexpectedly. OK, I think it is good to follow the rule of not calling .bi_end_io() in the failure path, just like __blk_rq_map_user_iov()/blk_rq_map_kern(). But seems pscsi_map_sg() depends on bio_endio(orig_bio) to free the 'orig_bio', could you fix it in this patch too? > > Before  caa4b02476e3, blk_rq_append_request wouldn't touch/delete the passed > bio at all under any circumstances. I believe it should stay that way and > incrementing the remaining counter, which effectively nullifies the extra > bio_endio call, does that pretty straightforwardly. Seems too tricky to use bio_inc_remaining() for avoiding bio_endio(orig_bio), if we have to do that, I suggest to add comment on that. Thanks, Ming