| Message ID | 20240322185023.131697-2-joshi.k@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/4] io_uring/rw: Get rid of flags field in struct io_rw | expand |
On 2024-03-22 11:50, Kanchan Joshi wrote: > From: Anuj Gupta <anuj20.g@samsung.com> > > Get rid of the flags field in io_rw. Flags can be set in kiocb->flags > during prep rather than doing it while issuing the I/O in io_read/io_write. > > Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> > --- > io_uring/rw.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) This patch looks fine and is a no-op on its own, but I think there is a subtle semantic change. If the rw_flags is invalid (i.e. kiocb_set_rw_flags() returns an err) and prep() fails, then the remaining submissions won't be submitted unless IORING_SETUP_SUBMIT_ALL is set. Currently if kiocb_set_rw_flags() fails in prep(), only the request will fail.
On 2024-03-27 16:30, David Wei wrote: > On 2024-03-22 11:50, Kanchan Joshi wrote: >> From: Anuj Gupta <anuj20.g@samsung.com> >> >> Get rid of the flags field in io_rw. Flags can be set in kiocb->flags >> during prep rather than doing it while issuing the I/O in io_read/io_write. >> >> Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> >> --- >> io_uring/rw.c | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) > > This patch looks fine and is a no-op on its own, but I think there is a > subtle semantic change. If the rw_flags is invalid (i.e. > kiocb_set_rw_flags() returns an err) and prep() fails, then the > remaining submissions won't be submitted unless IORING_SETUP_SUBMIT_ALL > is set. > > Currently if kiocb_set_rw_flags() fails in prep(), only the request will > fail. Sorry, that should say fails in _issue()_.
diff --git a/io_uring/rw.c b/io_uring/rw.c index 47e097ab5d7e..40f6c2a59928 100644 --- a/io_uring/rw.c +++ b/io_uring/rw.c @@ -27,7 +27,6 @@ struct io_rw { struct kiocb kiocb; u64 addr; u32 len; - rwf_t flags; }; static inline bool io_file_supports_nowait(struct io_kiocb *req) @@ -78,10 +77,16 @@ static int io_iov_buffer_select_prep(struct io_kiocb *req) int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe) { struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw); + struct kiocb *kiocb = &rw->kiocb; unsigned ioprio; int ret; - rw->kiocb.ki_pos = READ_ONCE(sqe->off); + kiocb->ki_flags = 0; + ret = kiocb_set_rw_flags(kiocb, READ_ONCE(sqe->rw_flags)); + if (unlikely(ret)) + return ret; + + kiocb->ki_pos = READ_ONCE(sqe->off); /* used for fixed read/write too - just read unconditionally */ req->buf_index = READ_ONCE(sqe->buf_index); @@ -91,15 +96,14 @@ int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe) if (ret) return ret; - rw->kiocb.ki_ioprio = ioprio; + kiocb->ki_ioprio = ioprio; } else { - rw->kiocb.ki_ioprio = get_current_ioprio(); + kiocb->ki_ioprio = get_current_ioprio(); } - rw->kiocb.dio_complete = NULL; + kiocb->dio_complete = NULL; rw->addr = READ_ONCE(sqe->addr); rw->len = READ_ONCE(sqe->len); - rw->flags = READ_ONCE(sqe->rw_flags); return 0; } @@ -720,7 +724,6 @@ static int io_rw_init_file(struct io_kiocb *req, fmode_t mode) struct kiocb *kiocb = &rw->kiocb; struct io_ring_ctx *ctx = req->ctx; struct file *file = req->file; - int ret; if (unlikely(!(file->f_mode & mode))) return -EBADF; @@ -728,10 +731,7 @@ static int io_rw_init_file(struct io_kiocb *req, fmode_t mode) if (!(req->flags & REQ_F_FIXED_FILE)) req->flags |= io_file_get_flags(file); - kiocb->ki_flags = file->f_iocb_flags; - ret = kiocb_set_rw_flags(kiocb, rw->flags); - if (unlikely(ret)) - return ret; + kiocb->ki_flags |= file->f_iocb_flags; kiocb->ki_flags |= IOCB_ALLOC_CACHE; /*