diff mbox series

[1/7] block: remove hctx->debugfs_dir

Message ID 20250209122035.1327325-2-ming.lei@redhat.com (mailing list archive)
State New
Headers show
Series block: remove all debugfs dir & don't grab debugfs_mutex | expand

Commit Message

Ming Lei Feb. 9, 2025, 12:20 p.m. UTC 
For each hctx, its debugfs path is fixed, which can be queried from
its parent dentry and hctx queue num, so it isn't necessary to cache
it in hctx structure because it isn't used in fast path.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-debugfs.c | 47 ++++++++++++++++++++++++++++++++----------
 include/linux/blk-mq.h |  5 -----
 2 files changed, 36 insertions(+), 16 deletions(-)

Comments

Christoph Hellwig Feb. 13, 2025, 6:41 a.m. UTC | #1 
On Sun, Feb 09, 2025 at 08:20:25PM +0800, Ming Lei wrote:
> For each hctx, its debugfs path is fixed, which can be queried from
> its parent dentry and hctx queue num, so it isn't necessary to cache
> it in hctx structure because it isn't used in fast path.

It's not needed, but it's also kinda convenient.  What is the argument
that speaks for recalculating the path?
Ming Lei Feb. 14, 2025, 2:07 a.m. UTC | #2 
On Thu, Feb 13, 2025 at 07:41:46AM +0100, Christoph Hellwig wrote:
> On Sun, Feb 09, 2025 at 08:20:25PM +0800, Ming Lei wrote:
> > For each hctx, its debugfs path is fixed, which can be queried from
> > its parent dentry and hctx queue num, so it isn't necessary to cache
> > it in hctx structure because it isn't used in fast path.
> 
> It's not needed, but it's also kinda convenient.  What is the argument
> that speaks for recalculating the path?

q->debugfs_lock will be not necessary if these cached entries are removed,
please see the last patch.


Thanks,
Ming
Christoph Hellwig Feb. 17, 2025, 9:12 a.m. UTC | #3 
On Fri, Feb 14, 2025 at 10:07:23AM +0800, Ming Lei wrote:
> On Thu, Feb 13, 2025 at 07:41:46AM +0100, Christoph Hellwig wrote:
> > On Sun, Feb 09, 2025 at 08:20:25PM +0800, Ming Lei wrote:
> > > For each hctx, its debugfs path is fixed, which can be queried from
> > > its parent dentry and hctx queue num, so it isn't necessary to cache
> > > it in hctx structure because it isn't used in fast path.
> > 
> > It's not needed, but it's also kinda convenient.  What is the argument
> > that speaks for recalculating the path?
> 
> q->debugfs_lock will be not necessary if these cached entries are removed,
> please see the last patch.

Please document the reasoning (and tradeoff if there are any) in the
commit log.  No one will see your cover letter when bisecting a bug
down to this commit in the future.
diff mbox series

Patch

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index adf5f0697b6b..16260bba4d11 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -633,8 +633,7 @@  void blk_mq_debugfs_register(struct request_queue *q)
 
 	/* Similarly, blk_mq_init_hctx() couldn't do this previously. */
 	queue_for_each_hw_ctx(q, hctx, i) {
-		if (!hctx->debugfs_dir)
-			blk_mq_debugfs_register_hctx(q, hctx);
+		blk_mq_debugfs_register_hctx(q, hctx);
 		if (q->elevator && !hctx->sched_debugfs_dir)
 			blk_mq_debugfs_register_sched_hctx(q, hctx);
 	}
@@ -649,14 +648,28 @@  void blk_mq_debugfs_register(struct request_queue *q)
 	}
 }
 
+static __must_check struct dentry *blk_mq_get_hctx_entry(
+		struct blk_mq_hw_ctx *hctx)
+{
+	char name[20];
+
+	snprintf(name, sizeof(name), "hctx%u", hctx->queue_num);
+	return debugfs_lookup(name, hctx->queue->debugfs_dir);
+}
+
 static void blk_mq_debugfs_register_ctx(struct blk_mq_hw_ctx *hctx,
 					struct blk_mq_ctx *ctx)
 {
+	struct dentry *hctx_dir = blk_mq_get_hctx_entry(hctx);
 	struct dentry *ctx_dir;
 	char name[20];
 
+	if (IS_ERR_OR_NULL(hctx_dir))
+		return;
+
 	snprintf(name, sizeof(name), "cpu%u", ctx->cpu);
-	ctx_dir = debugfs_create_dir(name, hctx->debugfs_dir);
+	ctx_dir = debugfs_create_dir(name, hctx_dir);
+	dput(hctx_dir);
 
 	debugfs_create_files(ctx_dir, ctx, blk_mq_debugfs_ctx_attrs);
 }
