Re: [PATCH 03/10] block: add a store_limit operations for sysfs entries

Damien Le Moal
2025-01-06T10:47:10Z

On 1/6/25 7:06 PM, Christoph Hellwig wrote:
> De-duplicate the code for updating queue limits by adding a store_limit
> method that allows having common code handle the actual queue limits
> update.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

[...]

> @@ -706,11 +687,24 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
> if (entry->load_module)
> entry->load_module(disk, page, length);
>
> - blk_mq_freeze_queue(q);
> mutex_lock(&q->sysfs_lock);
> - res = entry->store(disk, page, length);
> - mutex_unlock(&q->sysfs_lock);
> + blk_mq_freeze_queue(q);

The freeze must NOT be done for the "if (entry->store_limit)" case. So this
needs to go int the "else". Otherwise, you still have freeze the take limit
lock order which can cause the ABBA deadlocks in SCSI sd.

> + if (entry->store_limit) {
> + struct queue_limits lim = queue_limits_start_update(q);
> +
> + res = entry->store_limit(disk, page, length, &lim);
> + if (res < 0) {
> + queue_limits_cancel_update(q);
> + } else {
> + res = queue_limits_commit_update(q, &lim);

Here you must use queue_limits_commit_update_frozen().

> + if (!res)
> + res = length;
> + }
> + } else {
> + res = entry->store(disk, page, length);
> + }
> blk_mq_unfreeze_queue(q);

And this one needs to go in the else after the call to entry->store().

> + mutex_unlock(&q->sysfs_lock);
> return res;
> }
>

--
Damien Le Moal
Western Digital Research