Currently rq-qos debugfs entries is created from rq_qos_add(), while
rq_qos_add() requires queue to be freezed. This can deadlock because
creating new entries can trigger fs reclaim.

Fix this problem by delaying creating rq-qos debugfs entries until
it's initialization is complete.

- For wbt, it can be initialized by default of by blk-sysfs, fix it by
  calling blk_mq_debugfs_register_rq_qos() after wbt_init;
- For other policies, they can only be initialized by blkg configuration,
  fix it by calling blk_mq_debugfs_register_rq_qos() from
  blkg_conf_end();

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-cgroup.c | 6 ++++++
 block/blk-rq-qos.c | 7 -------
 block/blk-sysfs.c  | 4 ++++
 block/blk-wbt.c    | 7 ++++++-
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index d93654334854..e4ccabf132c0 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -33,6 +33,7 @@
 #include "blk-cgroup.h"
 #include "blk-ioprio.h"
 #include "blk-throttle.h"
+#include "blk-mq-debugfs.h"
 
 static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu);
 
@@ -746,6 +747,11 @@ void blkg_conf_end(struct blkg_conf_ctx *ctx)
 	mutex_unlock(&q->elevator_lock);
 	blk_mq_unfreeze_queue(q, ctx->memflags);
 	blkdev_put_no_open(ctx->bdev);
+
+	mutex_lock(&q->debugfs_mutex);
+	blk_mq_debugfs_register_rq_qos(q);
+	mutex_unlock(&q->debugfs_mutex);
+
 }
 EXPORT_SYMBOL_GPL(blkg_conf_end);
 
diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
index 654478dfbc20..d7ce99ce2e80 100644
--- a/block/blk-rq-qos.c
+++ b/block/blk-rq-qos.c
@@ -347,13 +347,6 @@ int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
 	blk_queue_flag_set(QUEUE_FLAG_QOS_ENABLED, q);
 
 	blk_mq_unfreeze_queue(q, memflags);
-
-	if (rqos->ops->debugfs_attrs) {
-		mutex_lock(&q->debugfs_mutex);
-		blk_mq_debugfs_register_rqos(rqos);
-		mutex_unlock(&q->debugfs_mutex);
-	}
-
 	return 0;
 ebusy:
 	blk_mq_unfreeze_queue(q, memflags);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 76c47fe9b8d6..52bb4db25cf5 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -688,6 +688,10 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page,
 	mutex_unlock(&disk->rqos_state_mutex);
 
 	blk_mq_unquiesce_queue(q);
+
+	mutex_lock(&q->debugfs_mutex);
+	blk_mq_debugfs_register_rq_qos(q);
+	mutex_unlock(&q->debugfs_mutex);
 out:
 	blk_mq_unfreeze_queue(q, memflags);
 
diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index eb8037bae0bd..a120b5ba54db 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -724,8 +724,13 @@ void wbt_enable_default(struct gendisk *disk)
 	if (!blk_queue_registered(q))
 		return;
 
-	if (queue_is_mq(q) && enable)
+	if (queue_is_mq(q) && enable) {
 		wbt_init(disk);
+
+		mutex_lock(&q->debugfs_mutex);
+		blk_mq_debugfs_register_rq_qos(q);
+		mutex_unlock(&q->debugfs_mutex);
+	}
 }
 EXPORT_SYMBOL_GPL(wbt_enable_default);
 
-- 
2.39.2

