| Message ID | 20250125-optimize-fuse-uring-req-timeouts-v2-7-7771a2300343@ddn.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: {io-uring} Ensure fuse requests are set/read with locks | expand |
On Sat, Jan 25, 2025 at 9:44 AM Bernd Schubert <bschubert@ddn.com> wrote: > > This is set and read by different threads, we better use > _ONCE. > > 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, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c > index 9af5314f63d54cb1158e9372f4472759f5151ac3..257ee375e79a369c18088664781dd29d538078ac 100644 > --- a/fs/fuse/dev_uring.c > +++ b/fs/fuse/dev_uring.c > @@ -36,7 +36,7 @@ static void uring_cmd_set_ring_ent(struct io_uring_cmd *cmd, > struct fuse_uring_pdu *pdu = > io_uring_cmd_to_pdu(cmd, struct fuse_uring_pdu); > > - pdu->ent = ring_ent; > + WRITE_ONCE(pdu->ent, ring_ent); > } > > static struct fuse_ring_ent *uring_cmd_to_ring_ent(struct io_uring_cmd *cmd) > @@ -44,7 +44,7 @@ static struct fuse_ring_ent *uring_cmd_to_ring_ent(struct io_uring_cmd *cmd) > struct fuse_uring_pdu *pdu = > io_uring_cmd_to_pdu(cmd, struct fuse_uring_pdu); > > - return pdu->ent; > + return READ_ONCE(pdu->ent); > } Not an expert in this so there's a good chance i'm wrong here, but why do we need _ONCE? from what I understand, we only read pdu->ent when handling IO_URING_F_CANCEL or when preparing to send the queued request. It looks like it's always guaranteed that pdu->ent is set before any reads on pdu->ent for the case of sending a queued request, and pdu->ent gets read when making a request cancellable (which happens when we register the cmd and when we do a commit), but it seems like it would always be guaranteed that making the request cancellable must happen before any IO_URING_F_CANCELs can occur? Is there a race condition I'm missing where we need these to be _ONCE? Thanks, Joanne > > static void fuse_uring_flush_bg(struct fuse_ring_queue *queue) > > -- > 2.43.0 >
diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c index 9af5314f63d54cb1158e9372f4472759f5151ac3..257ee375e79a369c18088664781dd29d538078ac 100644 --- a/fs/fuse/dev_uring.c +++ b/fs/fuse/dev_uring.c @@ -36,7 +36,7 @@ static void uring_cmd_set_ring_ent(struct io_uring_cmd *cmd, struct fuse_uring_pdu *pdu = io_uring_cmd_to_pdu(cmd, struct fuse_uring_pdu); - pdu->ent = ring_ent; + WRITE_ONCE(pdu->ent, ring_ent); } static struct fuse_ring_ent *uring_cmd_to_ring_ent(struct io_uring_cmd *cmd) @@ -44,7 +44,7 @@ static struct fuse_ring_ent *uring_cmd_to_ring_ent(struct io_uring_cmd *cmd) struct fuse_uring_pdu *pdu = io_uring_cmd_to_pdu(cmd, struct fuse_uring_pdu); - return pdu->ent; + return READ_ONCE(pdu->ent); } static void fuse_uring_flush_bg(struct fuse_ring_queue *queue)
This is set and read by different threads, we better use _ONCE. 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, 2 insertions(+), 2 deletions(-)