* [PATCH 0/3] io_uring: ensure that entries don't contain old data
@ 2021-08-26 16:45 Niklas Cassel
2021-08-26 16:45 ` [PATCH 1/3] io_uring: always initialize sqe->flags Niklas Cassel
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Niklas Cassel @ 2021-08-26 16:45 UTC (permalink / raw)
To: axboe; +Cc: fio, Damien Le Moal, Niklas Cassel
From: Niklas Cassel <niklas.cassel@wdc.com>
Hello,
I noticed a bug when testing cmdprio_percentage with io_uring.
Turns out that this was because sqe->ioprio contained old data.
Fixed one additional field that could contain old data (sqe->flags),
as well as one field that was always cleared _after_ the flags for
this field had been set (sqe->rw_flags).
There might be additional fields that might contain old data, e.g.
sqe->sync_range_flags and sqe->fsync_flags. However, I assume that
io_uring ignores those flags for read and write commands, however,
they might still be able to interfere with non-read/write commands.
Kind regards,
Niklas
Niklas Cassel (3):
io_uring: always initialize sqe->flags
io_uring: don't clear recently set sqe->rw_flags
io_uring: fix misbehaving cmdprio_percentage option
engines/io_uring.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/3] io_uring: always initialize sqe->flags
2021-08-26 16:45 [PATCH 0/3] io_uring: ensure that entries don't contain old data Niklas Cassel
@ 2021-08-26 16:45 ` Niklas Cassel
2021-08-26 16:45 ` [PATCH 2/3] io_uring: don't clear recently set sqe->rw_flags Niklas Cassel
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Niklas Cassel @ 2021-08-26 16:45 UTC (permalink / raw)
To: axboe; +Cc: fio, Damien Le Moal, Niklas Cassel
From: Niklas Cassel <niklas.cassel@wdc.com>
Commit 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
removed the memset of sqe from fio_ioring_prep().
Later, force_async was added in commit 5a59a81d2923 ("engines/io_uring:
allow setting of IOSQE_ASYNC").
The force_async commit sets sqe->flags every N requests, however,
since we no longer do a memset, this commit should have made sure that
flags is always initialized, such that we don't have sqe->flags set on
reused sqes where we didn't intend to.
Fixes: 5a59a81d2923 ("engines/io_uring: allow setting of IOSQE_ASYNC")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
engines/io_uring.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/engines/io_uring.c b/engines/io_uring.c
index 9c091e37..269e501f 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -234,6 +234,7 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
sqe->flags = IOSQE_FIXED_FILE;
} else {
sqe->fd = f->fd;
+ sqe->flags = 0;
}
if (io_u->ddir == DDIR_READ || io_u->ddir == DDIR_WRITE) {
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] io_uring: fix misbehaving cmdprio_percentage option
2021-08-26 16:45 [PATCH 0/3] io_uring: ensure that entries don't contain old data Niklas Cassel
2021-08-26 16:45 ` [PATCH 1/3] io_uring: always initialize sqe->flags Niklas Cassel
2021-08-26 16:45 ` [PATCH 2/3] io_uring: don't clear recently set sqe->rw_flags Niklas Cassel
@ 2021-08-26 16:45 ` Niklas Cassel
2021-08-26 16:50 ` [PATCH 0/3] io_uring: ensure that entries don't contain old data Jens Axboe
3 siblings, 0 replies; 5+ messages in thread
From: Niklas Cassel @ 2021-08-26 16:45 UTC (permalink / raw)
To: axboe; +Cc: fio, Damien Le Moal, Niklas Cassel
From: Niklas Cassel <niklas.cassel@wdc.com>
Commit 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
removed the memset of sqe from fio_ioring_prep().
Before this commit, fio_ioring_prio_prep() behaved properly, because
sqe->ioprio was always cleared by the memset in fio_ioring_prep().
cmdprio_percentage=20 is supposed to set the highest priority class for
20% of the total I/Os, however, because sqes got reused without clearing
the ioprio field, this meant that the number of I/Os sent with the highest
priority became 95% already after 10 seconds. Quite far off from the
intended 20%.
Fix this by explicitly clearing the priority in fio_ioring_prio_prep().
Note that prio/prioclass cannot be used together with cmdprio_percentage,
so we do not need to do an additional clear in fio_ioring_prep().
engines/libaio.c doesn't explicitly clear the ioprio, nor does it memset
the descriptor entry, this is because io_prep_pread()/io_prep_pwrite() in
libaio itself performs a memset.
Fixes: 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
engines/io_uring.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/engines/io_uring.c b/engines/io_uring.c
index 2b65572d..b8d4cf91 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -384,6 +384,8 @@ static void fio_ioring_prio_prep(struct thread_data *td, struct io_u *io_u)
if (rand_between(&td->prio_state, 0, 99) < o->cmdprio_percentage) {
ld->sqes[io_u->index].ioprio = IOPRIO_CLASS_RT << IOPRIO_CLASS_SHIFT;
io_u->flags |= IO_U_F_PRIORITY;
+ } else {
+ ld->sqes[io_u->index].ioprio = 0;
}
return;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] io_uring: don't clear recently set sqe->rw_flags
2021-08-26 16:45 [PATCH 0/3] io_uring: ensure that entries don't contain old data Niklas Cassel
2021-08-26 16:45 ` [PATCH 1/3] io_uring: always initialize sqe->flags Niklas Cassel
@ 2021-08-26 16:45 ` Niklas Cassel
2021-08-26 16:45 ` [PATCH 3/3] io_uring: fix misbehaving cmdprio_percentage option Niklas Cassel
2021-08-26 16:50 ` [PATCH 0/3] io_uring: ensure that entries don't contain old data Jens Axboe
3 siblings, 0 replies; 5+ messages in thread
From: Niklas Cassel @ 2021-08-26 16:45 UTC (permalink / raw)
To: axboe; +Cc: fio, Damien Le Moal, Niklas Cassel
From: Niklas Cassel <niklas.cassel@wdc.com>
Commit 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
removed the memset of sqe from fio_ioring_prep().
This commit did add a clear of the sqe->rw_flags, however, it did so
after both RWF_UNCACHED and RWF_NOWAIT flags might have been set,
effectively clearing these flags if they got set.
This doesn't make any sense. Make sure that we clear sqe->rw_flags
before, not after, setting the flags.
Fixes: 7c70f506e438 ("engines/io_uring: move sqe clear out of hot path")
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
engines/io_uring.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/engines/io_uring.c b/engines/io_uring.c
index 269e501f..2b65572d 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -262,8 +262,9 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
sqe->len = 1;
}
}
+ sqe->rw_flags = 0;
if (!td->o.odirect && o->uncached)
- sqe->rw_flags = RWF_UNCACHED;
+ sqe->rw_flags |= RWF_UNCACHED;
if (o->nowait)
sqe->rw_flags |= RWF_NOWAIT;
if (ld->ioprio_class_set)
@@ -271,7 +272,6 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
if (ld->ioprio_set)
sqe->ioprio |= td->o.ioprio;
sqe->off = io_u->offset;
- sqe->rw_flags = 0;
} else if (ddir_sync(io_u->ddir)) {
sqe->ioprio = 0;
if (io_u->ddir == DDIR_SYNC_FILE_RANGE) {
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/3] io_uring: ensure that entries don't contain old data
2021-08-26 16:45 [PATCH 0/3] io_uring: ensure that entries don't contain old data Niklas Cassel
` (2 preceding siblings ...)
2021-08-26 16:45 ` [PATCH 3/3] io_uring: fix misbehaving cmdprio_percentage option Niklas Cassel
@ 2021-08-26 16:50 ` Jens Axboe
3 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2021-08-26 16:50 UTC (permalink / raw)
To: Niklas Cassel; +Cc: fio, Damien Le Moal
On 8/26/21 10:45 AM, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
>
> Hello,
>
> I noticed a bug when testing cmdprio_percentage with io_uring.
> Turns out that this was because sqe->ioprio contained old data.
>
> Fixed one additional field that could contain old data (sqe->flags),
> as well as one field that was always cleared _after_ the flags for
> this field had been set (sqe->rw_flags).
>
> There might be additional fields that might contain old data, e.g.
> sqe->sync_range_flags and sqe->fsync_flags. However, I assume that
> io_uring ignores those flags for read and write commands, however,
> they might still be able to interfere with non-read/write commands.
Thanks, looks good, applied.
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-26 16:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-26 16:45 [PATCH 0/3] io_uring: ensure that entries don't contain old data Niklas Cassel
2021-08-26 16:45 ` [PATCH 1/3] io_uring: always initialize sqe->flags Niklas Cassel
2021-08-26 16:45 ` [PATCH 2/3] io_uring: don't clear recently set sqe->rw_flags Niklas Cassel
2021-08-26 16:45 ` [PATCH 3/3] io_uring: fix misbehaving cmdprio_percentage option Niklas Cassel
2021-08-26 16:50 ` [PATCH 0/3] io_uring: ensure that entries don't contain old data Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).