On Tue, Oct 14, 2025 at 10:21:48AM +0800, Yu Kuai wrote:
> Currently rq-qos debugfs entries is created from rq_qos_add(), while
> rq_qos_add() requires queue to be freezed. This can deadlock because
> creating new entries can trigger fs reclaim.
> 
> Fix this problem by delaying creating rq-qos debugfs entries until
> it's initialization is complete.
> 
> - For wbt, it can be initialized by default of by blk-sysfs, fix it by
>   calling blk_mq_debugfs_register_rq_qos() after wbt_init;
> - For other policies, they can only be initialized by blkg configuration,
>   fix it by calling blk_mq_debugfs_register_rq_qos() from
>   blkg_conf_end();
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  block/blk-cgroup.c | 6 ++++++
>  block/blk-rq-qos.c | 7 -------
>  block/blk-sysfs.c  | 4 ++++
>  block/blk-wbt.c    | 7 ++++++-
>  4 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index d93654334854..e4ccabf132c0 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -33,6 +33,7 @@
>  #include "blk-cgroup.h"
>  #include "blk-ioprio.h"
>  #include "blk-throttle.h"
> +#include "blk-mq-debugfs.h"
>  
>  static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu);
>  
> @@ -746,6 +747,11 @@ void blkg_conf_end(struct blkg_conf_ctx *ctx)
>  	mutex_unlock(&q->elevator_lock);
>  	blk_mq_unfreeze_queue(q, ctx->memflags);
>  	blkdev_put_no_open(ctx->bdev);
> +
> +	mutex_lock(&q->debugfs_mutex);
> +	blk_mq_debugfs_register_rq_qos(q);
> +	mutex_unlock(&q->debugfs_mutex);
> +
>  }
>  EXPORT_SYMBOL_GPL(blkg_conf_end);
>  
> diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
> index 654478dfbc20..d7ce99ce2e80 100644
> --- a/block/blk-rq-qos.c
> +++ b/block/blk-rq-qos.c
> @@ -347,13 +347,6 @@ int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
>  	blk_queue_flag_set(QUEUE_FLAG_QOS_ENABLED, q);
>  
>  	blk_mq_unfreeze_queue(q, memflags);
> -
> -	if (rqos->ops->debugfs_attrs) {
> -		mutex_lock(&q->debugfs_mutex);
> -		blk_mq_debugfs_register_rqos(rqos);
> -		mutex_unlock(&q->debugfs_mutex);
> -	}
> -
>  	return 0;
>  ebusy:
>  	blk_mq_unfreeze_queue(q, memflags);
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 76c47fe9b8d6..52bb4db25cf5 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -688,6 +688,10 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page,
>  	mutex_unlock(&disk->rqos_state_mutex);
>  
>  	blk_mq_unquiesce_queue(q);
> +
> +	mutex_lock(&q->debugfs_mutex);
> +	blk_mq_debugfs_register_rq_qos(q);
> +	mutex_unlock(&q->debugfs_mutex);
>  out:
>  	blk_mq_unfreeze_queue(q, memflags);
>  
> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> index eb8037bae0bd..a120b5ba54db 100644
> --- a/block/blk-wbt.c
> +++ b/block/blk-wbt.c
> @@ -724,8 +724,13 @@ void wbt_enable_default(struct gendisk *disk)
>  	if (!blk_queue_registered(q))
>  		return;
>  
> -	if (queue_is_mq(q) && enable)
> +	if (queue_is_mq(q) && enable) {
>  		wbt_init(disk);
> +
> +		mutex_lock(&q->debugfs_mutex);
> +		blk_mq_debugfs_register_rq_qos(q);
> +		mutex_unlock(&q->debugfs_mutex);
> +	}

->debugfs_mutex only may be not enough, because blk_mq_debugfs_register_rq_qos()
has to traverse rq_qos single list list, you may have to grab q->rq_qos_mutex
for protect the list.


Thanks,
Ming

Hi,

