| Message ID | 2bedcfe941cd2b594c4ee1658276f5c1b008feb8.1740412523.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clean up rw buffer import | expand |
On Mon, Feb 24, 2025 at 8:07 AM Pavel Begunkov <asml.silence@gmail.com> wrote: > > io_prep_rw() relies on async_data being allocated in io_prep_rw_setup(). > Be a bit more explicit and move the allocation earlier into io_prep_rw() > and don't hide it in a call chain. Hmm, where is async_data currently used in io_prep_rw()? I don't see any reference to async_data in io_prep_rw() until your patch 4, "io_uring/rw: open code io_prep_rw_setup()". Would it make sense to combine the 2 patches? Best, Caleb > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > io_uring/rw.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/io_uring/rw.c b/io_uring/rw.c > index 22612a956e75..7efc2337c5a0 100644 > --- a/io_uring/rw.c > +++ b/io_uring/rw.c > @@ -203,9 +203,6 @@ static int io_prep_rw_setup(struct io_kiocb *req, int ddir, bool do_import) > { > struct io_async_rw *rw; > > - if (io_rw_alloc_async(req)) > - return -ENOMEM; > - > if (!do_import || io_do_buffer_select(req)) > return 0; > > @@ -262,6 +259,9 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe, > u64 attr_type_mask; > int ret; > > + if (io_rw_alloc_async(req)) > + return -ENOMEM; > + > rw->kiocb.ki_pos = READ_ONCE(sqe->off); > /* used for fixed read/write too - just read unconditionally */ > req->buf_index = READ_ONCE(sqe->buf_index); > -- > 2.48.1 > >
On 2/24/25 16:52, Caleb Sander Mateos wrote: > On Mon, Feb 24, 2025 at 8:07 AM Pavel Begunkov <asml.silence@gmail.com> wrote: >> >> io_prep_rw() relies on async_data being allocated in io_prep_rw_setup(). >> Be a bit more explicit and move the allocation earlier into io_prep_rw() >> and don't hide it in a call chain. > > Hmm, where is async_data currently used in io_prep_rw()? I don't see It calls io_prep_rw_pi(), which uses it inside, that's the "relies" part. > any reference to async_data in io_prep_rw() until your patch 4, > "io_uring/rw: open code io_prep_rw_setup()". Would it make sense to > combine the 2 patches? Sure, if it rebases cleanly.
On 2/24/25 19:21, Pavel Begunkov wrote: > On 2/24/25 16:52, Caleb Sander Mateos wrote: >> On Mon, Feb 24, 2025 at 8:07 AM Pavel Begunkov <asml.silence@gmail.com> wrote: >>> >>> io_prep_rw() relies on async_data being allocated in io_prep_rw_setup(). >>> Be a bit more explicit and move the allocation earlier into io_prep_rw() >>> and don't hide it in a call chain. >> >> Hmm, where is async_data currently used in io_prep_rw()? I don't see > > It calls io_prep_rw_pi(), which uses it inside, that's the "relies" > part. > >> any reference to async_data in io_prep_rw() until your patch 4, >> "io_uring/rw: open code io_prep_rw_setup()". Would it make sense to >> combine the 2 patches? > > Sure, if it rebases cleanly. It doesn't... I'd rather prefer to keep it as is then.
diff --git a/io_uring/rw.c b/io_uring/rw.c index 22612a956e75..7efc2337c5a0 100644 --- a/io_uring/rw.c +++ b/io_uring/rw.c @@ -203,9 +203,6 @@ static int io_prep_rw_setup(struct io_kiocb *req, int ddir, bool do_import) { struct io_async_rw *rw; - if (io_rw_alloc_async(req)) - return -ENOMEM; - if (!do_import || io_do_buffer_select(req)) return 0; @@ -262,6 +259,9 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe, u64 attr_type_mask; int ret; + if (io_rw_alloc_async(req)) + return -ENOMEM; + rw->kiocb.ki_pos = READ_ONCE(sqe->off); /* used for fixed read/write too - just read unconditionally */ req->buf_index = READ_ONCE(sqe->buf_index);
io_prep_rw() relies on async_data being allocated in io_prep_rw_setup(). Be a bit more explicit and move the allocation earlier into io_prep_rw() and don't hide it in a call chain. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- io_uring/rw.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)