Re: [PATCH 2/4] io_uring/cmd: add per-op data to struct io_uring_cmd_data

[Jens Axboe <axboe@kernel.dk>]

On 1/6/25 5:47 AM, lizetao wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Mark Harmstone <maharmstone@fb.com>
>> Sent: Friday, January 3, 2025 11:02 PM
>> To: linux-btrfs@vger.kernel.org; io-uring@vger.kernel.org
>> Cc: Jens Axboe <axboe@kernel.dk>
>> Subject: [PATCH 2/4] io_uring/cmd: add per-op data to struct
>> io_uring_cmd_data
>>
>> From: Jens Axboe <axboe@kernel.dk>
>>
>> In case an op handler for ->uring_cmd() needs stable storage for user data, it
>> can allocate io_uring_cmd_data->op_data and use it for the duration of the
>> request. When the request gets cleaned up, uring_cmd will free it
>> automatically.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  include/linux/io_uring/cmd.h |  1 +
>>  io_uring/uring_cmd.c         | 13 +++++++++++--
>>  2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/io_uring/cmd.h b/include/linux/io_uring/cmd.h index
>> 61f97a398e9d..a65c7043078f 100644
>> --- a/include/linux/io_uring/cmd.h
>> +++ b/include/linux/io_uring/cmd.h
>> @@ -20,6 +20,7 @@ struct io_uring_cmd {
>>
>>  struct io_uring_cmd_data {
>>  	struct io_uring_sqe	sqes[2];
>> +	void			*op_data;
>>  };
>>
>>  static inline const void *io_uring_sqe_cmd(const struct io_uring_sqe *sqe) diff
>> --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index
>> 629cb4266da6..ce7726a04883 100644
>> --- a/io_uring/uring_cmd.c
>> +++ b/io_uring/uring_cmd.c
>> @@ -23,12 +23,16 @@ static struct io_uring_cmd_data
>> *io_uring_async_get(struct io_kiocb *req)
>>
>>  	cache = io_alloc_cache_get(&ctx->uring_cache);
>>  	if (cache) {
>> +		cache->op_data = NULL;
> 
> Why is op_data set to NULL here? If you are worried about some
> omissions, would it be better to use WARN_ON to assert that op_data is
> a null pointer? This will also make it easier to analyze the cause of
> the problem.

Clearing the per-op data is prudent when allocating getting this struct,
to avoid previous garbage. The alternative would be clearing it when
it's freed, either way is fine imho. A WARN_ON would not make sense, as
it can validly be non-NULL already.

-- 
Jens Axboe