From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758024Ab2ARQgo (ORCPT ); Wed, 18 Jan 2012 11:36:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:1025 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753397Ab2ARQgn (ORCPT ); Wed, 18 Jan 2012 11:36:43 -0500 Date: Wed, 18 Jan 2012 11:36:38 -0500 From: Vivek Goyal To: Jens Axboe Cc: Tejun Heo , Shaohua Li , linux kernel mailing list Subject: Re: Kernel crash in icq_free_icq_rcu Message-ID: <20120118163638.GD30204@redhat.com> References: <20120118010323.GA32160@htj.dyndns.org> <20120118011112.GB32160@htj.dyndns.org> <1326850253.22361.619.camel@sli10-conroe> <1326866602.22361.624.camel@sli10-conroe> <20120118135126.GB30204@redhat.com> <20120118142005.GC30204@redhat.com> <20120118160957.GB30664@google.com> <4F16F245.9000708@kernel.dk> <4F16F3CA.90904@kernel.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4F16F3CA.90904@kernel.dk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 18, 2012 at 05:31:06PM +0100, Jens Axboe wrote: > On 2012-01-18 17:24, Jens Axboe wrote: > > On 2012-01-18 17:09, Tejun Heo wrote: > >> On Wed, Jan 18, 2012 at 09:20:05AM -0500, Vivek Goyal wrote: > >>>> Not allocating icq if request is never going to go to elevator as elevator > >>>> switch was happening makes sense to me. > >>>> > >>>> I tried this patch. It went little further and crashed at a different > >>>> place. I think this seems to be separate merging issue Tejun is trying > >>>> to track down. > >>> > >>> Applied Tejun's debug patch to return early and not call into elevator > >>> for checking whether merge is allowed or not. Things seems to be stable > >>> now for me. > >> > >> Yeah, plug merge is calling into elevator code without any > >> synchronization, so it's bound to be broken. Given plugging is > >> per-task, I don't think we really need to query elevator about merging > >> bio's. The request is not on elevator and plugging is part of issuing > >> mechanism, not scheduling, after all. Jens, what do you think? > > > > Hmmm. We can bypass asking the elevator, as long as we query the > > restrictions. Does the below, by itself, resolve the crash? If yes, let > > me cook up a patch splitting the elv and blk rq merging logic. > > Something like the below, completely untested. > > But thinking about this a bit while doing it, why is the IO scheduler > going away while we have plugged requests that are elvpriv? Not calling ioscheduler during plug merge will allow merging of sync/async requests together. I guess we wouldn't want that. The only check we can skip in case of plug merge, is whether bio and rq beong to same task/cfqq or not. May be separate elevator functions for plug merge (without lock) and elevator merge (with lock) will do? Thanks Vivek > > diff --git a/block/blk-core.c b/block/blk-core.c > index e6c05a9..75eba5c 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -1283,7 +1283,7 @@ static bool attempt_plug_merge(struct request_queue *q, struct bio *bio, > if (rq->q != q) > continue; > > - el_ret = elv_try_merge(rq, bio); > + el_ret = blk_try_merge(rq, bio); > if (el_ret == ELEVATOR_BACK_MERGE) { > ret = bio_attempt_back_merge(q, rq, bio); > if (ret) > diff --git a/block/blk-merge.c b/block/blk-merge.c > index cfcc37c..ee9ec90 100644 > --- a/block/blk-merge.c > +++ b/block/blk-merge.c > @@ -471,3 +471,59 @@ int blk_attempt_req_merge(struct request_queue *q, struct request *rq, > { > return attempt_merge(q, rq, next); > } > + > +int blk_rq_merge_ok(struct request *rq, struct bio *bio) > +{ > + if (!rq_mergeable(rq)) > + return 0; > + > + /* > + * Don't merge file system requests and discard requests > + */ > + if ((bio->bi_rw & REQ_DISCARD) != (rq->bio->bi_rw & REQ_DISCARD)) > + return 0; > + > + /* > + * Don't merge discard requests and secure discard requests > + */ > + if ((bio->bi_rw & REQ_SECURE) != (rq->bio->bi_rw & REQ_SECURE)) > + return 0; > + > + /* > + * different data direction or already started, don't merge > + */ > + if (bio_data_dir(bio) != rq_data_dir(rq)) > + return 0; > + > + /* > + * must be same device and not a special request > + */ > + if (rq->rq_disk != bio->bi_bdev->bd_disk || rq->special) > + return 0; > + > + /* > + * only merge integrity protected bio into ditto rq > + */ > + if (bio_integrity(bio) != blk_integrity_rq(rq)) > + return 0; > + > + return 1; > +} > +EXPORT_SYMBOL(blk_rq_merge_ok); > + > +int blk_try_merge(struct request *__rq, struct bio *bio) > +{ > + int ret = ELEVATOR_NO_MERGE; > + > + /* > + * we can merge and sequence is ok, check if it's possible > + */ > + if (blk_rq_merge_ok(__rq, bio)) { > + if (blk_rq_pos(__rq) + blk_rq_sectors(__rq) == bio->bi_sector) > + ret = ELEVATOR_BACK_MERGE; > + else if (blk_rq_pos(__rq) - bio_sectors(bio) == bio->bi_sector) > + ret = ELEVATOR_FRONT_MERGE; > + } > + > + return ret; > +} > diff --git a/block/blk.h b/block/blk.h > index 7efd772..a117fa9 100644 > --- a/block/blk.h > +++ b/block/blk.h > @@ -137,6 +137,8 @@ int blk_attempt_req_merge(struct request_queue *q, struct request *rq, > struct request *next); > void blk_recalc_rq_segments(struct request *rq); > void blk_rq_set_mixed_merge(struct request *rq); > +int blk_rq_merge_ok(struct request *rq, struct bio *bio); > +int blk_try_merge(struct request *rq, struct bio *bio); > > void blk_queue_congestion_threshold(struct request_queue *q); > > diff --git a/block/elevator.c b/block/elevator.c > index 91e18f8..a1a75f7 100644 > --- a/block/elevator.c > +++ b/block/elevator.c > @@ -72,39 +72,8 @@ static int elv_iosched_allow_merge(struct request *rq, struct bio *bio) > */ > int elv_rq_merge_ok(struct request *rq, struct bio *bio) > { > - if (!rq_mergeable(rq)) > + if (!blk_rq_merge_ok(rq, bio)) > return 0; > - > - /* > - * Don't merge file system requests and discard requests > - */ > - if ((bio->bi_rw & REQ_DISCARD) != (rq->bio->bi_rw & REQ_DISCARD)) > - return 0; > - > - /* > - * Don't merge discard requests and secure discard requests > - */ > - if ((bio->bi_rw & REQ_SECURE) != (rq->bio->bi_rw & REQ_SECURE)) > - return 0; > - > - /* > - * different data direction or already started, don't merge > - */ > - if (bio_data_dir(bio) != rq_data_dir(rq)) > - return 0; > - > - /* > - * must be same device and not a special request > - */ > - if (rq->rq_disk != bio->bi_bdev->bd_disk || rq->special) > - return 0; > - > - /* > - * only merge integrity protected bio into ditto rq > - */ > - if (bio_integrity(bio) != blk_integrity_rq(rq)) > - return 0; > - > if (!elv_iosched_allow_merge(rq, bio)) > return 0; > > @@ -114,19 +83,13 @@ EXPORT_SYMBOL(elv_rq_merge_ok); > > int elv_try_merge(struct request *__rq, struct bio *bio) > { > - int ret = ELEVATOR_NO_MERGE; > - > /* > * we can merge and sequence is ok, check if it's possible > */ > - if (elv_rq_merge_ok(__rq, bio)) { > - if (blk_rq_pos(__rq) + blk_rq_sectors(__rq) == bio->bi_sector) > - ret = ELEVATOR_BACK_MERGE; > - else if (blk_rq_pos(__rq) - bio_sectors(bio) == bio->bi_sector) > - ret = ELEVATOR_FRONT_MERGE; > - } > + if (elv_rq_merge_ok(__rq, bio)) > + return blk_try_merge(__rq, bio); > > - return ret; > + return ELEVATOR_NO_MERGE; > } > > static struct elevator_type *elevator_find(const char *name) > > -- > Jens Axboe