| Message ID | 20250124-optimize-fuse-uring-req-timeouts-v1-1-b834b5f32e85@ddn.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: {io-uring} Ensure fuse requests are set/read with locks | expand |
On Fri, Jan 24, 2025 at 8:47 AM Bernd Schubert <bschubert@ddn.com> wrote: > > The value is read from another task without, while the task that > had set the value was holding queue->lock. Better use READ_ONCE > to ensure the compiler cannot optimize the read. > > Fixes: 284985711dc5 ("fuse: Allow to queue fg requests through io-uring") > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > --- > fs/fuse/dev_uring.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c > index 5c9b5a5fb7f7539149840378e224eb640cf8ef08..1834c1933d2bbab0342257fde4b030f06506c55d 100644 > --- a/fs/fuse/dev_uring.c > +++ b/fs/fuse/dev_uring.c > @@ -1202,10 +1202,12 @@ static void fuse_uring_send_in_task(struct io_uring_cmd *cmd, > { > struct fuse_ring_ent *ent = uring_cmd_to_ring_ent(cmd); > struct fuse_ring_queue *queue = ent->queue; > + struct fuse_req *req; > int err; > > if (!(issue_flags & IO_URING_F_TASK_DEAD)) { > - err = fuse_uring_prepare_send(ent); > + req = READ_ONCE(ent->fuse_req); > + err = fuse_uring_prepare_send(ent, req); Hi Bernd, did you mean something like this?: diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c index 5c9b5a5fb7f7..692e97f9870d 100644 --- a/fs/fuse/dev_uring.c +++ b/fs/fuse/dev_uring.c @@ -676,7 +676,7 @@ static int fuse_uring_copy_to_ring(struct fuse_ring_ent *ent, static int fuse_uring_prepare_send(struct fuse_ring_ent *ent) { - struct fuse_req *req = ent->fuse_req; + struct fuse_req *req = READ_ONCE(ent->fuse_req); int err; err = fuse_uring_copy_to_ring(ent, req); I'm on top of the for-next tree but I'm not seeing where fuse_uring_prepare_send() takes in req as an arg. Thanks, Joanne > if (err) { > fuse_uring_next_fuse_req(ent, queue, issue_flags); > return; > > -- > 2.43.0 >
On 1/25/25 01:31, Joanne Koong wrote: > On Fri, Jan 24, 2025 at 8:47 AM Bernd Schubert <bschubert@ddn.com> wrote: >> >> The value is read from another task without, while the task that >> had set the value was holding queue->lock. Better use READ_ONCE >> to ensure the compiler cannot optimize the read. >> >> Fixes: 284985711dc5 ("fuse: Allow to queue fg requests through io-uring") >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >> --- >> fs/fuse/dev_uring.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c >> index 5c9b5a5fb7f7539149840378e224eb640cf8ef08..1834c1933d2bbab0342257fde4b030f06506c55d 100644 >> --- a/fs/fuse/dev_uring.c >> +++ b/fs/fuse/dev_uring.c >> @@ -1202,10 +1202,12 @@ static void fuse_uring_send_in_task(struct io_uring_cmd *cmd, >> { >> struct fuse_ring_ent *ent = uring_cmd_to_ring_ent(cmd); >> struct fuse_ring_queue *queue = ent->queue; >> + struct fuse_req *req; >> int err; >> >> if (!(issue_flags & IO_URING_F_TASK_DEAD)) { >> - err = fuse_uring_prepare_send(ent); >> + req = READ_ONCE(ent->fuse_req); >> + err = fuse_uring_prepare_send(ent, req); > > Hi Bernd, did you mean something like this?: > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c > index 5c9b5a5fb7f7..692e97f9870d 100644 > --- a/fs/fuse/dev_uring.c > +++ b/fs/fuse/dev_uring.c > @@ -676,7 +676,7 @@ static int fuse_uring_copy_to_ring(struct > fuse_ring_ent *ent, > > static int fuse_uring_prepare_send(struct fuse_ring_ent *ent) > { > - struct fuse_req *req = ent->fuse_req; > + struct fuse_req *req = READ_ONCE(ent->fuse_req); > int err; > > err = fuse_uring_copy_to_ring(ent, req); > > I'm on top of the for-next tree but I'm not seeing where > fuse_uring_prepare_send() takes in req as an arg. > > Wrong order of patches. Initially it was all in one patch and I had split it up and was in a hurry - didn't test compilation of individual patches. Thanks, Bernd
diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c index 5c9b5a5fb7f7539149840378e224eb640cf8ef08..1834c1933d2bbab0342257fde4b030f06506c55d 100644 --- a/fs/fuse/dev_uring.c +++ b/fs/fuse/dev_uring.c @@ -1202,10 +1202,12 @@ static void fuse_uring_send_in_task(struct io_uring_cmd *cmd, { struct fuse_ring_ent *ent = uring_cmd_to_ring_ent(cmd); struct fuse_ring_queue *queue = ent->queue; + struct fuse_req *req; int err; if (!(issue_flags & IO_URING_F_TASK_DEAD)) { - err = fuse_uring_prepare_send(ent); + req = READ_ONCE(ent->fuse_req); + err = fuse_uring_prepare_send(ent, req); if (err) { fuse_uring_next_fuse_req(ent, queue, issue_flags); return;
The value is read from another task without, while the task that had set the value was holding queue->lock. Better use READ_ONCE to ensure the compiler cannot optimize the read. Fixes: 284985711dc5 ("fuse: Allow to queue fg requests through io-uring") Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- fs/fuse/dev_uring.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)