Message ID | 8760rogwmp.fsf_-_@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
I think this looks fine, especially with the detailed comment:
Reviewed-by: Christoph Hellwig <hch@lst.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/29/2016 11:52 AM, Gabriel Krisman Bertazi wrote: > Jens Axboe <axboe@kernel.dk> writes: > >> I can see that is an issue. Did you consider the case where >> blk_mq_timeout_work() is entered, but we don't have any requests >> allocated that currently hold a reference? This could happen if >> completion races with a timeout. > > Hi Jens, > > That shouldn't be an issue, I think. The only functional difference > should be during queue freezes, and in this case, if the request was > already completed by the time we touch it, we`ll only hold the queue > freeze for a little longer, until we release the reference by the end of > the timeout path. should the final request be released before > blk_mq_timeout_work acquire it's reference, q_usage_counter will reach > zero, and the call to percpu_ref_tryget will fail the same way > percpu_ref_tryget_live would. Thanks, I'm happy with it, and the detailed comment is a good improvement. I have added it for this series.
diff --git a/block/blk-mq.c b/block/blk-mq.c index e22a0f4..2cc5b82 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -672,7 +672,20 @@ static void blk_mq_timeout_work(struct work_struct *work) }; int i; - if (blk_queue_enter(q, true)) + /* A deadlock might occur if a request is stuck requiring a + * timeout at the same time a queue freeze is waiting + * completion, since the timeout code would not be able to + * acquire the queue reference here. + * + * That's why we don't use blk_queue_enter here; instead, we use + * percpu_ref_tryget directly, because we need to be able to + * obtain a reference even in the short window between the queue + * starting to freeze, by dropping the first reference in + * blk_mq_freeze_queue_start, and the moment the last request is + * consumed, marked by the instant q_usage_counter reaches + * zero. + */ + if (!percpu_ref_tryget(&q->q_usage_counter)) return; blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &data);