Re: [PATCH] block: remove per-disk debugfs files in blk_unregister_queue

[Ming Lei <ming.lei@redhat.com>]

On Tue, May 24, 2022 at 10:33:25AM +0200, Christoph Hellwig wrote:
> The block debugfs files are created in blk_register_queue, which is
> called by add_disk and use a naming scheme based on the disk_name.
> After del_gendisk returns that name can be reused and thus we must not
> leave these debugfs files around, otherwise the kernel is unhappy
> and spews messages like:
> 
> 	Directory XXXXX with parent 'block' already present!
> 
> and the newly created devices will not have working debugfs files.
> 
> Move the unregistration to blk_unregister_queue instead (which matches
> the sysfs unregistration) to make sure the debugfs life time rules match
> those of the disk name.
> 
> As part of the move also make sure the whole debugfs unregistration is
> inside a single debugfs_mutex critical section.
> 
> Note that this breaks blktests block/002, which checks that the debugfs
> directory has not been removed while blktests is running, but that
> particular check should simply be removed from the test case.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-mq-debugfs.c  |  8 ++------
>  block/blk-mq-debugfs.h  |  5 -----
>  block/blk-rq-qos.c      |  2 --
>  block/blk-sysfs.c       | 15 +++++++--------
>  kernel/trace/blktrace.c |  3 ---
>  5 files changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index 7e4136a60e1cc..f7eaa5405e4b5 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -713,6 +713,8 @@ void blk_mq_debugfs_register(struct request_queue *q)
>  
>  void blk_mq_debugfs_unregister(struct request_queue *q)
>  {
> +	/* all entries were removed by the caller */
> +	q->rqos_debugfs_dir = NULL;
>  	q->sched_debugfs_dir = NULL;
>  }
>  
> @@ -833,12 +835,6 @@ void blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
>  	debugfs_create_files(rqos->debugfs_dir, rqos, rqos->ops->debugfs_attrs);
>  }
>  
> -void blk_mq_debugfs_unregister_queue_rqos(struct request_queue *q)
> -{
> -	debugfs_remove_recursive(q->rqos_debugfs_dir);
> -	q->rqos_debugfs_dir = NULL;
> -}
> -
>  void blk_mq_debugfs_register_sched_hctx(struct request_queue *q,
>  					struct blk_mq_hw_ctx *hctx)
>  {
> diff --git a/block/blk-mq-debugfs.h b/block/blk-mq-debugfs.h
> index 69918f4170d69..401e9e9da640b 100644
> --- a/block/blk-mq-debugfs.h
> +++ b/block/blk-mq-debugfs.h
> @@ -36,7 +36,6 @@ void blk_mq_debugfs_unregister_sched_hctx(struct blk_mq_hw_ctx *hctx);
>  
>  void blk_mq_debugfs_register_rqos(struct rq_qos *rqos);
>  void blk_mq_debugfs_unregister_rqos(struct rq_qos *rqos);
> -void blk_mq_debugfs_unregister_queue_rqos(struct request_queue *q);
>  #else
>  static inline void blk_mq_debugfs_register(struct request_queue *q)
>  {
> @@ -87,10 +86,6 @@ static inline void blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
>  static inline void blk_mq_debugfs_unregister_rqos(struct rq_qos *rqos)
>  {
>  }
> -
> -static inline void blk_mq_debugfs_unregister_queue_rqos(struct request_queue *q)
> -{
> -}
>  #endif
>  
>  #ifdef CONFIG_BLK_DEBUG_FS_ZONED
> diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
> index e83af7bc75919..d3a75693adbf4 100644
> --- a/block/blk-rq-qos.c
> +++ b/block/blk-rq-qos.c
> @@ -294,8 +294,6 @@ void rq_qos_wait(struct rq_wait *rqw, void *private_data,
>  
>  void rq_qos_exit(struct request_queue *q)
>  {
> -	blk_mq_debugfs_unregister_queue_rqos(q);
> -
>  	while (q->rq_qos) {
>  		struct rq_qos *rqos = q->rq_qos;
>  		q->rq_qos = rqos->next;
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 88bd41d4cb593..c02fec8e9a3f6 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -779,14 +779,6 @@ static void blk_release_queue(struct kobject *kobj)
>  	if (queue_is_mq(q))
>  		blk_mq_release(q);
>  
> -	blk_trace_shutdown(q);
> -	mutex_lock(&q->debugfs_mutex);
> -	debugfs_remove_recursive(q->debugfs_dir);
> -	mutex_unlock(&q->debugfs_mutex);
> -
> -	if (queue_is_mq(q))
> -		blk_mq_debugfs_unregister(q);
> -
>  	bioset_exit(&q->bio_split);
>  
>  	if (blk_queue_has_srcu(q))
> @@ -951,5 +943,12 @@ void blk_unregister_queue(struct gendisk *disk)
>  
>  	mutex_unlock(&q->sysfs_dir_lock);
>  
> +	mutex_lock(&q->debugfs_mutex);
> +	blk_trace_shutdown(q);
> +	debugfs_remove_recursive(q->debugfs_dir);

The above line code may cause kernel panic in the following code paths:

1) blk_mq_debugfs_unregister_hctxs() called from __blk_mq_update_nr_hw_queues()

2) blk_mq_debugfs_unregister_sched_hctx() called from blk_mq_exit_sched()


Thanks, 
Ming