| Message ID | 20181214113926.7451-2-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: support debugfs on rq_qos & wbt | expand |
On Fri, 2018-12-14 at 19:39 +0800, Ming Lei wrote: > static void print_stat(struct seq_file *m, struct blk_rq_stat *stat) > { > @@ -856,6 +857,15 @@ int blk_mq_debugfs_register(struct request_queue *q) > goto err; > } > > + if (q->rq_qos) { > + struct rq_qos *rqos = q->rq_qos; > + > + while (rqos) { > + blk_mq_debugfs_register_rqos(rqos); > + rqos = rqos->next; > + } > + } Have you considered to use a for-loop instead of a while loop? That would allow to remove the if-statement and hence would make the code more compact. > +int blk_mq_debugfs_register_rqos(struct rq_qos *rqos) > +{ > + struct request_queue *q = rqos->q; > + char *dir_name = rq_qos_id_to_name(rqos->id); Please change "char *" into "const char *". > enum rq_qos_id { > RQ_QOS_WBT, > RQ_QOS_CGROUP, > @@ -22,6 +26,9 @@ struct rq_qos { > struct request_queue *q; > enum rq_qos_id id; > struct rq_qos *next; > +#ifdef CONFIG_BLK_DEBUG_FS > + struct dentry *debugfs_dir; > +#endif > }; The indentation of "debugfs_dir" looks odd to me. > +static inline char *rq_qos_id_to_name(enum rq_qos_id id) > +{ > + switch (id) { > + case RQ_QOS_WBT: > + return "wbt"; > + case RQ_QOS_CGROUP: > + return "cgroup"; > + } > + return "unknown"; > +} Same comment here as earlier: please use "const char *" instead of "char *" for constant strings. Otherwise this patch looks fine to me. Thanks, Bart.
On 12/14/18 12:11 PM, Bart Van Assche wrote: > On Fri, 2018-12-14 at 19:39 +0800, Ming Lei wrote: >> static void print_stat(struct seq_file *m, struct blk_rq_stat *stat) >> { >> @@ -856,6 +857,15 @@ int blk_mq_debugfs_register(struct request_queue *q) >> goto err; >> } >> >> + if (q->rq_qos) { >> + struct rq_qos *rqos = q->rq_qos; >> + >> + while (rqos) { >> + blk_mq_debugfs_register_rqos(rqos); >> + rqos = rqos->next; >> + } >> + } > > Have you considered to use a for-loop instead of a while loop? That would > allow to remove the if-statement and hence would make the code more compact. It would fit better with the style in that code do to: rqos = q->rq_qos; while (rqos) { .... rqos = rqos->next; } > >> +int blk_mq_debugfs_register_rqos(struct rq_qos *rqos) >> +{ >> + struct request_queue *q = rqos->q; >> + char *dir_name = rq_qos_id_to_name(rqos->id); > > Please change "char *" into "const char *". > >> enum rq_qos_id { >> RQ_QOS_WBT, >> RQ_QOS_CGROUP, >> @@ -22,6 +26,9 @@ struct rq_qos { >> struct request_queue *q; >> enum rq_qos_id id; >> struct rq_qos *next; >> +#ifdef CONFIG_BLK_DEBUG_FS >> + struct dentry *debugfs_dir; >> +#endif >> }; > > The indentation of "debugfs_dir" looks odd to me. > >> +static inline char *rq_qos_id_to_name(enum rq_qos_id id) >> +{ >> + switch (id) { >> + case RQ_QOS_WBT: >> + return "wbt"; >> + case RQ_QOS_CGROUP: >> + return "cgroup"; >> + } >> + return "unknown"; >> +} > > Same comment here as earlier: please use "const char *" instead of "char *" > for constant strings. Otherwise this patch looks fine to me. Agree on the rest of your comments.
On Sun, Dec 16, 2018 at 09:04:37AM -0700, Jens Axboe wrote: > It would fit better with the style in that code do to: > > rqos = q->rq_qos; > while (rqos) { > .... > rqos = rqos->next; > } But that is just a really strangly written for loop to start with.. Maybe Bart need to send a cleanup patch for the whole file, especially given that this patch has been applied already.
On 12/16/18 9:09 AM, Christoph Hellwig wrote: > On Sun, Dec 16, 2018 at 09:04:37AM -0700, Jens Axboe wrote: >> It would fit better with the style in that code do to: >> >> rqos = q->rq_qos; >> while (rqos) { >> .... >> rqos = rqos->next; >> } > > But that is just a really strangly written for loop to start with.. I think it looks fine, and it's more readable than using a for() loop for something like this. > Maybe Bart need to send a cleanup patch for the whole file, especially > given that this patch has been applied already. As mentioned in the same thread, I dropped these since I had to amend a commit anyway. So we're free to do whatever.
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c index a32bb79d6c95..bbcbc2ca0176 100644 --- a/block/blk-mq-debugfs.c +++ b/block/blk-mq-debugfs.c @@ -23,6 +23,7 @@ #include "blk-mq.h" #include "blk-mq-debugfs.h" #include "blk-mq-tag.h" +#include "blk-rq-qos.h" static void print_stat(struct seq_file *m, struct blk_rq_stat *stat) { @@ -856,6 +857,15 @@ int blk_mq_debugfs_register(struct request_queue *q) goto err; } + if (q->rq_qos) { + struct rq_qos *rqos = q->rq_qos; + + while (rqos) { + blk_mq_debugfs_register_rqos(rqos); + rqos = rqos->next; + } + } + return 0; err: @@ -978,6 +988,50 @@ void blk_mq_debugfs_unregister_sched(struct request_queue *q) q->sched_debugfs_dir = NULL; } +void blk_mq_debugfs_unregister_rqos(struct rq_qos *rqos) +{ + debugfs_remove_recursive(rqos->debugfs_dir); + rqos->debugfs_dir = NULL; +} + +int blk_mq_debugfs_register_rqos(struct rq_qos *rqos) +{ + struct request_queue *q = rqos->q; + char *dir_name = rq_qos_id_to_name(rqos->id); + + if (!q->debugfs_dir) + return -ENOENT; + + if (rqos->debugfs_dir || !rqos->ops->debugfs_attrs) + return 0; + + if (!q->rqos_debugfs_dir) { + q->rqos_debugfs_dir = debugfs_create_dir("rqos", + q->debugfs_dir); + if (!q->rqos_debugfs_dir) + return -ENOMEM; + } + + rqos->debugfs_dir = debugfs_create_dir(dir_name, + rqos->q->rqos_debugfs_dir); + if (!rqos->debugfs_dir) + return -ENOMEM; + + if (!debugfs_create_files(rqos->debugfs_dir, rqos, + rqos->ops->debugfs_attrs)) + goto err; + return 0; + err: + blk_mq_debugfs_unregister_rqos(rqos); + return -ENOMEM; +} + +void blk_mq_debugfs_unregister_queue_rqos(struct request_queue *q) +{ + debugfs_remove_recursive(q->rqos_debugfs_dir); + q->rqos_debugfs_dir = NULL; +} + int 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 a9160be12be0..8c9012a578c1 100644 --- a/block/blk-mq-debugfs.h +++ b/block/blk-mq-debugfs.h @@ -31,6 +31,10 @@ void blk_mq_debugfs_unregister_sched(struct request_queue *q); int blk_mq_debugfs_register_sched_hctx(struct request_queue *q, struct blk_mq_hw_ctx *hctx); void blk_mq_debugfs_unregister_sched_hctx(struct blk_mq_hw_ctx *hctx); + +int 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 int blk_mq_debugfs_register(struct request_queue *q) { @@ -78,6 +82,19 @@ static inline int blk_mq_debugfs_register_sched_hctx(struct request_queue *q, static inline void blk_mq_debugfs_unregister_sched_hctx(struct blk_mq_hw_ctx *hctx) { } + +static inline int blk_mq_debugfs_register_rqos(struct rq_qos *rqos) +{ + return 0; +} + +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 e932ef9d2718..d169d7188fa6 100644 --- a/block/blk-rq-qos.c +++ b/block/blk-rq-qos.c @@ -264,6 +264,8 @@ 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-rq-qos.h b/block/blk-rq-qos.h index 8678875de420..fd8a0c5debd3 100644 --- a/block/blk-rq-qos.h +++ b/block/blk-rq-qos.h @@ -7,6 +7,10 @@ #include <linux/atomic.h> #include <linux/wait.h> +#include "blk-mq-debugfs.h" + +struct blk_mq_debugfs_attr; + enum rq_qos_id { RQ_QOS_WBT, RQ_QOS_CGROUP, @@ -22,6 +26,9 @@ struct rq_qos { struct request_queue *q; enum rq_qos_id id; struct rq_qos *next; +#ifdef CONFIG_BLK_DEBUG_FS + struct dentry *debugfs_dir; +#endif }; struct rq_qos_ops { @@ -33,6 +40,9 @@ struct rq_qos_ops { void (*done_bio)(struct rq_qos *, struct bio *); void (*cleanup)(struct rq_qos *, struct bio *); void (*exit)(struct rq_qos *); +#ifdef CONFIG_BLK_DEBUG_FS + const struct blk_mq_debugfs_attr *debugfs_attrs; +#endif }; struct rq_depth { @@ -66,6 +76,17 @@ static inline struct rq_qos *blkcg_rq_qos(struct request_queue *q) return rq_qos_id(q, RQ_QOS_CGROUP); } +static inline char *rq_qos_id_to_name(enum rq_qos_id id) +{ + switch (id) { + case RQ_QOS_WBT: + return "wbt"; + case RQ_QOS_CGROUP: + return "cgroup"; + } + return "unknown"; +} + static inline void rq_wait_init(struct rq_wait *rq_wait) { atomic_set(&rq_wait->inflight, 0); @@ -76,6 +97,9 @@ static inline void rq_qos_add(struct request_queue *q, struct rq_qos *rqos) { rqos->next = q->rq_qos; q->rq_qos = rqos; + + if (rqos->ops->debugfs_attrs) + blk_mq_debugfs_register_rqos(rqos); } static inline void rq_qos_del(struct request_queue *q, struct rq_qos *rqos) @@ -91,6 +115,8 @@ static inline void rq_qos_del(struct request_queue *q, struct rq_qos *rqos) } prev = cur; } + + blk_mq_debugfs_unregister_rqos(rqos); } typedef bool (acquire_inflight_cb_t)(struct rq_wait *rqw, void *private_data); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 81f1b105946b..45552e6eae1e 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -560,6 +560,7 @@ struct request_queue { #ifdef CONFIG_BLK_DEBUG_FS struct dentry *debugfs_dir; struct dentry *sched_debugfs_dir; + struct dentry *rqos_debugfs_dir; #endif bool mq_sysfs_init_done;
blk-mq-debugfs has been proved as very helpful for debug some tough issues, such as IO hang. We have seen blk-wbt related IO hang several times, even inside Red Hat BZ, there is such report not sovled yet, so this patch adds support debugfs on rq_qos. Cc: Bart Van Assche <bart.vanassche@wdc.com> Cc: Omar Sandoval <osandov@fb.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Josef Bacik <josef@toxicpanda.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/blk-mq-debugfs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ block/blk-mq-debugfs.h | 17 ++++++++++++++++ block/blk-rq-qos.c | 2 ++ block/blk-rq-qos.h | 26 ++++++++++++++++++++++++ include/linux/blkdev.h | 1 + 5 files changed, 100 insertions(+)