在 2025/10/14 16:13, Ming Lei 写道:
> On Tue, Oct 14, 2025 at 10:21:48AM +0800, Yu Kuai wrote:
>> Currently rq-qos debugfs entries is created from rq_qos_add(), while
>> rq_qos_add() requires queue to be freezed. This can deadlock because
>> creating new entries can trigger fs reclaim.
>>
>> Fix this problem by delaying creating rq-qos debugfs entries until
>> it's initialization is complete.
>>
>> - For wbt, it can be initialized by default of by blk-sysfs, fix it by
>>    calling blk_mq_debugfs_register_rq_qos() after wbt_init;
>> - For other policies, they can only be initialized by blkg configuration,
>>    fix it by calling blk_mq_debugfs_register_rq_qos() from
>>    blkg_conf_end();
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   block/blk-cgroup.c | 6 ++++++
>>   block/blk-rq-qos.c | 7 -------
>>   block/blk-sysfs.c  | 4 ++++
>>   block/blk-wbt.c    | 7 ++++++-
>>   4 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index d93654334854..e4ccabf132c0 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -33,6 +33,7 @@
>>   #include "blk-cgroup.h"
>>   #include "blk-ioprio.h"
>>   #include "blk-throttle.h"
>> +#include "blk-mq-debugfs.h"
>>   
>>   static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu);
>>   
>> @@ -746,6 +747,11 @@ void blkg_conf_end(struct blkg_conf_ctx *ctx)
>>   	mutex_unlock(&q->elevator_lock);
>>   	blk_mq_unfreeze_queue(q, ctx->memflags);
>>   	blkdev_put_no_open(ctx->bdev);
>> +
>> +	mutex_lock(&q->debugfs_mutex);
>> +	blk_mq_debugfs_register_rq_qos(q);
>> +	mutex_unlock(&q->debugfs_mutex);
>> +
>>   }
>>   EXPORT_SYMBOL_GPL(blkg_conf_end);
>>   
>> diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
>> index 654478dfbc20..d7ce99ce2e80 100644
>> --- a/block/blk-rq-qos.c
>> +++ b/block/blk-rq-qos.c
>> @@ -347,13 +347,6 @@ int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
>>   	blk_queue_flag_set(QUEUE_FLAG_QOS_ENABLED, q);
>>   
>>   	blk_mq_unfreeze_queue(q, memflags);
>> -
>> -	if (rqos->ops->debugfs_attrs) {
>> -		mutex_lock(&q->debugfs_mutex);
>> -		blk_mq_debugfs_register_rqos(rqos);
>> -		mutex_unlock(&q->debugfs_mutex);
>> -	}
>> -
>>   	return 0;
>>   ebusy:
>>   	blk_mq_unfreeze_queue(q, memflags);
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index 76c47fe9b8d6..52bb4db25cf5 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -688,6 +688,10 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page,
>>   	mutex_unlock(&disk->rqos_state_mutex);
>>   
>>   	blk_mq_unquiesce_queue(q);
>> +
>> +	mutex_lock(&q->debugfs_mutex);
>> +	blk_mq_debugfs_register_rq_qos(q);
>> +	mutex_unlock(&q->debugfs_mutex);
>>   out:
>>   	blk_mq_unfreeze_queue(q, memflags);
>>   
>> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
>> index eb8037bae0bd..a120b5ba54db 100644
>> --- a/block/blk-wbt.c
>> +++ b/block/blk-wbt.c
>> @@ -724,8 +724,13 @@ void wbt_enable_default(struct gendisk *disk)
>>   	if (!blk_queue_registered(q))
>>   		return;
>>   
>> -	if (queue_is_mq(q) && enable)
>> +	if (queue_is_mq(q) && enable) {
>>   		wbt_init(disk);
>> +
>> +		mutex_lock(&q->debugfs_mutex);
>> +		blk_mq_debugfs_register_rq_qos(q);
>> +		mutex_unlock(&q->debugfs_mutex);
>> +	}
> 
> ->debugfs_mutex only may be not enough, because blk_mq_debugfs_register_rq_qos()
> has to traverse rq_qos single list list, you may have to grab q->rq_qos_mutex
> for protect the list.
> 

I think we can't grab rq_qos_mutex to create debugfs entries, right?
With the respect of this, perhaps we can grab debugfs_mutex to protect
insering rq_qos list instead?

Thanks,
Kuai

> 
> Thanks,
> Ming
> 
> 
> .
> 

