| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120118163638.GD30204@redhat.com>
Date: Wed, 18 Jan 2012 11:36:38 -0500
From: Vivek Goyal <vgoyal@...hat.com>
To: Jens Axboe <axboe@...nel.dk>
Cc: Tejun Heo <tj@...nel.org>, Shaohua Li <shaohua.li@...el.com>,
linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: Kernel crash in icq_free_icq_rcu
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists