| Message ID | 20250124-optimize-fuse-uring-req-timeouts-v1-2-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: > > This ensures that ent->cmd and ent->fuse_req are accessed in > fuse_uring_entry_teardown while holding the queue lock. > > Fixes: a4bdb3d786c0 ("fuse: enable fuse-over-io-uring") > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > --- > fs/fuse/dev_uring.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c > index 1834c1933d2bbab0342257fde4b030f06506c55d..87bb89994c311f435c370f78984be060fcb8036f 100644 > --- a/fs/fuse/dev_uring.c > +++ b/fs/fuse/dev_uring.c > @@ -315,14 +315,20 @@ static void fuse_uring_stop_fuse_req_end(struct fuse_ring_ent *ent) > */ > static void fuse_uring_entry_teardown(struct fuse_ring_ent *ent) > { > - struct fuse_ring_queue *queue = ent->queue; > - if (ent->cmd) { > - io_uring_cmd_done(ent->cmd, -ENOTCONN, 0, IO_URING_F_UNLOCKED); > - ent->cmd = NULL; > - } > + struct fuse_req *req; > + struct io_uring_cmd *cmd; > > - if (ent->fuse_req) > - fuse_uring_stop_fuse_req_end(ent); > + struct fuse_ring_queue *queue = ent->queue; > + > + spin_lock(&queue->lock); > + ent->fuse_req = NULL; > + > + req = ent->fuse_req; I think you meant here to switch these two lines? otherwise i think req will alwyas be null here. Thanks, Joanne > + if (req) > + list_del_init(&req->list); > + > + cmd = ent->cmd; > + ent->cmd = NULL; > > /* > * The entry must not be freed immediately, due to access of direct > @@ -330,10 +336,15 @@ static void fuse_uring_entry_teardown(struct fuse_ring_ent *ent) > * of race between daemon termination (which triggers IO_URING_F_CANCEL > * and accesses entries without checking the list state first > */ > - spin_lock(&queue->lock); > list_move(&ent->list, &queue->ent_released); > ent->state = FRRS_RELEASED; > spin_unlock(&queue->lock); > + > + if (cmd) > + io_uring_cmd_done(cmd, -ENOTCONN, 0, IO_URING_F_UNLOCKED); > + > + if (req) > + fuse_uring_stop_fuse_req_end(req); > } > > static void fuse_uring_stop_list_entries(struct list_head *head, > > -- > 2.43.0 >
On 1/25/25 01:37, Joanne Koong wrote: > On Fri, Jan 24, 2025 at 8:47 AM Bernd Schubert <bschubert@ddn.com> wrote: >> >> This ensures that ent->cmd and ent->fuse_req are accessed in >> fuse_uring_entry_teardown while holding the queue lock. >> >> Fixes: a4bdb3d786c0 ("fuse: enable fuse-over-io-uring") >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >> --- >> fs/fuse/dev_uring.c | 27 +++++++++++++++++++-------- >> 1 file changed, 19 insertions(+), 8 deletions(-) >> >> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c >> index 1834c1933d2bbab0342257fde4b030f06506c55d..87bb89994c311f435c370f78984be060fcb8036f 100644 >> --- a/fs/fuse/dev_uring.c >> +++ b/fs/fuse/dev_uring.c >> @@ -315,14 +315,20 @@ static void fuse_uring_stop_fuse_req_end(struct fuse_ring_ent *ent) >> */ >> static void fuse_uring_entry_teardown(struct fuse_ring_ent *ent) >> { >> - struct fuse_ring_queue *queue = ent->queue; >> - if (ent->cmd) { >> - io_uring_cmd_done(ent->cmd, -ENOTCONN, 0, IO_URING_F_UNLOCKED); >> - ent->cmd = NULL; >> - } >> + struct fuse_req *req; >> + struct io_uring_cmd *cmd; >> >> - if (ent->fuse_req) >> - fuse_uring_stop_fuse_req_end(ent); >> + struct fuse_ring_queue *queue = ent->queue; >> + >> + spin_lock(&queue->lock); >> + ent->fuse_req = NULL; >> + >> + req = ent->fuse_req; > > I think you meant here to switch these two lines? otherwise i think > req will alwyas be null here. > Ah yes, thanks for spotting it, it is for extra safety, so bypassed my basic testing. Thanks, Bernd
diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c index 1834c1933d2bbab0342257fde4b030f06506c55d..87bb89994c311f435c370f78984be060fcb8036f 100644 --- a/fs/fuse/dev_uring.c +++ b/fs/fuse/dev_uring.c @@ -315,14 +315,20 @@ static void fuse_uring_stop_fuse_req_end(struct fuse_ring_ent *ent) */ static void fuse_uring_entry_teardown(struct fuse_ring_ent *ent) { - struct fuse_ring_queue *queue = ent->queue; - if (ent->cmd) { - io_uring_cmd_done(ent->cmd, -ENOTCONN, 0, IO_URING_F_UNLOCKED); - ent->cmd = NULL; - } + struct fuse_req *req; + struct io_uring_cmd *cmd; - if (ent->fuse_req) - fuse_uring_stop_fuse_req_end(ent); + struct fuse_ring_queue *queue = ent->queue; + + spin_lock(&queue->lock); + ent->fuse_req = NULL; + + req = ent->fuse_req; + if (req) + list_del_init(&req->list); + + cmd = ent->cmd; + ent->cmd = NULL; /* * The entry must not be freed immediately, due to access of direct @@ -330,10 +336,15 @@ static void fuse_uring_entry_teardown(struct fuse_ring_ent *ent) * of race between daemon termination (which triggers IO_URING_F_CANCEL * and accesses entries without checking the list state first */ - spin_lock(&queue->lock); list_move(&ent->list, &queue->ent_released); ent->state = FRRS_RELEASED; spin_unlock(&queue->lock); + + if (cmd) + io_uring_cmd_done(cmd, -ENOTCONN, 0, IO_URING_F_UNLOCKED); + + if (req) + fuse_uring_stop_fuse_req_end(req); } static void fuse_uring_stop_list_entries(struct list_head *head,
This ensures that ent->cmd and ent->fuse_req are accessed in fuse_uring_entry_teardown while holding the queue lock. Fixes: a4bdb3d786c0 ("fuse: enable fuse-over-io-uring") Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- fs/fuse/dev_uring.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)