On Tue, Oct 14, 2025 at 04:24:23PM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2025/10/14 16:13, Ming Lei 写道:
> > On Tue, Oct 14, 2025 at 10:21:48AM +0800, Yu Kuai wrote:
> > > Currently rq-qos debugfs entries is created from rq_qos_add(), while
> > > rq_qos_add() requires queue to be freezed. This can deadlock because
> > > creating new entries can trigger fs reclaim.
> > > 
> > > Fix this problem by delaying creating rq-qos debugfs entries until
> > > it's initialization is complete.
> > > 
> > > - For wbt, it can be initialized by default of by blk-sysfs, fix it by
> > >    calling blk_mq_debugfs_register_rq_qos() after wbt_init;
> > > - For other policies, they can only be initialized by blkg configuration,
> > >    fix it by calling blk_mq_debugfs_register_rq_qos() from
> > >    blkg_conf_end();
> > > 
> > > Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> > > ---
> > >   block/blk-cgroup.c | 6 ++++++
> > >   block/blk-rq-qos.c | 7 -------
> > >   block/blk-sysfs.c  | 4 ++++
> > >   block/blk-wbt.c    | 7 ++++++-
> > >   4 files changed, 16 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> > > index d93654334854..e4ccabf132c0 100644
> > > --- a/block/blk-cgroup.c
> > > +++ b/block/blk-cgroup.c
> > > @@ -33,6 +33,7 @@
> > >   #include "blk-cgroup.h"
> > >   #include "blk-ioprio.h"
> > >   #include "blk-throttle.h"
> > > +#include "blk-mq-debugfs.h"
> > >   static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu);
> > > @@ -746,6 +747,11 @@ void blkg_conf_end(struct blkg_conf_ctx *ctx)
> > >   	mutex_unlock(&q->elevator_lock);
> > >   	blk_mq_unfreeze_queue(q, ctx->memflags);
> > >   	blkdev_put_no_open(ctx->bdev);
> > > +
> > > +	mutex_lock(&q->debugfs_mutex);
> > > +	blk_mq_debugfs_register_rq_qos(q);
> > > +	mutex_unlock(&q->debugfs_mutex);
> > > +
> > >   }
> > >   EXPORT_SYMBOL_GPL(blkg_conf_end);
> > > diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
> > > index 654478dfbc20..d7ce99ce2e80 100644
> > > --- a/block/blk-rq-qos.c
> > > +++ b/block/blk-rq-qos.c
> > > @@ -347,13 +347,6 @@ int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
> > >   	blk_queue_flag_set(QUEUE_FLAG_QOS_ENABLED, q);
> > >   	blk_mq_unfreeze_queue(q, memflags);
> > > -
> > > -	if (rqos->ops->debugfs_attrs) {
> > > -		mutex_lock(&q->debugfs_mutex);
> > > -		blk_mq_debugfs_register_rqos(rqos);
> > > -		mutex_unlock(&q->debugfs_mutex);
> > > -	}
> > > -
> > >   	return 0;
> > >   ebusy:
> > >   	blk_mq_unfreeze_queue(q, memflags);
> > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > > index 76c47fe9b8d6..52bb4db25cf5 100644
> > > --- a/block/blk-sysfs.c
> > > +++ b/block/blk-sysfs.c
> > > @@ -688,6 +688,10 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page,
> > >   	mutex_unlock(&disk->rqos_state_mutex);
> > >   	blk_mq_unquiesce_queue(q);
> > > +
> > > +	mutex_lock(&q->debugfs_mutex);
> > > +	blk_mq_debugfs_register_rq_qos(q);
> > > +	mutex_unlock(&q->debugfs_mutex);
> > >   out:
> > >   	blk_mq_unfreeze_queue(q, memflags);
> > > diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> > > index eb8037bae0bd..a120b5ba54db 100644
> > > --- a/block/blk-wbt.c
> > > +++ b/block/blk-wbt.c
> > > @@ -724,8 +724,13 @@ void wbt_enable_default(struct gendisk *disk)
> > >   	if (!blk_queue_registered(q))
> > >   		return;
> > > -	if (queue_is_mq(q) && enable)
> > > +	if (queue_is_mq(q) && enable) {
> > >   		wbt_init(disk);
> > > +
> > > +		mutex_lock(&q->debugfs_mutex);
> > > +		blk_mq_debugfs_register_rq_qos(q);
> > > +		mutex_unlock(&q->debugfs_mutex);
> > > +	}
> > 
> > ->debugfs_mutex only may be not enough, because blk_mq_debugfs_register_rq_qos()
> > has to traverse rq_qos single list list, you may have to grab q->rq_qos_mutex
> > for protect the list.
> > 
> 
> I think we can't grab rq_qos_mutex to create debugfs entries, right?

It depends on the finalized order between rq_qos_mutex and freezing queue.

> With the respect of this, perhaps we can grab debugfs_mutex to protect
> insering rq_qos list instead?

No, debugfs_mutex shouldn't protect rq_qos list, and rq_qos_mutex is
supposed to do the job at least from naming viewpoint.


Thanks,
Ming

Hi,

