On Fri, Jan 24, 2025 at 2:58 PM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> On 1/24/25 23:05, Joanne Koong wrote:
> > On Fri, Jan 24, 2025 at 10:22 AM Bernd Schubert
> > <bernd.schubert@xxxxxxxxxxx> 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
