Re: [PATCHv6 2/3] block: fix lockdep warning caused by lock dependency in elv_iosched_store

[Nilay Shroff <nilay@linux.ibm.com>]

On 6/30/25 11:50 AM, Hannes Reinecke wrote:

>> +struct elevator_tags *blk_mq_alloc_sched_tags(struct blk_mq_tag_set *set,
>> +        unsigned int nr_hw_queues)
>> +{
>> +    unsigned int nr_tags;
>> +    int i;
>> +    struct elevator_tags *et;
>> +    gfp_t gfp = GFP_NOIO | __GFP_ZERO | __GFP_NOWARN | __GFP_NORETRY;
>> +
>> +    if (blk_mq_is_shared_tags(set->flags))
>> +        nr_tags = 1;
>> +    else
>> +        nr_tags = nr_hw_queues;
>> +
>> +    et = kmalloc(sizeof(struct elevator_tags) +
>> +            nr_tags * sizeof(struct blk_mq_tags *), gfp);
>> +    if (!et)
>> +        return NULL;
>> +    /*
>> +     * Default to double of smaller one between hw queue_depth and
>> +     * 128, since we don't split into sync/async like the old code
>> +     * did. Additionally, this is a per-hw queue depth.
>> +     */
>> +    et->nr_requests = 2 * min_t(unsigned int, set->queue_depth,
>> +            BLKDEV_DEFAULT_RQ);
>> +    et->nr_hw_queues = nr_hw_queues;
>> +
>> +    if (blk_mq_is_shared_tags(set->flags)) {
>> +        /* Shared tags are stored at index 0 in @tags. */
>> +        et->tags[0] = blk_mq_alloc_map_and_rqs(set, BLK_MQ_NO_HCTX_IDX,
>> +                    MAX_SCHED_RQ);
>> +        if (!et->tags[0])
>> +            goto out;
>> +    } else {
>> +        for (i = 0; i < et->nr_hw_queues; i++) {
>> +            et->tags[i] = blk_mq_alloc_map_and_rqs(set, i,
>> +                    et->nr_requests);
>> +            if (!et->tags[i])
>> +                goto out_unwind;
>> +        }
>> +    }
>> +
>> +    return et;
>> +out_unwind:
>> +    while (--i >= 0)
>> +        blk_mq_free_map_and_rqs(set, et->tags[i], i);
>> +out:
>> +    kfree(et);
>> +    return NULL;
>> +}
>> +
> 
> As smatch stated, the unwind pattern is a bit odd.
> Maybe move the unwind into the 'else' branch, and us a conditional
> to invoke it:
> 
> if (i < et->nr_hw_queues)
>   while (--i >= 0)
>     blk_mq_free_map_and_request()
> 

I believe the 'if (i < et->nr_hw_queues)' check is unnecessary here. When
we jump to the @out_unwind label, @i is always less than @et->nr_hw_queues
because the for loop exits early (on allocation failure) before reaching
the upper bound. If @i had reached @et->nr_hw_queues, the loop would have
completed and we wouldn't jump to @out_unwind at all — we’d simply return 
@et instead.

The Smatch flagged the unwind loop due to the use of an unsigned @i in the 
previous patch. In that case, if the first allocation (i == 0) fails, then
'--i' underflows to UINT_MAX, and the condition '--i >= 0' is always true —
hence the warning.

This patch corrects that by declaring @i as a signed int, so that 
'--i >= 0' behaves as expected and avoids the Smatch warning.

So, I don't think an extra condition like 'if (i < et->nr_hw_queues)' is 
needed around the unwind loop. Agreed?

Thnaks,
--Nilay