On Fri, Jan 24, 2025 at 3:31 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> 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.
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
