On 1/28/25 11:19 AM, Christoph Hellwig wrote:
> On Thu, Jan 23, 2025 at 11:10:24PM +0530, Nilay Shroff wrote:
>>
>> + mutex_lock(&q->tag_set->tag_list_lock);
>> +
>> queue_for_each_hw_ctx(q, hctx, i) {
>> ret = blk_mq_register_hctx(hctx);
>> - if (ret)
>> + if (ret) {
>> + mutex_unlock(&q->tag_set->tag_list_lock);
>> goto unreg;
>> + }
>> }
>>
>> + mutex_unlock(&q->tag_set->tag_list_lock);
>>
>> out:
>> return ret;
>>
>> unreg:
>> + mutex_lock(&q->tag_set->tag_list_lock);
>> +
>
> No real need for a unlock/lock cycle here I think. Also as something
> that is really just a nitpick, I love to keep the locks for the critical
> sections close to the code they're protecting. e.g. format this as:
>
> if (ret < 0)
> return ret;
>
> mutex_lock(&q->tag_set->tag_list_lock);
> queue_for_each_hw_ctx(q, hctx, i) {
> ret = blk_mq_register_hctx(hctx);
> if (ret)
> goto out_unregister;
> }
> mutex_unlock(&q->tag_set->tag_list_lock);
> return 0
>
> out_unregister:
> queue_for_each_hw_ctx(q, hctx, j) {
> if (j < i)
> blk_mq_unregister_hctx(hctx);
> }
> mutex_unlock(&q->tag_set->tag_list_lock);
>
> ...
>
> (and similar for blk_mq_sysfs_unregister).
Yes looks good. I will update code and send next patch.
> I assume you did run this through blktests and xfstests with lockdep
> enabled to catch if we created some new lock ordering problems?
> I can't think of any right now, but it's good to validate that.
>
Yeah I ran blktests with lockdep enabled before submitting patch to
ensure that lockdep doesn't generate any waning with changes. However
I didn't run xfstests. Anyways, I would now run both blktests and
xfstests before submitting the next patch.
Thanks,
--Nilay
