On 7/18/25 19:50, Joanne Koong wrote:
> On Fri, Jul 18, 2025 at 9:37 AM Bernd Schubert <bernd@xxxxxxxxxxx> wrote:
>>
>> On 7/18/25 01:26, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>>>
>>> +/*
>>> + * Flush all pending requests and wait for them. Only call this function when
>>> + * it is no longer possible for other threads to add requests.
>>> + */
>>> +void fuse_flush_requests(struct fuse_conn *fc, unsigned long timeout)
>>
>> I wonder if this should have "abort" in its name. Because it is not a
>> simple flush attempt, but also sets fc->blocked and fc->max_background.
>>
>>> +{
>>> + unsigned long deadline;
>>> +
>>> + spin_lock(&fc->lock);
>>> + if (!fc->connected) {
>>> + spin_unlock(&fc->lock);
>>> + return;
>>> + }
>>> +
>>> + /* Push all the background requests to the queue. */
>>> + spin_lock(&fc->bg_lock);
>>> + fc->blocked = 0;
>>> + fc->max_background = UINT_MAX;
>>> + flush_bg_queue(fc);
>>> + spin_unlock(&fc->bg_lock);
>>> + spin_unlock(&fc->lock);
>>> +
>>> + /*
>>> + * Wait 30s for all the events to complete or abort. Touch the
>>> + * watchdog once per second so that we don't trip the hangcheck timer
>>> + * while waiting for the fuse server.
>>> + */
>>> + deadline = jiffies + timeout;
>>> + smp_mb();
>>> + while (fc->connected &&
>>> + (!timeout || time_before(jiffies, deadline)) &&
>>> + wait_event_timeout(fc->blocked_waitq,
>>> + !fc->connected || atomic_read(&fc->num_waiting) == 0,
>>> + HZ) == 0)
>>> + touch_softlockup_watchdog();
>>> +}
>>> +
>>> /*
>>> * Abort all requests.
>>> *
>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>> index 9572bdef49eecc..1734c263da3a77 100644
>>> --- a/fs/fuse/inode.c
>>> +++ b/fs/fuse/inode.c
>>> @@ -2047,6 +2047,7 @@ void fuse_conn_destroy(struct fuse_mount *fm)
>>> {
>>> struct fuse_conn *fc = fm->fc;
>>>
>>> + fuse_flush_requests(fc, 30 * HZ);
>>
>> I think fc->connected should be set to 0, to avoid that new requests can
>> be allocated.
>
> fuse_abort_conn() logic is gated on "if (fc->connected)" so I think
> fc->connected can only get set to 0 within fuse_abort_conn()
Hmm yeah, I wonder if we should allow multiple values in there. Like
fuse_abort_conn sets UINT64_MAX and checks that and other functions
could set values in between? We could add another variable, but given
that it is used on every request allocation might be better to avoid too
many conditions.
Thanks,
Bernd
