Re: [PATCH 1/2] blk-mq-debugfs: support rq_qos

[Bart Van Assche <bvanassche@acm.org>]

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.