在 2025/10/14 16:37, Ming Lei 写道:
> On Tue, Oct 14, 2025 at 04:24:23PM +0800, Yu Kuai wrote:
>> Hi,
>>
>> 在 2025/10/14 16:13, Ming Lei 写道:
>>> On Tue, Oct 14, 2025 at 10:21:48AM +0800, Yu Kuai wrote:
>>>> Currently rq-qos debugfs entries is created from rq_qos_add(), while
>>>> rq_qos_add() requires queue to be freezed. This can deadlock because
>>>> creating new entries can trigger fs reclaim.
>>>>
>>>> Fix this problem by delaying creating rq-qos debugfs entries until
>>>> it's initialization is complete.
>>>>
>>>> - For wbt, it can be initialized by default of by blk-sysfs, fix it by
>>>>     calling blk_mq_debugfs_register_rq_qos() after wbt_init;
>>>> - For other policies, they can only be initialized by blkg configuration,
>>>>     fix it by calling blk_mq_debugfs_register_rq_qos() from
>>>>     blkg_conf_end();
>>>>
>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>>> ---
>>>>    block/blk-cgroup.c | 6 ++++++
>>>>    block/blk-rq-qos.c | 7 -------
>>>>    block/blk-sysfs.c  | 4 ++++
>>>>    block/blk-wbt.c    | 7 ++++++-
>>>>    4 files changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>>>> index d93654334854..e4ccabf132c0 100644
>>>> --- a/block/blk-cgroup.c
>>>> +++ b/block/blk-cgroup.c
>>>> @@ -33,6 +33,7 @@
>>>>    #include "blk-cgroup.h"
>>>>    #include "blk-ioprio.h"
>>>>    #include "blk-throttle.h"
>>>> +#include "blk-mq-debugfs.h"
>>>>    static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu);
>>>> @@ -746,6 +747,11 @@ void blkg_conf_end(struct blkg_conf_ctx *ctx)
>>>>    	mutex_unlock(&q->elevator_lock);
>>>>    	blk_mq_unfreeze_queue(q, ctx->memflags);
>>>>    	blkdev_put_no_open(ctx->bdev);
>>>> +
>>>> +	mutex_lock(&q->debugfs_mutex);
>>>> +	blk_mq_debugfs_register_rq_qos(q);
>>>> +	mutex_unlock(&q->debugfs_mutex);
>>>> +
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(blkg_conf_end);
>>>> diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
>>>> index 654478dfbc20..d7ce99ce2e80 100644
>>>> --- a/block/blk-rq-qos.c
>>>> +++ b/block/blk-rq-qos.c
>>>> @@ -347,13 +347,6 @@ int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
>>>>    	blk_queue_flag_set(QUEUE_FLAG_QOS_ENABLED, q);
>>>>    	blk_mq_unfreeze_queue(q, memflags);
>>>> -
>>>> -	if (rqos->ops->debugfs_attrs) {
>>>> -		mutex_lock(&q->debugfs_mutex);
>>>> -		blk_mq_debugfs_register_rqos(rqos);
>>>> -		mutex_unlock(&q->debugfs_mutex);
>>>> -	}
>>>> -
>>>>    	return 0;
>>>>    ebusy:
>>>>    	blk_mq_unfreeze_queue(q, memflags);
>>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>>> index 76c47fe9b8d6..52bb4db25cf5 100644
>>>> --- a/block/blk-sysfs.c
>>>> +++ b/block/blk-sysfs.c
>>>> @@ -688,6 +688,10 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page,
>>>>    	mutex_unlock(&disk->rqos_state_mutex);
>>>>    	blk_mq_unquiesce_queue(q);
>>>> +
>>>> +	mutex_lock(&q->debugfs_mutex);
>>>> +	blk_mq_debugfs_register_rq_qos(q);
>>>> +	mutex_unlock(&q->debugfs_mutex);
>>>>    out:
>>>>    	blk_mq_unfreeze_queue(q, memflags);
>>>> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
>>>> index eb8037bae0bd..a120b5ba54db 100644
>>>> --- a/block/blk-wbt.c
>>>> +++ b/block/blk-wbt.c
>>>> @@ -724,8 +724,13 @@ void wbt_enable_default(struct gendisk *disk)
>>>>    	if (!blk_queue_registered(q))
>>>>    		return;
>>>> -	if (queue_is_mq(q) && enable)
>>>> +	if (queue_is_mq(q) && enable) {
>>>>    		wbt_init(disk);
>>>> +
>>>> +		mutex_lock(&q->debugfs_mutex);
>>>> +		blk_mq_debugfs_register_rq_qos(q);
>>>> +		mutex_unlock(&q->debugfs_mutex);
>>>> +	}
>>>
>>> ->debugfs_mutex only may be not enough, because blk_mq_debugfs_register_rq_qos()
>>> has to traverse rq_qos single list list, you may have to grab q->rq_qos_mutex
>>> for protect the list.
>>>
>>
>> I think we can't grab rq_qos_mutex to create debugfs entries, right?
> 
> It depends on the finalized order between rq_qos_mutex and freezing queue.
> 
>> With the respect of this, perhaps we can grab debugfs_mutex to protect
>> insering rq_qos list instead?
> 
> No, debugfs_mutex shouldn't protect rq_qos list, and rq_qos_mutex is
> supposed to do the job at least from naming viewpoint.

Ok, then we'll have to make sure the order is rq_qos_mutex before
freezing queue, I was thinking the inverse order because of the helper
blkg_conf_open_bdev_frozen().

