Re: [PATCH v4 13/16] blktrace: add block trace commands for zone operations

[Damien Le Moal <dlemoal@kernel.org>]

On 10/20/25 22:41, Johannes Thumshirn wrote:
> Add block trace commands for zone operations. These commands can only be
> handled with version 2 of the blktrace protocol. For version 1, warn if a
> command that does not fit into the 16 bits reserved for the command in
> this version is passed in.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  include/uapi/linux/blktrace_api.h | 13 +++++++++++--
>  kernel/trace/blktrace.c           | 28 ++++++++++++++++++++++++----
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/blktrace_api.h b/include/uapi/linux/blktrace_api.h
> index 3a771b9802aa..925f78af939e 100644
> --- a/include/uapi/linux/blktrace_api.h
> +++ b/include/uapi/linux/blktrace_api.h
> @@ -26,11 +26,20 @@ enum blktrace_cat {
>  	BLK_TC_DRV_DATA	= 1 << 14,	/* binary per-driver data */
>  	BLK_TC_FUA	= 1 << 15,	/* fua requests */
>  
> -	BLK_TC_END	= 1 << 15,	/* we've run out of bits! */
> +	BLK_TC_END_V1	= 1 << 15,	/* we've run out of bits! */
> +
> +	BLK_TC_ZONE_APPEND	= 1ull << 16,  	/* zone append */
> +	BLK_TC_ZONE_RESET	= 1ull << 17,	/* zone reset */
> +	BLK_TC_ZONE_RESET_ALL	= 1ull << 18,	/* zone reset all */
> +	BLK_TC_ZONE_FINISH	= 1ull << 19,	/* zone finish */
> +	BLK_TC_ZONE_OPEN	= 1ull << 20,	/* zone open */
> +	BLK_TC_ZONE_CLOSE	= 1ull << 21,	/* zone close */
> +
> +	BLK_TC_END_V2		= 1ull << 21,
>  };
>  
>  #define BLK_TC_SHIFT		(16)
> -#define BLK_TC_ACT(act)		((act) << BLK_TC_SHIFT)
> +#define BLK_TC_ACT(act)		((u64)(act) << BLK_TC_SHIFT)
>  
>  /*
>   * Basic trace actions
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 8ffb218e9fb7..e8effb6cb393 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -163,8 +163,8 @@ static void relay_blktrace_event(struct blk_trace *bt, unsigned long sequence,
>  					     bytes, what, error, cgid, cgid_len,
>  					     pdu_data, pdu_len);
>  	return relay_blktrace_event1(bt, sequence, pid, cpu, sector, bytes,
> -				     lower_32_bits(what), error, cgid, cgid_len,
> -				     pdu_data, pdu_len);
> +				     what, error, cgid, cgid_len, pdu_data,
> +				     pdu_len);
>  }
>  
>  /*
> @@ -342,10 +342,31 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
>  	case REQ_OP_FLUSH:
>  		what |= BLK_TC_ACT(BLK_TC_FLUSH);
>  		break;
> +	case REQ_OP_ZONE_APPEND:
> +		what |= BLK_TC_ACT(BLK_TC_ZONE_APPEND);
> +		break;
> +	case REQ_OP_ZONE_RESET:
> +		what |= BLK_TC_ACT(BLK_TC_ZONE_RESET);
> +		break;
> +	case REQ_OP_ZONE_RESET_ALL:
> +		what |= BLK_TC_ACT(BLK_TC_ZONE_RESET_ALL);
> +		break;
> +	case REQ_OP_ZONE_FINISH:
> +		what |= BLK_TC_ACT(BLK_TC_ZONE_FINISH);
> +		break;
> +	case REQ_OP_ZONE_OPEN:
> +		what |= BLK_TC_ACT(BLK_TC_ZONE_OPEN);
> +		break;
> +	case REQ_OP_ZONE_CLOSE:
> +		what |= BLK_TC_ACT(BLK_TC_ZONE_CLOSE);
> +		break;
>  	default:
>  		break;
>  	}
>  
> +	WARN_ON_ONCE(bt->version == 1 &&
> +		     (what >> BLK_TC_SHIFT) > BLK_TC_END_V1);

Shouldn't this be a "if (WARN_ON_ONCE())" and return doing nothing if true ?

> +
>  	if (cgid)
>  		what |= __BLK_TA_CGROUP;
>  
> @@ -386,8 +407,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
>  	sequence = per_cpu_ptr(bt->sequence, cpu);
>  	(*sequence)++;
>  	relay_blktrace_event(bt, *sequence, pid, cpu, sector, bytes,
> -			     lower_32_bits(what), error, cgid, cgid_len,
> -			     pdu_data, pdu_len);
> +			     what, error, cgid, cgid_len, pdu_data, pdu_len);
>  	local_irq_restore(flags);
>  }
>  


-- 
Damien Le Moal
Western Digital Research