Re: [PATCH v4] block: Add ioprio to block_rq tracepoint

[Bart Van Assche <bvanassche@xxxxxxx>]

On 6/11/24 12:35 AM, Dongliang Cui wrote:
> +#define IOPRIO_CLASS_STRINGS \
> +	{ IOPRIO_CLASS_NONE,	"none" }, \
> +	{ IOPRIO_CLASS_RT,	"rt" }, \
> +	{ IOPRIO_CLASS_BE,	"be" }, \
> +	{ IOPRIO_CLASS_IDLE,	"idle" }, \
> +	{ IOPRIO_CLASS_INVALID,	"invalid"}

Shouldn't this array be defined in a C file instead of in a header file?

> @@ -79,27 +87,32 @@ TRACE_EVENT(block_rq_requeue,
>   	TP_ARGS(rq),
>   
>   	TP_STRUCT__entry(
> -		__field(  dev_t,	dev			)
> -		__field(  sector_t,	sector			)
> -		__field(  unsigned int,	nr_sector		)
> -		__array(  char,		rwbs,	RWBS_LEN	)
> -		__dynamic_array( char,	cmd,	1		)
> +		__field(  dev_t,		dev			)
> +		__field(  sector_t,		sector			)
> +		__field(  unsigned int,		nr_sector		)
> +		__field(  unsigned short,	ioprio			)
> +		__array(  char,			rwbs,	RWBS_LEN	)
> +		__dynamic_array( char,		cmd,	1		)
>   	),

I see unnecessary whitespace changes. These changes make this patch harder to
read than necessary. Please undo the whitespace changes.

>   DECLARE_EVENT_CLASS(block_rq_completion,
> @@ -109,12 +122,13 @@ DECLARE_EVENT_CLASS(block_rq_completion,
>   	TP_ARGS(rq, error, nr_bytes),
>   
>   	TP_STRUCT__entry(
> -		__field(  dev_t,	dev			)
> -		__field(  sector_t,	sector			)
> -		__field(  unsigned int,	nr_sector		)
> -		__field(  int	,	error			)
> -		__array(  char,		rwbs,	RWBS_LEN	)
> -		__dynamic_array( char,	cmd,	1		)
> +		__field(  dev_t,		dev			)
> +		__field(  sector_t,		sector			)
> +		__field(  unsigned int,		nr_sector		)
> +		__field(  int	,		error			)
> +		__field(  unsigned short,	ioprio			)
> +		__array(  char,			rwbs,	RWBS_LEN	)
> +		__dynamic_array( char,		cmd,	1		)
>   	),

Also here, please do not reformat lines that are not modified otherwise.

> @@ -176,13 +194,14 @@ DECLARE_EVENT_CLASS(block_rq,
>   	TP_ARGS(rq),
>   
>   	TP_STRUCT__entry(
> -		__field(  dev_t,	dev			)
> -		__field(  sector_t,	sector			)
> -		__field(  unsigned int,	nr_sector		)
> -		__field(  unsigned int,	bytes			)
> -		__array(  char,		rwbs,	RWBS_LEN	)
> -		__array(  char,         comm,   TASK_COMM_LEN   )
> -		__dynamic_array( char,	cmd,	1		)
> +		__field(  dev_t,		dev			)
> +		__field(  sector_t,		sector			)
> +		__field(  unsigned int,		nr_sector		)
> +		__field(  unsigned int,		bytes			)
> +		__field(  unsigned short,	ioprio			)
> +		__array(  char,			rwbs,	RWBS_LEN	)
> +		__array(  char,			comm,   TASK_COMM_LEN	)
> +		__dynamic_array( char,		cmd,	1		)
>   	),

Same comment here.

Thanks,

Bart.