I'll check first if this is possible.

Thanks,
Kuai
> 
> 
> Thanks,
> Ming
> 
> 
> .
> 

On Tue, Oct 14, 2025 at 04:42:30PM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2025/10/14 16:37, Ming Lei 写道:
> > On Tue, Oct 14, 2025 at 04:24:23PM +0800, Yu Kuai wrote:
> > > Hi,
> > > 
> > > 在 2025/10/14 16:13, Ming Lei 写道:
> > > > On Tue, Oct 14, 2025 at 10:21:48AM +0800, Yu Kuai wrote:
> > > > > Currently rq-qos debugfs entries is created from rq_qos_add(), while
> > > > > rq_qos_add() requires queue to be freezed. This can deadlock because
> > > > > creating new entries can trigger fs reclaim.
> > > > > 
> > > > > Fix this problem by delaying creating rq-qos debugfs entries until
> > > > > it's initialization is complete.
> > > > > 
> > > > > - For wbt, it can be initialized by default of by blk-sysfs, fix it by
> > > > >     calling blk_mq_debugfs_register_rq_qos() after wbt_init;
> > > > > - For other policies, they can only be initialized by blkg configuration,
> > > > >     fix it by calling blk_mq_debugfs_register_rq_qos() from
> > > > >     blkg_conf_end();
> > > > > 
> > > > > Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> > > > > ---
> > > > >    block/blk-cgroup.c | 6 ++++++
> > > > >    block/blk-rq-qos.c | 7 -------
> > > > >    block/blk-sysfs.c  | 4 ++++
> > > > >    block/blk-wbt.c    | 7 ++++++-
> > > > >    4 files changed, 16 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> > > > > index d93654334854..e4ccabf132c0 100644
> > > > > --- a/block/blk-cgroup.c
> > > > > +++ b/block/blk-cgroup.c
> > > > > @@ -33,6 +33,7 @@
> > > > >    #include "blk-cgroup.h"
> > > > >    #include "blk-ioprio.h"
> > > > >    #include "blk-throttle.h"
> > > > > +#include "blk-mq-debugfs.h"
> > > > >    static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu);
> > > > > @@ -746,6 +747,11 @@ void blkg_conf_end(struct blkg_conf_ctx *ctx)
> > > > >    	mutex_unlock(&q->elevator_lock);
> > > > >    	blk_mq_unfreeze_queue(q, ctx->memflags);
> > > > >    	blkdev_put_no_open(ctx->bdev);
> > > > > +
> > > > > +	mutex_lock(&q->debugfs_mutex);
> > > > > +	blk_mq_debugfs_register_rq_qos(q);
> > > > > +	mutex_unlock(&q->debugfs_mutex);
> > > > > +
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(blkg_conf_end);
> > > > > diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
> > > > > index 654478dfbc20..d7ce99ce2e80 100644
> > > > > --- a/block/blk-rq-qos.c
> > > > > +++ b/block/blk-rq-qos.c
> > > > > @@ -347,13 +347,6 @@ int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
> > > > >    	blk_queue_flag_set(QUEUE_FLAG_QOS_ENABLED, q);
> > > > >    	blk_mq_unfreeze_queue(q, memflags);
> > > > > -
> > > > > -	if (rqos->ops->debugfs_attrs) {
> > > > > -		mutex_lock(&q->debugfs_mutex);
> > > > > -		blk_mq_debugfs_register_rqos(rqos);
> > > > > -		mutex_unlock(&q->debugfs_mutex);
> > > > > -	}
> > > > > -
> > > > >    	return 0;
> > > > >    ebusy:
> > > > >    	blk_mq_unfreeze_queue(q, memflags);
> > > > > diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> > > > > index 76c47fe9b8d6..52bb4db25cf5 100644
> > > > > --- a/block/blk-sysfs.c
> > > > > +++ b/block/blk-sysfs.c
> > > > > @@ -688,6 +688,10 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page,
> > > > >    	mutex_unlock(&disk->rqos_state_mutex);
> > > > >    	blk_mq_unquiesce_queue(q);
> > > > > +
> > > > > +	mutex_lock(&q->debugfs_mutex);
> > > > > +	blk_mq_debugfs_register_rq_qos(q);
> > > > > +	mutex_unlock(&q->debugfs_mutex);
> > > > >    out:
> > > > >    	blk_mq_unfreeze_queue(q, memflags);
> > > > > diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> > > > > index eb8037bae0bd..a120b5ba54db 100644
> > > > > --- a/block/blk-wbt.c
> > > > > +++ b/block/blk-wbt.c
> > > > > @@ -724,8 +724,13 @@ void wbt_enable_default(struct gendisk *disk)
> > > > >    	if (!blk_queue_registered(q))
> > > > >    		return;
> > > > > -	if (queue_is_mq(q) && enable)
> > > > > +	if (queue_is_mq(q) && enable) {
> > > > >    		wbt_init(disk);
> > > > > +
> > > > > +		mutex_lock(&q->debugfs_mutex);
> > > > > +		blk_mq_debugfs_register_rq_qos(q);
> > > > > +		mutex_unlock(&q->debugfs_mutex);
> > > > > +	}
> > > > 
> > > > ->debugfs_mutex only may be not enough, because blk_mq_debugfs_register_rq_qos()
> > > > has to traverse rq_qos single list list, you may have to grab q->rq_qos_mutex
> > > > for protect the list.
> > > > 
> > > 
> > > I think we can't grab rq_qos_mutex to create debugfs entries, right?
> > 
> > It depends on the finalized order between rq_qos_mutex and freezing queue.
> > 
> > > With the respect of this, perhaps we can grab debugfs_mutex to protect
> > > insering rq_qos list instead?
> > 
> > No, debugfs_mutex shouldn't protect rq_qos list, and rq_qos_mutex is
> > supposed to do the job at least from naming viewpoint.
> 
> Ok, then we'll have to make sure the order is rq_qos_mutex before
> freezing queue, I was thinking the inverse order because of the helper
> blkg_conf_open_bdev_frozen().
> 
> I'll check first if this is possible.

