On Tue, 2017-08-01 at 00:51 +0800, Ming Lei wrote:
> @@ -810,7 +810,11 @@ static void blk_mq_timeout_work(struct work_struct *work)
>
> struct ctx_iter_data {
> struct blk_mq_hw_ctx *hctx;
> - struct list_head *list;
> +
> + union {
> + struct list_head *list;
> + struct request *rq;
> + };
> };
Hello Ming,
Please introduce a new data structure for dispatch_rq_from_ctx() /
blk_mq_dispatch_rq_from_ctxs() instead of introducing a union in struct
ctx_iter_data. That will avoid that .list can be used in a context where
a struct request * pointer has been stored in the structure and vice versa.
> static bool flush_busy_ctx(struct sbitmap *sb, unsigned int bitnr, void *data)
> @@ -826,6 +830,26 @@ static bool flush_busy_ctx(struct sbitmap *sb, unsigned int bitnr, void *data)
> return true;
> }
>
> +static bool dispatch_rq_from_ctx(struct sbitmap *sb, unsigned int bitnr, void *data)
> +{
> + struct ctx_iter_data *dispatch_data = data;
> + struct blk_mq_hw_ctx *hctx = dispatch_data->hctx;
> + struct blk_mq_ctx *ctx = hctx->ctxs[bitnr];
> + bool empty = true;
> +
> + spin_lock(&ctx->lock);
> + if (unlikely(!list_empty(&ctx->rq_list))) {
> + dispatch_data->rq = list_entry_rq(ctx->rq_list.next);
> + list_del_init(&dispatch_data->rq->queuelist);
> + empty = list_empty(&ctx->rq_list);
> + }
> + spin_unlock(&ctx->lock);
> + if (empty)
> + sbitmap_clear_bit(sb, bitnr);
This sbitmap_clear_bit() occurs without holding blk_mq_ctx.lock. Sorry but
I don't think this is safe. Please either remove this sbitmap_clear_bit() call
or make sure that it happens with blk_mq_ctx.lock held.
Thanks,
Bart.