@@ -664,6 +677,7 @@  static void blk_mq_debugfs_register_ctx(struct blk_mq_hw_ctx *hctx,
 void blk_mq_debugfs_register_hctx(struct request_queue *q,
 				  struct blk_mq_hw_ctx *hctx)
 {
+	struct dentry *hctx_dir;
 	struct blk_mq_ctx *ctx;
 	char name[20];
 	int i;
@@ -672,9 +686,11 @@  void blk_mq_debugfs_register_hctx(struct request_queue *q,
 		return;
 
 	snprintf(name, sizeof(name), "hctx%u", hctx->queue_num);
-	hctx->debugfs_dir = debugfs_create_dir(name, q->debugfs_dir);
+	hctx_dir = debugfs_create_dir(name, q->debugfs_dir);
+	if (IS_ERR_OR_NULL(hctx_dir))
+		return;
 
-	debugfs_create_files(hctx->debugfs_dir, hctx, blk_mq_debugfs_hctx_attrs);
+	debugfs_create_files(hctx_dir, hctx, blk_mq_debugfs_hctx_attrs);
 
 	hctx_for_each_ctx(hctx, ctx, i)
 		blk_mq_debugfs_register_ctx(hctx, ctx);
@@ -682,11 +698,18 @@  void blk_mq_debugfs_register_hctx(struct request_queue *q,
 
 void blk_mq_debugfs_unregister_hctx(struct blk_mq_hw_ctx *hctx)
 {
+	struct dentry *hctx_dir;
+
 	if (!hctx->queue->debugfs_dir)
 		return;
-	debugfs_remove_recursive(hctx->debugfs_dir);
+
+	hctx_dir = blk_mq_get_hctx_entry(hctx);
+	if (IS_ERR_OR_NULL(hctx_dir))
+		return;
+
+	debugfs_remove_recursive(hctx_dir);
 	hctx->sched_debugfs_dir = NULL;
-	hctx->debugfs_dir = NULL;
+	dput(hctx_dir);
 }
 
 void blk_mq_debugfs_register_hctxs(struct request_queue *q)
@@ -780,6 +803,7 @@  void blk_mq_debugfs_register_rqos(struct rq_qos *rqos)
 void blk_mq_debugfs_register_sched_hctx(struct request_queue *q,
 					struct blk_mq_hw_ctx *hctx)
 {
+	struct dentry *hctx_dir = blk_mq_get_hctx_entry(hctx);
 	struct elevator_type *e = q->elevator->type;
 
 	lockdep_assert_held(&q->debugfs_mutex);
@@ -789,16 +813,17 @@  void blk_mq_debugfs_register_sched_hctx(struct request_queue *q,
 	 * We will be called again later on with appropriate parent debugfs
 	 * directory from blk_register_queue()
 	 */
-	if (!hctx->debugfs_dir)
+	if (IS_ERR_OR_NULL(hctx_dir))
 		return;
 
 	if (!e->hctx_debugfs_attrs)
-		return;
+		goto exit;
 
-	hctx->sched_debugfs_dir = debugfs_create_dir("sched",
-						     hctx->debugfs_dir);
+	hctx->sched_debugfs_dir = debugfs_create_dir("sched", hctx_dir);
 	debugfs_create_files(hctx->sched_debugfs_dir, hctx,
 			     e->hctx_debugfs_attrs);
+exit:
+	dput(hctx_dir);
 }
 
 void blk_mq_debugfs_unregister_sched_hctx(struct blk_mq_hw_ctx *hctx)
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 9ebb53f031cd..8c8682491403 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -427,11 +427,6 @@  struct blk_mq_hw_ctx {
 	struct kobject		kobj;
 
 #ifdef CONFIG_BLK_DEBUG_FS
-	/**
-	 * @debugfs_dir: debugfs directory for this hardware queue. Named
-	 * as cpu<cpu_number>.
-	 */
-	struct dentry		*debugfs_dir;
 	/** @sched_debugfs_dir:	debugfs directory for the scheduler. */
 	struct dentry		*sched_debugfs_dir;
 #endif