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

[lizetao <lizetao1@huawei.com>]

Hi,

> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Monday, January 6, 2025 10:46 PM
> To: lizetao <lizetao1@huawei.com>; Mark Harmstone <maharmstone@fb.com>
> Cc: linux-btrfs@vger.kernel.org; io-uring@vger.kernel.org
> 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@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.

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?
> 
> --
> Jens Axboe

---
Li Zetao