| Message ID | 20250123235251.1139078-1-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] fuse: optimize over-io-uring request expiration check | expand |
On 1/24/25 00:52, Joanne Koong wrote: > Currently, when checking whether a request has timed out, we check > fpq processing, but fuse-over-io-uring has one fpq per core and 256 > entries in the processing table. For systems where there are a > large number of cores, this may be too much overhead. > > Instead of checking the fpq processing list, check ent_w_req_queue, > ent_in_userspace, and ent_commit_queue. > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > --- > fs/fuse/dev.c | 2 +- > fs/fuse/dev_uring.c | 23 ++++++++++++++++++++--- > fs/fuse/fuse_dev_i.h | 1 - > 3 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index 3c03aac480a4..80a11ef4b69a 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -45,7 +45,7 @@ bool fuse_request_expired(struct fuse_conn *fc, struct list_head *list) > return time_is_before_jiffies(req->create_time + fc->timeout.req_timeout); > } > > -bool fuse_fpq_processing_expired(struct fuse_conn *fc, struct list_head *processing) > +static bool fuse_fpq_processing_expired(struct fuse_conn *fc, struct list_head *processing) > { > int i; > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c > index 5c9b5a5fb7f7..dfa6c5337bbf 100644 > --- a/fs/fuse/dev_uring.c > +++ b/fs/fuse/dev_uring.c > @@ -90,6 +90,7 @@ static void fuse_uring_req_end(struct fuse_ring_ent *ent, int error) > fuse_uring_flush_bg(queue); > spin_unlock(&fc->bg_lock); > } > + ent->fuse_req = NULL; > > spin_unlock(&queue->lock); > > @@ -97,8 +98,7 @@ static void fuse_uring_req_end(struct fuse_ring_ent *ent, int error) > req->out.h.error = error; > > clear_bit(FR_SENT, &req->flags); > - fuse_request_end(ent->fuse_req); > - ent->fuse_req = NULL; > + fuse_request_end(req); > } > Oh, this is actually a fix, it should be always set with the lock being held. > /* Abort all list queued request on the given ring queue */ > @@ -140,6 +140,21 @@ void fuse_uring_abort_end_requests(struct fuse_ring *ring) > } > } > > +static bool ent_list_request_expired(struct fuse_conn *fc, struct list_head *list) > +{ > + struct fuse_ring_ent *ent; > + struct fuse_req *req; > + > + list_for_each_entry(ent, list, list) { > + req = ent->fuse_req; > + if (req) > + return time_is_before_jiffies(req->create_time + > + fc->timeout.req_timeout); > + } > + > + return false; > +} Hmm, would only need to check head? Oh I see it, we need to use list_move_tail(). > + > bool fuse_uring_request_expired(struct fuse_conn *fc) > { > struct fuse_ring *ring = fc->ring; > @@ -157,7 +172,9 @@ bool fuse_uring_request_expired(struct fuse_conn *fc) > spin_lock(&queue->lock); > if (fuse_request_expired(fc, &queue->fuse_req_queue) || > fuse_request_expired(fc, &queue->fuse_req_bg_queue) || > - fuse_fpq_processing_expired(fc, queue->fpq.processing)) { > + ent_list_request_expired(fc, &queue->ent_w_req_queue) || > + ent_list_request_expired(fc, &queue->ent_in_userspace) || > + ent_list_request_expired(fc, &queue->ent_commit_queue)) { > spin_unlock(&queue->lock); > return true; > } > diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h > index 3c4ae4d52b6f..19c29c6000a7 100644 > --- a/fs/fuse/fuse_dev_i.h > +++ b/fs/fuse/fuse_dev_i.h > @@ -63,7 +63,6 @@ void fuse_dev_queue_forget(struct fuse_iqueue *fiq, > void fuse_dev_queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req); > > bool fuse_request_expired(struct fuse_conn *fc, struct list_head *list); > -bool fuse_fpq_processing_expired(struct fuse_conn *fc, struct list_head *processing); > > #endif >
Hi Joanne, On 1/24/25 12:30, Bernd Schubert wrote: > Hmm, would only need to check head? Oh I see it, we need to use > list_move_tail(). how about the attached updated patch, which uses list_first_entry_or_null()? It also changes from list_move() to list_move_tail() so that oldest entry is always on top. I didn't give it any testing, though. This is on top of the other fuse-uring updates I had sent out about an hour ago. Here is the corresponding branch https://github.com/bsbernd/linux/tree/optimize-fuse-uring-req-timeouts Thanks, Bernd
On Fri, Jan 24, 2025 at 10:22 AM Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > Hi Joanne, > > On 1/24/25 12:30, Bernd Schubert wrote: > > Hmm, would only need to check head? Oh I see it, we need to use > > list_move_tail(). > > > how about the attached updated patch, which uses > list_first_entry_or_null()? It also changes from list_move() > to list_move_tail() so that oldest entry is always on top. > I didn't give it any testing, though. Woah that's cool, I didn't know you could send attachments over the mailing list. Ah I didn't realize list_move doesn't already by default add to the tail of the list - thanks for catching that, yes those should be list_move_tail() then. In t he attached patch, I think we still need the original ent_list_request_expired() logic: static bool ent_list_request_expired(struct fuse_conn *fc, struct list_head *list) { struct fuse_ring_ent *ent; struct fuse_req *req; list_for_each_entry(ent, list, list) { req = ent->fuse_req; if (req) return time_is_before_jiffies(req->create_time + fc->timeout.req_timeout); } return false; } and we can't assume req is non-NULL. For entries that have been committed, their ->req is set to NULL but they are still on the ent_commit_queue. Thanks, Joanne > > This is on top of the other fuse-uring updates I had sent > out about an hour ago. > > > Here is the corresponding branch > https://github.com/bsbernd/linux/tree/optimize-fuse-uring-req-timeouts > > > Thanks, > Bernd
On 1/24/25 23:05, Joanne Koong wrote: > On Fri, Jan 24, 2025 at 10:22 AM Bernd Schubert > <bernd.schubert@fastmail.fm> wrote: >> >> Hi Joanne, >> >> On 1/24/25 12:30, Bernd Schubert wrote: >>> Hmm, would only need to check head? Oh I see it, we need to use >>> list_move_tail(). >> >> >> how about the attached updated patch, which uses >> list_first_entry_or_null()? It also changes from list_move() >> to list_move_tail() so that oldest entry is always on top. >> I didn't give it any testing, though. > > Woah that's cool, I didn't know you could send attachments over the > mailing list. > Ah I didn't realize list_move doesn't already by default add to the > tail of the list - thanks for catching that, yes those should be > list_move_tail() then. > > In t he attached patch, I think we still need the original > ent_list_request_expired() logic: > > static bool ent_list_request_expired(struct fuse_conn *fc, struct > list_head *list) > { > struct fuse_ring_ent *ent; > struct fuse_req *req; > > list_for_each_entry(ent, list, list) { > req = ent->fuse_req; > if (req) > return time_is_before_jiffies(req->create_time + > fc->timeout.req_timeout); > } > > return false; > } Could you explain why? That would be super expensive if lists have many entries? Due to fg and bg queues it might not be perfectly ordered, but that it actually also true for fuse_req_queue and also without io-uring. Server might process requests in different order, so ent_commit_queue might also not be perfectly sorted. But then I'm not even sure if we need to process that queue, as it has entries that are already processed - at best that would catch fuse client/kernel bugs. Though, if there is some kind if req processing issue, either everything times out, or the head will eventually get the older requests. So I don't understand why would need to go over all entries. > > and we can't assume req is non-NULL. For entries that have been > committed, their ->req is set to NULL but they are still on the > ent_commit_queue. Yeah sorry, right, though I wonder if we can just avoid checking that queue. Thanks, Bernd
On Fri, Jan 24, 2025 at 2:58 PM Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > > > On 1/24/25 23:05, Joanne Koong wrote: > > On Fri, Jan 24, 2025 at 10:22 AM Bernd Schubert > > <bernd.schubert@fastmail.fm> wrote: > >> > >> Hi Joanne, > >> > >> On 1/24/25 12:30, Bernd Schubert wrote: > >>> Hmm, would only need to check head? Oh I see it, we need to use > >>> list_move_tail(). > >> > >> > >> how about the attached updated patch, which uses > >> list_first_entry_or_null()? It also changes from list_move() > >> to list_move_tail() so that oldest entry is always on top. > >> I didn't give it any testing, though. > > > > Woah that's cool, I didn't know you could send attachments over the > > mailing list. > > Ah I didn't realize list_move doesn't already by default add to the > > tail of the list - thanks for catching that, yes those should be > > list_move_tail() then. > > > > In t he attached patch, I think we still need the original > > ent_list_request_expired() logic: > > > > static bool ent_list_request_expired(struct fuse_conn *fc, struct > > list_head *list) > > { > > struct fuse_ring_ent *ent; > > struct fuse_req *req; > > > > list_for_each_entry(ent, list, list) { > > req = ent->fuse_req; > > if (req) > > return time_is_before_jiffies(req->create_time + > > fc->timeout.req_timeout); > > } > > > > return false; > > } > > Could you explain why? That would be super expensive if lists > have many entries? Due to fg and bg queues it might not be > perfectly ordered, but that it actually also true for > fuse_req_queue and also without io-uring. Server might process > requests in different order, so ent_commit_queue might also not > be perfectly sorted. But then I'm not even sure if we need to > process that queue, as it has entries that are already processed - at > best that would catch fuse client/kernel bugs. > Though, if there is some kind if req processing issue, either > everything times out, or the head will eventually get the older > requests. So I don't understand why would need to go over all entries. > I don't think this in most cases goes over all entries. For ent_w_req_queue and ent_in_userspace queues, I think this is equivalent to just checking the head of the list since every entry on it will have a non-NULL request attached. But I think we need this "list_for_each_entry() { ... if (req) ... }" logic in order to properly handle the ent_commit_queue cases where an entry is on that queue but has a NULL request (eg the condition where fuse_uring_commit() has finished being called on the entry but a new request hasn't yet been attached to that entry). I think another option is if we move the fuse_uring_ent_avail() call from fuse_uring_next_fuse_req() to be in fuse_uring_req_end() within the scope of the queue->lock instead, then that ensures all entries on the committed queue have valid requests. > > > > and we can't assume req is non-NULL. For entries that have been sorry, this should have been "because we can't assume req is non-NULL", not "and". > > committed, their ->req is set to NULL but they are still on the > > ent_commit_queue. > > Yeah sorry, right, though I wonder if we can just avoid checking > that queue. > I like the idea of skipping the ent_commit_queue! We pretty much know the request is being responded to right now if it's being committed, so even if it has technically timed out, we can skip aborting it. Timeouts are kind of fuzzy right now anyways given the FUSE_TIMEOUT_TIMER_FREQ periodic check, so I don't see why this would be a problem. Thanks, Joanne > > Thanks, > Bernd
On Fri, Jan 24, 2025 at 3:31 PM Joanne Koong <joannelkoong@gmail.com> wrote: > > On Fri, Jan 24, 2025 at 2:58 PM Bernd Schubert > <bernd.schubert@fastmail.fm> wrote: > > > > > > > > On 1/24/25 23:05, Joanne Koong wrote: > > > On Fri, Jan 24, 2025 at 10:22 AM Bernd Schubert > > > <bernd.schubert@fastmail.fm> wrote: > > >> > > >> Hi Joanne, > > >> > > >> On 1/24/25 12:30, Bernd Schubert wrote: > > >>> Hmm, would only need to check head? Oh I see it, we need to use > > >>> list_move_tail(). > > >> > > >> > > >> how about the attached updated patch, which uses > > >> list_first_entry_or_null()? It also changes from list_move() > > >> to list_move_tail() so that oldest entry is always on top. > > >> I didn't give it any testing, though. I think this attached patch looks good, except with the "ent_list_request_expired(fc, &queue->ent_commit_queue))" line in it removed. Thanks, Joanne > > > > > > Woah that's cool, I didn't know you could send attachments over the > > > mailing list. > > > Ah I didn't realize list_move doesn't already by default add to the > > > tail of the list - thanks for catching that, yes those should be > > > list_move_tail() then. > > > > > > In t he attached patch, I think we still need the original > > > ent_list_request_expired() logic: > > > > > > static bool ent_list_request_expired(struct fuse_conn *fc, struct > > > list_head *list) > > > { > > > struct fuse_ring_ent *ent; > > > struct fuse_req *req; > > > > > > list_for_each_entry(ent, list, list) { > > > req = ent->fuse_req; > > > if (req) > > > return time_is_before_jiffies(req->create_time + > > > fc->timeout.req_timeout); > > > } > > > > > > return false; > > > } > > > > Could you explain why? That would be super expensive if lists > > have many entries? Due to fg and bg queues it might not be > > perfectly ordered, but that it actually also true for > > fuse_req_queue and also without io-uring. Server might process > > requests in different order, so ent_commit_queue might also not > > be perfectly sorted. But then I'm not even sure if we need to > > process that queue, as it has entries that are already processed - at > > best that would catch fuse client/kernel bugs. > > Though, if there is some kind if req processing issue, either > > everything times out, or the head will eventually get the older > > requests. So I don't understand why would need to go over all entries. > > > > I don't think this in most cases goes over all entries. For > ent_w_req_queue and ent_in_userspace queues, I think this is > equivalent to just checking the head of the list since every entry on > it will have a non-NULL request attached. But I think we need this > "list_for_each_entry() { ... if (req) ... }" logic in order to > properly handle the ent_commit_queue cases where an entry is on that > queue but has a NULL request (eg the condition where > fuse_uring_commit() has finished being called on the entry but a new > request hasn't yet been attached to that entry). > > I think another option is if we move the fuse_uring_ent_avail() call > from fuse_uring_next_fuse_req() to be in fuse_uring_req_end() within > the scope of the queue->lock instead, then that ensures all entries on > the committed queue have valid requests. > > > > > > > > and we can't assume req is non-NULL. For entries that have been > > sorry, this should have been "because we can't assume req is > non-NULL", not "and". > > > > committed, their ->req is set to NULL but they are still on the > > > ent_commit_queue. > > > > Yeah sorry, right, though I wonder if we can just avoid checking > > that queue. > > > > I like the idea of skipping the ent_commit_queue! We pretty much know > the request is being responded to right now if it's being committed, > so even if it has technically timed out, we can skip aborting it. > Timeouts are kind of fuzzy right now anyways given the > FUSE_TIMEOUT_TIMER_FREQ periodic check, so I don't see why this would > be a problem. > > > Thanks, > Joanne > > > > Thanks, > > Bernd
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 3c03aac480a4..80a11ef4b69a 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -45,7 +45,7 @@ bool fuse_request_expired(struct fuse_conn *fc, struct list_head *list) return time_is_before_jiffies(req->create_time + fc->timeout.req_timeout); } -bool fuse_fpq_processing_expired(struct fuse_conn *fc, struct list_head *processing) +static bool fuse_fpq_processing_expired(struct fuse_conn *fc, struct list_head *processing) { int i; diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c index 5c9b5a5fb7f7..dfa6c5337bbf 100644 --- a/fs/fuse/dev_uring.c +++ b/fs/fuse/dev_uring.c @@ -90,6 +90,7 @@ static void fuse_uring_req_end(struct fuse_ring_ent *ent, int error) fuse_uring_flush_bg(queue); spin_unlock(&fc->bg_lock); } + ent->fuse_req = NULL; spin_unlock(&queue->lock); @@ -97,8 +98,7 @@ static void fuse_uring_req_end(struct fuse_ring_ent *ent, int error) req->out.h.error = error; clear_bit(FR_SENT, &req->flags); - fuse_request_end(ent->fuse_req); - ent->fuse_req = NULL; + fuse_request_end(req); } /* Abort all list queued request on the given ring queue */ @@ -140,6 +140,21 @@ void fuse_uring_abort_end_requests(struct fuse_ring *ring) } } +static bool ent_list_request_expired(struct fuse_conn *fc, struct list_head *list) +{ + struct fuse_ring_ent *ent; + struct fuse_req *req; + + list_for_each_entry(ent, list, list) { + req = ent->fuse_req; + if (req) + return time_is_before_jiffies(req->create_time + + fc->timeout.req_timeout); + } + + return false; +} + bool fuse_uring_request_expired(struct fuse_conn *fc) { struct fuse_ring *ring = fc->ring; @@ -157,7 +172,9 @@ bool fuse_uring_request_expired(struct fuse_conn *fc) spin_lock(&queue->lock); if (fuse_request_expired(fc, &queue->fuse_req_queue) || fuse_request_expired(fc, &queue->fuse_req_bg_queue) || - fuse_fpq_processing_expired(fc, queue->fpq.processing)) { + ent_list_request_expired(fc, &queue->ent_w_req_queue) || + ent_list_request_expired(fc, &queue->ent_in_userspace) || + ent_list_request_expired(fc, &queue->ent_commit_queue)) { spin_unlock(&queue->lock); return true; } diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h index 3c4ae4d52b6f..19c29c6000a7 100644 --- a/fs/fuse/fuse_dev_i.h +++ b/fs/fuse/fuse_dev_i.h @@ -63,7 +63,6 @@ void fuse_dev_queue_forget(struct fuse_iqueue *fiq, void fuse_dev_queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req); bool fuse_request_expired(struct fuse_conn *fc, struct list_head *list); -bool fuse_fpq_processing_expired(struct fuse_conn *fc, struct list_head *processing); #endif
Currently, when checking whether a request has timed out, we check fpq processing, but fuse-over-io-uring has one fpq per core and 256 entries in the processing table. For systems where there are a large number of cores, this may be too much overhead. Instead of checking the fpq processing list, check ent_w_req_queue, ent_in_userspace, and ent_commit_queue. Signed-off-by: Joanne Koong <joannelkoong@gmail.com> --- fs/fuse/dev.c | 2 +- fs/fuse/dev_uring.c | 23 ++++++++++++++++++++--- fs/fuse/fuse_dev_i.h | 1 - 3 files changed, 21 insertions(+), 5 deletions(-)