You may misunderstand my point, I meant `debugfs_mutex` can't be used for
protecting rq_qos list because of its name. But order between rq_qos_mutex
and freeze queue might be fine in either way, just it has to be fixed.
Not look into it yet.

Thanks,
Ming

Hi,

在 2025/10/14 16:55, Ming Lei 写道:
> On Tue, Oct 14, 2025 at 04:42:30PM +0800, Yu Kuai wrote:
>> Hi,
>>
>> 在 2025/10/14 16:37, Ming Lei 写道:
>>> On Tue, Oct 14, 2025 at 04:24:23PM +0800, Yu Kuai wrote:
>>>> Hi,
>>>>
>>>> 在 2025/10/14 16:13, Ming Lei 写道:
>>>>> On Tue, Oct 14, 2025 at 10:21:48AM +0800, Yu Kuai wrote:
>>>>>> Currently rq-qos debugfs entries is created from rq_qos_add(), while
>>>>>> rq_qos_add() requires queue to be freezed. This can deadlock because
>>>>>> creating new entries can trigger fs reclaim.
>>>>>>
>>>>>> Fix this problem by delaying creating rq-qos debugfs entries until
>>>>>> it's initialization is complete.
>>>>>>
>>>>>> - For wbt, it can be initialized by default of by blk-sysfs, fix it by
>>>>>>      calling blk_mq_debugfs_register_rq_qos() after wbt_init;
>>>>>> - For other policies, they can only be initialized by blkg configuration,
>>>>>>      fix it by calling blk_mq_debugfs_register_rq_qos() from
>>>>>>      blkg_conf_end();
>>>>>>
>>>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>>>>> ---
>>>>>>     block/blk-cgroup.c | 6 ++++++
>>>>>>     block/blk-rq-qos.c | 7 -------
>>>>>>     block/blk-sysfs.c  | 4 ++++
>>>>>>     block/blk-wbt.c    | 7 ++++++-
>>>>>>     4 files changed, 16 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>>>>>> index d93654334854..e4ccabf132c0 100644
>>>>>> --- a/block/blk-cgroup.c
>>>>>> +++ b/block/blk-cgroup.c
>>>>>> @@ -33,6 +33,7 @@
>>>>>>     #include "blk-cgroup.h"
>>>>>>     #include "blk-ioprio.h"
>>>>>>     #include "blk-throttle.h"
>>>>>> +#include "blk-mq-debugfs.h"
>>>>>>     static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu);
>>>>>> @@ -746,6 +747,11 @@ void blkg_conf_end(struct blkg_conf_ctx *ctx)
>>>>>>     	mutex_unlock(&q->elevator_lock);
>>>>>>     	blk_mq_unfreeze_queue(q, ctx->memflags);
>>>>>>     	blkdev_put_no_open(ctx->bdev);
>>>>>> +
>>>>>> +	mutex_lock(&q->debugfs_mutex);
>>>>>> +	blk_mq_debugfs_register_rq_qos(q);
>>>>>> +	mutex_unlock(&q->debugfs_mutex);
>>>>>> +
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_GPL(blkg_conf_end);
>>>>>> diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
>>>>>> index 654478dfbc20..d7ce99ce2e80 100644
>>>>>> --- a/block/blk-rq-qos.c
>>>>>> +++ b/block/blk-rq-qos.c
>>>>>> @@ -347,13 +347,6 @@ int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id,
>>>>>>     	blk_queue_flag_set(QUEUE_FLAG_QOS_ENABLED, q);
>>>>>>     	blk_mq_unfreeze_queue(q, memflags);
>>>>>> -
>>>>>> -	if (rqos->ops->debugfs_attrs) {
>>>>>> -		mutex_lock(&q->debugfs_mutex);
>>>>>> -		blk_mq_debugfs_register_rqos(rqos);
>>>>>> -		mutex_unlock(&q->debugfs_mutex);
>>>>>> -	}
>>>>>> -
>>>>>>     	return 0;
>>>>>>     ebusy:
>>>>>>     	blk_mq_unfreeze_queue(q, memflags);
>>>>>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>>>>>> index 76c47fe9b8d6..52bb4db25cf5 100644
>>>>>> --- a/block/blk-sysfs.c
>>>>>> +++ b/block/blk-sysfs.c
>>>>>> @@ -688,6 +688,10 @@ static ssize_t queue_wb_lat_store(struct gendisk *disk, const char *page,
>>>>>>     	mutex_unlock(&disk->rqos_state_mutex);
>>>>>>     	blk_mq_unquiesce_queue(q);
>>>>>> +
>>>>>> +	mutex_lock(&q->debugfs_mutex);
>>>>>> +	blk_mq_debugfs_register_rq_qos(q);
>>>>>> +	mutex_unlock(&q->debugfs_mutex);
>>>>>>     out:
>>>>>>     	blk_mq_unfreeze_queue(q, memflags);
>>>>>> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
>>>>>> index eb8037bae0bd..a120b5ba54db 100644
>>>>>> --- a/block/blk-wbt.c
>>>>>> +++ b/block/blk-wbt.c
>>>>>> @@ -724,8 +724,13 @@ void wbt_enable_default(struct gendisk *disk)
>>>>>>     	if (!blk_queue_registered(q))
>>>>>>     		return;
>>>>>> -	if (queue_is_mq(q) && enable)
>>>>>> +	if (queue_is_mq(q) && enable) {
>>>>>>     		wbt_init(disk);
>>>>>> +
>>>>>> +		mutex_lock(&q->debugfs_mutex);
>>>>>> +		blk_mq_debugfs_register_rq_qos(q);
>>>>>> +		mutex_unlock(&q->debugfs_mutex);
>>>>>> +	}
>>>>>
>>>>> ->debugfs_mutex only may be not enough, because blk_mq_debugfs_register_rq_qos()
>>>>> has to traverse rq_qos single list list, you may have to grab q->rq_qos_mutex
>>>>> for protect the list.
>>>>>
>>>>
>>>> I think we can't grab rq_qos_mutex to create debugfs entries, right?
>>>
>>> It depends on the finalized order between rq_qos_mutex and freezing queue.
>>>
>>>> With the respect of this, perhaps we can grab debugfs_mutex to protect
>>>> insering rq_qos list instead?
>>>
>>> No, debugfs_mutex shouldn't protect rq_qos list, and rq_qos_mutex is
>>> supposed to do the job at least from naming viewpoint.
>>
>> Ok, then we'll have to make sure the order is rq_qos_mutex before
>> freezing queue, I was thinking the inverse order because of the helper
>> blkg_conf_open_bdev_frozen().
>>
>> I'll check first if this is possible.
> 
> You may misunderstand my point, I meant `debugfs_mutex` can't be used for
> protecting rq_qos list because of its name. But order between rq_qos_mutex
> and freeze queue might be fine in either way, just it has to be fixed.
> Not look into it yet.

No misunderstood :) I mean if we want to fix this by delaying creating
debugfs entries after queue is unfreezed, and we have to hold
rq_qos_mutex for ierating rqos, then rq_qos_mutex have to be hold before
freeing queue.

A quick look I feel it's ok, I'll try a new version.

Thanks,
Kuai

> 
> Thanks,
> Ming
> 
> 
> .
>