Re: [PATCH] block: mq flush: fix race between IPI handler and mq flush worker

[Ming Lei]

On Tue, May 20, 2014 at 11:23 PM, Christoph Hellwig <hch@xxxxxx> wrote:
> On Tue, May 20, 2014 at 11:20:25AM +0800, Ming Lei wrote:
>> - the conflict on the two structures just happens with flush
>> requests because rq->requeue_work is only used to queue
>> flush requests
>
> Once we get non-trivial block drivers we'll need to be able
> to requeue arbitrary requests, that's why I added blk_mq_requeue_request.

I am wondering if virtio-blk is trivial block driver, :-)
And virtio-blk still supports requeuing with returning
BLK_MQ_RQ_QUEUE_BUSY.

> The scsi-mq work that I plant to submit for the next merge window is
> the prime example.

It depends if one scsi-mq req has to requeue itself with rq->requeue_work
inside its own .softirq_done_fn. If yes, we can't put call_single_data
and requeue_work into one union simply. From you last scsi-mq post,
looks the request may do that if I understand correctly.

But scsi_cmnd has already one 'abort_work', and I am wondering
why scsi-mq requeuing doesn't share its own requeue_work with
abort_work, which seems doable since requeuing and aborting belong
to different stage.

>
> I'd really prefer to avoid breaking that use case if we can avoid it.

This patch won't break the coming scsi-mq, and it is a fix.

I'd like to figure out one patch to cover scsi-mq case if
we can get that before 3.15 release since your 'respect-affinity'
patch has enabled IPI at default already. Otherwise, we
still have enough time to fix the issue for scsi-mq, don't we?

>
>
> Note that the flush code already is very nasy for blk-mq and this just
> makes it worse.

I think the patch is clean and simple, with documenting the special
conflict case clearly too.

>
> One fix that would also help to sort out some of the flush issues would
> be to add a list of requests that need requeueing to the blk_mq context,
> which we can add requeusts to from irq context.  The next time we run

That won't be easy to introduce a requeue_list for the purpose
since we need to keep order of requests per blk-mq ctx.

Also lockless list won't work since there are both
'add_front'/'add_tail' requirement.

> hw contexts for the queue we'll pick them up in user context and insert
> them.

IMO, the requests can be inserted to ctx list directly from irq
context, but with cost of spin_lock_irqsave(&ctx->lock) everywhere.

Follows current ideas:
1), this patch with scsi-mq sharing abort_work together?
2), move requeue_work out of the union inside request
3), spin_lock_irqsave(&ctx->lock) everywhere and requeue
request directly to ctx without using work

Any other ideas?

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/