* [PATCH v3 1/4] io_uring: remove duplicated calls to io_kiocb_ppos
2022-02-22 10:55 [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write Dylan Yudaken
@ 2022-02-22 10:55 ` Dylan Yudaken
2022-02-23 23:06 ` Pavel Begunkov
2022-02-22 10:55 ` [PATCH v3 2/4] io_uring: update kiocb->ki_pos at execution time Dylan Yudaken
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Dylan Yudaken @ 2022-02-22 10:55 UTC (permalink / raw)
To: Jens Axboe, Pavel Begunkov, io-uring; +Cc: kernel-team, Dylan Yudaken
io_kiocb_ppos is called in both branches, and it seems that the compiler
does not fuse this. Fusing removes a few bytes from loop_rw_iter.
Before:
$ nm -S fs/io_uring.o | grep loop_rw_iter
0000000000002430 0000000000000124 t loop_rw_iter
After:
$ nm -S fs/io_uring.o | grep loop_rw_iter
0000000000002430 000000000000010d t loop_rw_iter
Signed-off-by: Dylan Yudaken <[email protected]>
---
fs/io_uring.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 77b9c7e4793b..1f9b4466c269 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3400,6 +3400,7 @@ static ssize_t loop_rw_iter(int rw, struct io_kiocb *req, struct iov_iter *iter)
struct kiocb *kiocb = &req->rw.kiocb;
struct file *file = req->file;
ssize_t ret = 0;
+ loff_t *ppos;
/*
* Don't support polled IO through this interface, and we can't
@@ -3412,6 +3413,8 @@ static ssize_t loop_rw_iter(int rw, struct io_kiocb *req, struct iov_iter *iter)
!(kiocb->ki_filp->f_flags & O_NONBLOCK))
return -EAGAIN;
+ ppos = io_kiocb_ppos(kiocb);
+
while (iov_iter_count(iter)) {
struct iovec iovec;
ssize_t nr;
@@ -3425,10 +3428,10 @@ static ssize_t loop_rw_iter(int rw, struct io_kiocb *req, struct iov_iter *iter)
if (rw == READ) {
nr = file->f_op->read(file, iovec.iov_base,
- iovec.iov_len, io_kiocb_ppos(kiocb));
+ iovec.iov_len, ppos);
} else {
nr = file->f_op->write(file, iovec.iov_base,
- iovec.iov_len, io_kiocb_ppos(kiocb));
+ iovec.iov_len, ppos);
}
if (nr < 0) {
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 2/4] io_uring: update kiocb->ki_pos at execution time
2022-02-22 10:55 [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write Dylan Yudaken
2022-02-22 10:55 ` [PATCH v3 1/4] io_uring: remove duplicated calls to io_kiocb_ppos Dylan Yudaken
@ 2022-02-22 10:55 ` Dylan Yudaken
2022-02-23 23:06 ` Pavel Begunkov
2022-02-22 10:55 ` [PATCH v3 3/4] io_uring: do not recalculate ppos unnecessarily Dylan Yudaken
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Dylan Yudaken @ 2022-02-22 10:55 UTC (permalink / raw)
To: Jens Axboe, Pavel Begunkov, io-uring; +Cc: kernel-team, Dylan Yudaken
Update kiocb->ki_pos at execution time rather than in io_prep_rw().
io_prep_rw() happens before the job is enqueued to a worker and so the
offset might be read multiple times before being executed once.
Ensures that the file position in a set of _linked_ SQEs will be only
obtained after earlier SQEs have completed, and so will include their
incremented file position.
Signed-off-by: Dylan Yudaken <[email protected]>
---
fs/io_uring.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 1f9b4466c269..aba2a426a2d1 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3000,14 +3000,6 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe)
req->flags |= io_file_get_flags(file) << REQ_F_SUPPORT_NOWAIT_BIT;
kiocb->ki_pos = READ_ONCE(sqe->off);
- if (kiocb->ki_pos == -1) {
- if (!(file->f_mode & FMODE_STREAM)) {
- req->flags |= REQ_F_CUR_POS;
- kiocb->ki_pos = file->f_pos;
- } else {
- kiocb->ki_pos = 0;
- }
- }
kiocb->ki_flags = iocb_flags(file);
ret = kiocb_set_rw_flags(kiocb, READ_ONCE(sqe->rw_flags));
if (unlikely(ret))
@@ -3074,6 +3066,20 @@ static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
}
}
+static inline void io_kiocb_update_pos(struct io_kiocb *req)
+{
+ struct kiocb *kiocb = &req->rw.kiocb;
+
+ if (kiocb->ki_pos == -1) {
+ if (!(req->file->f_mode & FMODE_STREAM)) {
+ req->flags |= REQ_F_CUR_POS;
+ kiocb->ki_pos = req->file->f_pos;
+ } else {
+ kiocb->ki_pos = 0;
+ }
+ }
+}
+
static void kiocb_done(struct io_kiocb *req, ssize_t ret,
unsigned int issue_flags)
{
@@ -3662,6 +3668,8 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
kiocb->ki_flags &= ~IOCB_NOWAIT;
}
+ io_kiocb_update_pos(req);
+
ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), req->result);
if (unlikely(ret)) {
kfree(iovec);
@@ -3791,6 +3799,8 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
kiocb->ki_flags &= ~IOCB_NOWAIT;
}
+ io_kiocb_update_pos(req);
+
ret = rw_verify_area(WRITE, req->file, io_kiocb_ppos(kiocb), req->result);
if (unlikely(ret))
goto out_free;
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 3/4] io_uring: do not recalculate ppos unnecessarily
2022-02-22 10:55 [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write Dylan Yudaken
2022-02-22 10:55 ` [PATCH v3 1/4] io_uring: remove duplicated calls to io_kiocb_ppos Dylan Yudaken
2022-02-22 10:55 ` [PATCH v3 2/4] io_uring: update kiocb->ki_pos at execution time Dylan Yudaken
@ 2022-02-22 10:55 ` Dylan Yudaken
2022-02-23 23:07 ` Pavel Begunkov
2022-02-22 10:55 ` [PATCH v3 4/4] io_uring: pre-increment f_pos on rw Dylan Yudaken
2022-02-23 23:51 ` [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write Jens Axboe
4 siblings, 1 reply; 10+ messages in thread
From: Dylan Yudaken @ 2022-02-22 10:55 UTC (permalink / raw)
To: Jens Axboe, Pavel Begunkov, io-uring; +Cc: kernel-team, Dylan Yudaken
There is a slight optimisation to be had by calculating the correct pos
pointer inside io_kiocb_update_pos and then using that later.
It seems code size drops by a bit:
000000000000a1b0 0000000000000400 t io_read
000000000000a5b0 0000000000000319 t io_write
vs
000000000000a1b0 00000000000003f6 t io_read
000000000000a5b0 0000000000000310 t io_write
Signed-off-by: Dylan Yudaken <[email protected]>
---
fs/io_uring.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index aba2a426a2d1..8954d82def36 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3066,18 +3066,22 @@ static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
}
}
-static inline void io_kiocb_update_pos(struct io_kiocb *req)
+static inline loff_t *io_kiocb_update_pos(struct io_kiocb *req)
{
struct kiocb *kiocb = &req->rw.kiocb;
+ bool is_stream = req->file->f_mode & FMODE_STREAM;
if (kiocb->ki_pos == -1) {
- if (!(req->file->f_mode & FMODE_STREAM)) {
+ if (!is_stream) {
req->flags |= REQ_F_CUR_POS;
kiocb->ki_pos = req->file->f_pos;
+ return &kiocb->ki_pos;
} else {
kiocb->ki_pos = 0;
+ return NULL;
}
}
+ return is_stream ? NULL : &kiocb->ki_pos;
}
static void kiocb_done(struct io_kiocb *req, ssize_t ret,
@@ -3638,6 +3642,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
struct io_async_rw *rw;
ssize_t ret, ret2;
+ loff_t *ppos;
if (!req_has_async_data(req)) {
ret = io_import_iovec(READ, req, &iovec, s, issue_flags);
@@ -3668,9 +3673,9 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
kiocb->ki_flags &= ~IOCB_NOWAIT;
}
- io_kiocb_update_pos(req);
+ ppos = io_kiocb_update_pos(req);
- ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), req->result);
+ ret = rw_verify_area(READ, req->file, ppos, req->result);
if (unlikely(ret)) {
kfree(iovec);
return ret;
@@ -3769,6 +3774,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
struct kiocb *kiocb = &req->rw.kiocb;
bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
ssize_t ret, ret2;
+ loff_t *ppos;
if (!req_has_async_data(req)) {
ret = io_import_iovec(WRITE, req, &iovec, s, issue_flags);
@@ -3799,9 +3805,9 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
kiocb->ki_flags &= ~IOCB_NOWAIT;
}
- io_kiocb_update_pos(req);
+ ppos = io_kiocb_update_pos(req);
- ret = rw_verify_area(WRITE, req->file, io_kiocb_ppos(kiocb), req->result);
+ ret = rw_verify_area(WRITE, req->file, ppos, req->result);
if (unlikely(ret))
goto out_free;
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v3 4/4] io_uring: pre-increment f_pos on rw
2022-02-22 10:55 [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write Dylan Yudaken
` (2 preceding siblings ...)
2022-02-22 10:55 ` [PATCH v3 3/4] io_uring: do not recalculate ppos unnecessarily Dylan Yudaken
@ 2022-02-22 10:55 ` Dylan Yudaken
2022-02-23 23:51 ` [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write Jens Axboe
4 siblings, 0 replies; 10+ messages in thread
From: Dylan Yudaken @ 2022-02-22 10:55 UTC (permalink / raw)
To: Jens Axboe, Pavel Begunkov, io-uring; +Cc: kernel-team, Dylan Yudaken
In read/write ops, preincrement f_pos when no offset is specified, and
then attempt fix up the position after IO completes if it completed less
than expected. This fixes the problem where multiple queued up IO will all
obtain the same f_pos, and so perform the same read/write.
This is still not as consistent as sync r/w, as it is able to advance the
file offset past the end of the file. It seems it would be quite a
performance hit to work around this limitation - such as by keeping track
of concurrent operations - and the downside does not seem to be too
problematic.
The attempt to fix up the f_pos after will at least mean that in situations
where a single operation is run, then the position will be consistent.
Co-developed-by: Jens Axboe <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
Signed-off-by: Dylan Yudaken <[email protected]>
---
fs/io_uring.c | 95 ++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 82 insertions(+), 13 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 8954d82def36..adb15234e53c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3066,22 +3066,86 @@ static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
}
}
-static inline loff_t *io_kiocb_update_pos(struct io_kiocb *req)
+static bool __io_kiocb_update_pos(struct io_kiocb *req, loff_t **ppos,
+ u64 expected, bool force_nonblock)
+{
+ struct kiocb *kiocb = &req->rw.kiocb;
+
+ WARN_ON(req->flags & REQ_F_CUR_POS);
+ if (req->file->f_mode & FMODE_ATOMIC_POS) {
+ if (force_nonblock) {
+ if (!mutex_trylock(&req->file->f_pos_lock))
+ return true;
+ } else {
+ mutex_lock(&req->file->f_pos_lock);
+ }
+ }
+ kiocb->ki_pos = req->file->f_pos;
+ req->file->f_pos += expected;
+ if (req->file->f_mode & FMODE_ATOMIC_POS)
+ mutex_unlock(&req->file->f_pos_lock);
+
+ *ppos = &kiocb->ki_pos;
+ req->flags |= REQ_F_CUR_POS;
+ return false;
+}
+
+static inline bool io_kiocb_update_pos(struct io_kiocb *req, loff_t **ppos,
+ u64 expected, bool force_nonblock)
{
struct kiocb *kiocb = &req->rw.kiocb;
bool is_stream = req->file->f_mode & FMODE_STREAM;
if (kiocb->ki_pos == -1) {
if (!is_stream) {
- req->flags |= REQ_F_CUR_POS;
- kiocb->ki_pos = req->file->f_pos;
- return &kiocb->ki_pos;
+ return __io_kiocb_update_pos(req, ppos, expected,
+ force_nonblock);
} else {
kiocb->ki_pos = 0;
- return NULL;
+ *ppos = NULL;
+ return false;
}
}
- return is_stream ? NULL : &kiocb->ki_pos;
+ *ppos = is_stream ? NULL : &kiocb->ki_pos;
+ return false;
+}
+
+static void __io_kiocb_done_pos(struct io_kiocb *req, u64 actual)
+{
+ struct kiocb *kiocb = &req->rw.kiocb;
+ u64 expected;
+
+ expected = req->rw.len;
+ if (actual >= expected)
+ return;
+
+ /*
+ * It's not definitely safe to lock here, and the assumption is,
+ * that if we cannot lock the position that it will be changing,
+ * and if it will be changing - then we can't update it anyway
+ */
+ if (req->file->f_mode & FMODE_ATOMIC_POS
+ && !mutex_trylock(&req->file->f_pos_lock))
+ return;
+
+ /*
+ * now we want to move the pointer, but only if everything is consistent
+ * with how we left it originally
+ */
+ if (req->file->f_pos == kiocb->ki_pos + (expected - actual))
+ req->file->f_pos = kiocb->ki_pos;
+
+ /* else something else messed with f_pos and we can't do anything */
+
+ if (req->file->f_mode & FMODE_ATOMIC_POS)
+ mutex_unlock(&req->file->f_pos_lock);
+}
+
+static inline void io_kiocb_done_pos(struct io_kiocb *req, u64 actual)
+{
+ if (likely(!(req->flags & REQ_F_CUR_POS)))
+ return;
+ __io_kiocb_done_pos(req, actual);
}
static void kiocb_done(struct io_kiocb *req, ssize_t ret,
@@ -3097,8 +3161,7 @@ static void kiocb_done(struct io_kiocb *req, ssize_t ret,
ret += io->bytes_done;
}
- if (req->flags & REQ_F_CUR_POS)
- req->file->f_pos = req->rw.kiocb.ki_pos;
+ io_kiocb_done_pos(req, ret >= 0 ? ret : 0);
if (ret >= 0 && (req->rw.kiocb.ki_complete == io_complete_rw))
__io_complete_rw(req, ret, issue_flags);
else
@@ -3663,21 +3726,23 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
if (force_nonblock) {
/* If the file doesn't support async, just async punt */
- if (unlikely(!io_file_supports_nowait(req))) {
+ if (unlikely(!io_file_supports_nowait(req) ||
+ io_kiocb_update_pos(req, &ppos,
+ req->rw.len, true))) {
ret = io_setup_async_rw(req, iovec, s, true);
return ret ?: -EAGAIN;
}
kiocb->ki_flags |= IOCB_NOWAIT;
} else {
+ io_kiocb_update_pos(req, &ppos, req->rw.len, false);
/* Ensure we clear previously set non-block flag */
kiocb->ki_flags &= ~IOCB_NOWAIT;
}
- ppos = io_kiocb_update_pos(req);
-
ret = rw_verify_area(READ, req->file, ppos, req->result);
if (unlikely(ret)) {
kfree(iovec);
+ io_kiocb_done_pos(req, 0);
return ret;
}
@@ -3799,14 +3864,17 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
(req->flags & REQ_F_ISREG))
goto copy_iov;
+ /* if we cannot lock the file position then punt */
+ if (unlikely(io_kiocb_update_pos(req, &ppos, req->rw.len, true)))
+ goto copy_iov;
+
kiocb->ki_flags |= IOCB_NOWAIT;
} else {
+ io_kiocb_update_pos(req, &ppos, req->rw.len, false);
/* Ensure we clear previously set non-block flag */
kiocb->ki_flags &= ~IOCB_NOWAIT;
}
- ppos = io_kiocb_update_pos(req);
-
ret = rw_verify_area(WRITE, req->file, ppos, req->result);
if (unlikely(ret))
goto out_free;
@@ -3859,6 +3927,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
return ret ?: -EAGAIN;
}
out_free:
+ io_kiocb_done_pos(req, 0);
/* it's reportedly faster than delegating the null check to kfree() */
if (iovec)
kfree(iovec);
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write
2022-02-22 10:55 [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write Dylan Yudaken
` (3 preceding siblings ...)
2022-02-22 10:55 ` [PATCH v3 4/4] io_uring: pre-increment f_pos on rw Dylan Yudaken
@ 2022-02-23 23:51 ` Jens Axboe
2022-02-24 10:45 ` Dylan Yudaken
4 siblings, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2022-02-23 23:51 UTC (permalink / raw)
To: Dylan Yudaken, Pavel Begunkov, io-uring; +Cc: kernel-team
On 2/22/22 3:55 AM, Dylan Yudaken wrote:
> Currently submitting multiple read/write for one file with offset = -1 will
> not behave as if calling read(2)/write(2) multiple times. The offset may be
> pinned to the same value for each submission (for example if they are
> punted to the async worker) and so each read/write will have the same
> offset.
>
> This patch series fixes this.
>
> Patch 1,3 cleans up the code a bit
>
> Patch 2 grabs the file position at execution time, rather than when the job
> is queued to be run which fixes inconsistincies when jobs are run asynchronously.
>
> Patch 4 increments the file's f_pos when reading it, which fixes
> inconsistincies with concurrent runs.
>
> A test for this will be submitted to liburing separately.
Applied 1-3 for now, as those are clean fixes and #4 is still being
debated. Thanks!
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write
2022-02-23 23:51 ` [PATCH v3 0/4] io_uring: consistent behaviour with linked read/write Jens Axboe
@ 2022-02-24 10:45 ` Dylan Yudaken
0 siblings, 0 replies; 10+ messages in thread
From: Dylan Yudaken @ 2022-02-24 10:45 UTC (permalink / raw)
To: [email protected], [email protected], [email protected]
Cc: Kernel Team
On Wed, 2022-02-23 at 16:51 -0700, Jens Axboe wrote:
> On 2/22/22 3:55 AM, Dylan Yudaken wrote:
> > Currently submitting multiple read/write for one file with offset =
> > -1 will
> > not behave as if calling read(2)/write(2) multiple times. The
> > offset may be
> > pinned to the same value for each submission (for example if they
> > are
> > punted to the async worker) and so each read/write will have the
> > same
> > offset.
> >
> > This patch series fixes this.
> >
> > Patch 1,3 cleans up the code a bit
> >
> > Patch 2 grabs the file position at execution time, rather than when
> > the job
> > is queued to be run which fixes inconsistincies when jobs are run
> > asynchronously.
> >
> > Patch 4 increments the file's f_pos when reading it, which fixes
> > inconsistincies with concurrent runs.
> >
> > A test for this will be submitted to liburing separately.
>
> Applied 1-3 for now, as those are clean fixes and #4 is still being
> debated. Thanks!
>
Thanks! I'll send a follow-up liburing test patch to remove the tests
that rely on #4. Can always revert it if/when #4 gets merged.
^ permalink raw reply [flat|nested] 10+ messages in thread