diff mbox series

[v4,1/7] block: Introduce blk_mq_debugfs_init()

Message ID 20230130212656.876311-2-bvanassche@acm.org
State New
Headers show
Series Add support for limits below the page size | expand

Commit Message

Bart Van Assche Jan. 30, 2023, 9:26 p.m. UTC
Move the code for creating the block layer debugfs root directory into
blk-mq-debugfs.c. This patch prepares for adding more debugfs
initialization code by introducing the function blk_mq_debugfs_init().

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Keith Busch <kbusch@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-core.c       | 3 ++-
 block/blk-mq-debugfs.c | 5 +++++
 block/blk-mq-debugfs.h | 6 ++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

Comments

Luis Chamberlain Feb. 1, 2023, 8:58 p.m. UTC | #1
On Mon, Jan 30, 2023 at 01:26:50PM -0800, Bart Van Assche wrote:
> Move the code for creating the block layer debugfs root directory into
> blk-mq-debugfs.c. This patch prepares for adding more debugfs
> initialization code by introducing the function blk_mq_debugfs_init().
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Keith Busch <kbusch@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis
Luis Chamberlain Feb. 1, 2023, 9:23 p.m. UTC | #2
On Wed, Feb 01, 2023 at 12:58:00PM -0800, Luis Chamberlain wrote:
> On Mon, Jan 30, 2023 at 01:26:50PM -0800, Bart Van Assche wrote:
> > Move the code for creating the block layer debugfs root directory into
> > blk-mq-debugfs.c. This patch prepares for adding more debugfs
> > initialization code by introducing the function blk_mq_debugfs_init().
> > 
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Ming Lei <ming.lei@redhat.com>
> > Cc: Keith Busch <kbusch@kernel.org>
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> 
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

