On 1/6/25 7:04 PM, lizetao wrote:
> Hi,
>
>> -----Original Message-----
>> From: Jens Axboe <axboe@xxxxxxxxx>
>> Sent: Monday, January 6, 2025 10:46 PM
>> To: lizetao <lizetao1@xxxxxxxxxx>; Mark Harmstone <maharmstone@xxxxxx>
>> Cc: linux-btrfs@xxxxxxxxxxxxxxx; io-uring@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH 2/4] io_uring/cmd: add per-op data to struct
>> io_uring_cmd_data
>>
>> On 1/6/25 5:47 AM, lizetao wrote:
>>> Hi,
>>>
>>>> -----Original Message-----
>>>> From: Mark Harmstone <maharmstone@xxxxxx>
>>>> Sent: Friday, January 3, 2025 11:02 PM
>>>> To: linux-btrfs@xxxxxxxxxxxxxxx; io-uring@xxxxxxxxxxxxxxx
>>>> Cc: Jens Axboe <axboe@xxxxxxxxx>
>>>> Subject: [PATCH 2/4] io_uring/cmd: add per-op data to struct
>>>> io_uring_cmd_data
>>>>
>>>> From: Jens Axboe <axboe@xxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxx>
>>>> ---
>>>> 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.
>
> I still can't fully understand, the usage logic of op_data should be
> as follows: When applying for and initializing the cache, op_data has
> been set to NULL. In io_req_uring_cleanup, the op_data memory will be
> released and set to NULL. So if the cache in uring_cache, its op_data
> should be NULL? If it is non-NULL, is there a risk of memory leak if
> it is directly set to null?
Ah forgot I did clear it for freeing. So yes, this NULL setting on the
alloc side is redundant. But let's just leave it for now, once this gets
merged with the alloc cache cleanups that are pending for 6.14, it'll go
away anyway.
--
Jens Axboe
