From: Liu Song <[email protected]>
If "blk_mq_get_tag" returns BLK_MQ_NO_TAG because the value of
"tags->nr_reserved_tags" is 0, it will fall into an infinite loop in
"__blk_mq_alloc_requests", so borrow BLK_MQ_REQ_NOWAIT to exit the loop.
Because "blk_mq_alloc_data" objects are allocated on the stack, changing
the content of flags will not generate extra impact.
Signed-off-by: Liu Song <[email protected]>
---
block/blk-mq-tag.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index 2dcd738..6f1d6e6 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c
@@ -139,6 +139,7 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
if (data->flags & BLK_MQ_REQ_RESERVED) {
if (unlikely(!tags->nr_reserved_tags)) {
WARN_ON_ONCE(1);
+ data->flags |= BLK_MQ_REQ_NOWAIT;
return BLK_MQ_NO_TAG;
}
bt = &tags->breserved_tags;
--
1.8.3.1
On Fri, Jul 22, 2022 at 01:22:23PM +0800, Liu Song wrote:
> From: Liu Song <[email protected]>
>
> If "blk_mq_get_tag" returns BLK_MQ_NO_TAG because the value of
> "tags->nr_reserved_tags" is 0, it will fall into an infinite loop in
> "__blk_mq_alloc_requests", so borrow BLK_MQ_REQ_NOWAIT to exit the loop.
That means the driver calling blk_mq_alloc_request has a bug, and
we should not work round that in the low level tag allocation path.
If we want to be nice we can add a WARN_ON before going all the way
down into the tag allocator, something like:
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 92aae03103b74..d6c7e2ece025f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -520,6 +520,10 @@ struct request *blk_mq_alloc_request(struct request_queue *q, unsigned int op,
struct request *rq;
int ret;
+ if (WARN_ON_ONCE((flags & BLK_MQ_REQ_RESERVED) &&
+ !q->tag_set->reserved_tags))
+ return ERR_PTR(-EINVAL);
+
ret = blk_queue_enter(q, flags);
if (ret)
return ERR_PTR(ret);
> On Fri, Jul 22, 2022 at 01:22:23PM +0800, Liu Song wrote:
>> From: Liu Song <[email protected]>
>>
>> If "blk_mq_get_tag" returns BLK_MQ_NO_TAG because the value of
>> "tags->nr_reserved_tags" is 0, it will fall into an infinite loop in
>> "__blk_mq_alloc_requests", so borrow BLK_MQ_REQ_NOWAIT to exit the loop.
> That means the driver calling blk_mq_alloc_request has a bug, and
> we should not work round that in the low level tag allocation path.
>
> If we want to be nice we can add a WARN_ON before going all the way
> down into the tag allocator, something like:
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 92aae03103b74..d6c7e2ece025f 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -520,6 +520,10 @@ struct request *blk_mq_alloc_request(struct request_queue *q, unsigned int op,
> struct request *rq;
> int ret;
>
> + if (WARN_ON_ONCE((flags & BLK_MQ_REQ_RESERVED) &&
> + !q->tag_set->reserved_tags))
> + return ERR_PTR(-EINVAL);
> +
Hi，
It is a reasonable approach to prevent abnormal alloc from going down,
but this is a very rare exception after all, and above modification is
checked
every alloc request, which seems to be a bit excessive overhead.
After the rare exception occurs and fix it, the impact on the alloc
request will be lower.
Thanks
> ret = blk_queue_enter(q, flags);
> if (ret)
> return ERR_PTR(ret);
On Fri, Jul 22, 2022 at 04:15:49PM +0800, Liu Song wrote:
> It is a reasonable approach to prevent abnormal alloc from going down,
> but this is a very rare exception after all, and above modification is
> checked
> every alloc request, which seems to be a bit excessive overhead.
Every allocation of a passthrough requests. Which isn't really the
normal fast path.
> On Fri, Jul 22, 2022 at 04:15:49PM +0800, Liu Song wrote:
>> It is a reasonable approach to prevent abnormal alloc from going down,
>> but this is a very rare exception after all, and above modification is
>> checked
>> every alloc request, which seems to be a bit excessive overhead.
> Every allocation of a passthrough requests. Which isn't really the
> normal fast path.
Hi,
It is true that there is no possibility of triggering an infinite loop
in the current code,
but the necessary guard code should also exist without adding extra
overhead.
Thanks