Sorry but actually a neuron triggered after this to remind me of commit
85e0cbbb8a  ("block: create the request_queue debugfs_dir on
registration") and so using the terminology on that commit, wouldn't
this not create now the root block debugfs dir for request-based block
drivers?

  Luis
Bart Van Assche Feb. 1, 2023, 10:01 p.m. UTC | #3
On 2/1/23 13:23, Luis Chamberlain wrote:
> On Wed, Feb 01, 2023 at 12:58:00PM -0800, Luis Chamberlain wrote:
>> On Mon, Jan 30, 2023 at 01:26:50PM -0800, Bart Van Assche wrote:
>>> Move the code for creating the block layer debugfs root directory into
>>> blk-mq-debugfs.c. This patch prepares for adding more debugfs
>>> initialization code by introducing the function blk_mq_debugfs_init().
>>>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Ming Lei <ming.lei@redhat.com>
>>> Cc: Keith Busch <kbusch@kernel.org>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>
>> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
> 
> Sorry but actually a neuron triggered after this to remind me of commit
> 85e0cbbb8a  ("block: create the request_queue debugfs_dir on
> registration") and so using the terminology on that commit, wouldn't
> this not create now the root block debugfs dir for request-based block
> drivers?

Hi Luis,

This patch should not change any behavior with CONFIG_DEBUG_FS=y.

As one can see in include/linux/debugfs.h, debugfs_create_dir() does not 
create a directory with CONFIG_DEBUG_FS=n:

static inline struct dentry *debugfs_create_dir(const char *name,
						struct dentry *parent)
{
	return ERR_PTR(-ENODEV);
}

I think the only behavior change introduced by this patch is that 
blk_debugfs_root remains NULL with CONFIG_DEBUG_FS=n instead of being 
set to ERR_PTR(-ENODEV).

Thanks,

Bart.
Luis Chamberlain Feb. 1, 2023, 11:59 p.m. UTC | #4
On Wed, Feb 01, 2023 at 02:01:18PM -0800, Bart Van Assche wrote:
> On 2/1/23 13:23, Luis Chamberlain wrote:
> > On Wed, Feb 01, 2023 at 12:58:00PM -0800, Luis Chamberlain wrote:
> > > On Mon, Jan 30, 2023 at 01:26:50PM -0800, Bart Van Assche wrote:
> > > > Move the code for creating the block layer debugfs root directory into
> > > > blk-mq-debugfs.c. This patch prepares for adding more debugfs
> > > > initialization code by introducing the function blk_mq_debugfs_init().
> > > > 
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Ming Lei <ming.lei@redhat.com>
> > > > Cc: Keith Busch <kbusch@kernel.org>
> > > > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > > 
> > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
> > 
> > Sorry but actually a neuron triggered after this to remind me of commit
> > 85e0cbbb8a  ("block: create the request_queue debugfs_dir on
> > registration") and so using the terminology on that commit, wouldn't
> > this not create now the root block debugfs dir for request-based block
> > drivers?
> 
> Hi Luis,
> 
> This patch should not change any behavior with CONFIG_DEBUG_FS=y.

Ignore CONFIG_DEBUG_FS=y. This is about blktrace which needs it for
both types of drivers.

> As one can see in include/linux/debugfs.h, debugfs_create_dir() does not
> create a directory with CONFIG_DEBUG_FS=n:
> 
> static inline struct dentry *debugfs_create_dir(const char *name,
> 						struct dentry *parent)
> {
> 	return ERR_PTR(-ENODEV);
> }
> 
> I think the only behavior change introduced by this patch is that
> blk_debugfs_root remains NULL with CONFIG_DEBUG_FS=n instead of being set to
> ERR_PTR(-ENODEV).

My point was commit 85e0cbbb8a made blk_debugfs_root non-null always now
when debugfs is enabled for both request-based block drivers and for
make_request block drivers (multiqueue). My reading is that with your
patch blk_debugfs_root will not be created for request-based block
drivers.

  Luis
Bart Van Assche Feb. 2, 2023, 1:06 a.m. UTC | #5
On 2/1/23 15:59, Luis Chamberlain wrote:
> My point was commit 85e0cbbb8a made blk_debugfs_root non-null always now
> when debugfs is enabled for both request-based block drivers and for
> make_request block drivers (multiqueue). My reading is that with your
> patch blk_debugfs_root will not be created for request-based block
> drivers.

Hi Luis,

The empty version of blk_mq_debugfs_init() in my patch is only selected 
if  CONFIG_BLK_DEBUG_FS=n. I think that my patch preserves the behavior 
that /sys/kernel/debug/block/ is created independent of the type of 
request queues that is created. Am I perhaps misunderstanding your feedback?

Thanks,

Bart.
Luis Chamberlain Feb. 6, 2023, 4:03 p.m. UTC | #6
On Wed, Feb 01, 2023 at 05:06:22PM -0800, Bart Van Assche wrote:
> On 2/1/23 15:59, Luis Chamberlain wrote:
> > My point was commit 85e0cbbb8a made blk_debugfs_root non-null always now
> > when debugfs is enabled for both request-based block drivers and for
> > make_request block drivers (multiqueue). My reading is that with your
> > patch blk_debugfs_root will not be created for request-based block
> > drivers.
> 
> Hi Luis,
> 
> The empty version of blk_mq_debugfs_init() in my patch is only selected if
> CONFIG_BLK_DEBUG_FS=n. I think that my patch preserves the behavior that
> /sys/kernel/debug/block/ is created independent of the type of request
> queues that is created. Am I perhaps misunderstanding your feedback?

Yes, my point is prior to this patch the debugfs directory is created
even if you have CONFIG_BLK_DEBUG_FS=n and I've mentioned the commit
which describes why, but the skinnys is blktrace.

  Luis
Bart Van Assche Feb. 6, 2023, 9:11 p.m. UTC | #7
On 2/6/23 08:03, Luis Chamberlain wrote:
> Yes, my point is prior to this patch the debugfs directory is created
> even if you have CONFIG_BLK_DEBUG_FS=n and I've mentioned the commit
> which describes why, but the skinnys is blktrace.

I overlooked that the blk-mq-debugfs code is guarded by 
CONFIG_BLK_DEBUG_FS instead of CONFIG_DEBUG_FS. I will fix the issue 
that you reported.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index ccf9a7683a3c..0dacc2df9588 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -45,6 +45,7 @@ 
 #include <trace/events/block.h>
 
 #include "blk.h"
+#include "blk-mq-debugfs.h"
 #include "blk-mq-sched.h"
 #include "blk-pm.h"
 #include "blk-cgroup.h"
@@ -1202,7 +1203,7 @@  int __init blk_dev_init(void)
 	blk_requestq_cachep = kmem_cache_create("request_queue",
 			sizeof(struct request_queue), 0, SLAB_PANIC, NULL);
 
-	blk_debugfs_root = debugfs_create_dir("block", NULL);
+	blk_mq_debugfs_init();
 
 	return 0;
 }
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index bd942341b638..60d1de0ce624 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -874,3 +874,8 @@  void blk_mq_debugfs_unregister_sched_hctx(struct blk_mq_hw_ctx *hctx)
 	debugfs_remove_recursive(hctx->sched_debugfs_dir);
 	hctx->sched_debugfs_dir = NULL;
 }
+
+void blk_mq_debugfs_init(void)
+{
+	blk_debugfs_root = debugfs_create_dir("block", NULL);
+}
diff --git a/block/blk-mq-debugfs.h b/block/blk-mq-debugfs.h
index 9c7d4b6117d4..7942119051f5 100644
--- a/block/blk-mq-debugfs.h
+++ b/block/blk-mq-debugfs.h
@@ -17,6 +17,8 @@  struct blk_mq_debugfs_attr {
 	const struct seq_operations *seq_ops;
 };
 
+void blk_mq_debugfs_init(void);
+
 int __blk_mq_debugfs_rq_show(struct seq_file *m, struct request *rq);
 int blk_mq_debugfs_rq_show(struct seq_file *m, void *v);
 
@@ -36,6 +38,10 @@  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);
 #else
+static inline void blk_mq_debugfs_init(void)
+{
+}
+
 static inline void blk_mq_debugfs_register(struct request_queue *q)
 {
 }