Re: [PATCH] fuse: add optional kernel-enforced timeout for fuse requests

[Bernd Schubert <bs_lists@aakef.fastmail.fm>]

On 7/18/24 07:24, Joanne Koong wrote:
> On Wed, Jul 17, 2024 at 3:23 PM Bernd Schubert
> <bernd.schubert@fastmail.fm> wrote:
>>
>> Hi Joanne,
>>
>> On 7/17/24 23:34, Joanne Koong wrote:
>>> There are situations where fuse servers can become unresponsive or take
>>> too long to reply to a request. Currently there is no upper bound on
>>> how long a request may take, which may be frustrating to users who get
>>> stuck waiting for a request to complete.
>>>
>>> This commit adds a daemon timeout option (in seconds) for fuse requests.
>>> If the timeout elapses before the request is replied to, the request will
>>> fail with -ETIME.
>>>
>>> There are 3 possibilities for a request that times out:
>>> a) The request times out before the request has been sent to userspace
>>> b) The request times out after the request has been sent to userspace
>>> and before it receives a reply from the server
>>> c) The request times out after the request has been sent to userspace
>>> and the server replies while the kernel is timing out the request
>>>
>>> Proper synchronization must be added to ensure that the request is
>>> handled correctly in all of these cases. To this effect, there is a new
>>> FR_PROCESSING bit added to the request flags, which is set atomically by
>>> either the timeout handler (see fuse_request_timeout()) which is invoked
>>> after the request timeout elapses or set by the request reply handler
>>> (see dev_do_write()), whichever gets there first.
>>>
>>> If the reply handler and the timeout handler are executing simultaneously
>>> and the reply handler sets FR_PROCESSING before the timeout handler, then
>>> the request is re-queued onto the waitqueue and the kernel will process the
>>> reply as though the timeout did not elapse. If the timeout handler sets
>>> FR_PROCESSING before the reply handler, then the request will fail with
>>> -ETIME and the request will be cleaned up.
>>>
>>> Proper acquires on the request reference must be added to ensure that the
>>> timeout handler does not drop the last refcount on the request while the
>>> reply handler (dev_do_write()) or forwarder handler (dev_do_read()) is
>>> still accessing the request. (By "forwarder handler", this is the handler
>>> that forwards the request to userspace).
>>>
>>> Currently, this is the lifecycle of the request refcount:
>>>
>>> Request is created:
>>> fuse_simple_request -> allocates request, sets refcount to 1
>>>   __fuse_request_send -> acquires refcount
>>>     queues request and waits for reply...
>>> fuse_simple_request -> drops refcount
>>>
>>> Request is freed:
>>> fuse_dev_do_write
>>>   fuse_request_end -> drops refcount on request
>>>
>>> The timeout handler drops the refcount on the request so that the
>>> request is properly cleaned up if a reply is never received. Because of
>>> this, both the forwarder handler and the reply handler must acquire a refcount
>>> on the request while it accesses the request, and the refcount must be
>>> acquired while the lock of the list the request is on is held.
>>>
>>> There is a potential race if the request is being forwarded to
>>> userspace while the timeout handler is executing (eg FR_PENDING has
>>> already been cleared but dev_do_read() hasn't finished executing). This
>>> is a problem because this would free the request but the request has not
>>> been removed from the fpq list it's on. To prevent this, dev_do_read()
>>> must check FR_PROCESSING at the end of its logic and remove the request
>>> from the fpq list if the timeout occurred.
>>>
>>> There is also the case where the connection may be aborted or the
>>> device may be released while the timeout handler is running. To protect
>>> against an extra refcount drop on the request, the timeout handler
>>> checks the connected state of the list and lets the abort handler drop the
>>> last reference if the abort is running simultaneously. Similarly, the
>>> timeout handler also needs to check if the req->out.h.error is set to
>>> -ESTALE, which indicates that the device release is cleaning up the
>>> request. In both these cases, the timeout handler will return without
>>> dropping the refcount.
>>>
>>> Please also note that background requests are not applicable for timeouts
>>> since they are asynchronous.
>>
>>
>> This and that thread here actually make me wonder if this is the right
>> approach
>>
>> https://lore.kernel.org/lkml/20240613040147.329220-1-haifeng.xu@shopee.com/T/
>>
>>
>> In  th3 thread above a request got interrupted, but fuse-server still
>> does not manage stop it. From my point of view, interrupting a request
>> suggests to add a rather short kernel lifetime for it. With that one
> 
> Hi Bernd,
> 
> I believe this solution fixes the problem outlined in that thread
> (namely, that the process gets stuck waiting for a reply). If the
> request is interrupted before it times out, the kernel will wait with
> a timeout again on the request (timeout would start over, but the
> request will still eventually sooner or later time out). I'm not sure
> I agree that we want to cancel the request altogether if it's
> interrupted. For example, if the user uses the user-defined signal
> SIGUSR1, it can be unexpected and arbitrary behavior for the request
> to be aborted by the kernel. I also don't think this can be consistent
> for what the fuse server will see since some requests may have already
> been forwarded to userspace when the request is aborted and some
> requests may not have.
> 
> I think if we were to enforce that the request should be aborted when
> it's interrupted regardless of whether a timeout is specified or not,
> then we should do it similarly to how the timeout handler logic
> handles it in this patch,rather than the implementation in the thread
> linked above (namely, that the request should be explicitly cleaned up
> immediately instead of when the interrupt request sends a reply); I
> don't believe the implementation in the link handles the case where
> for example the fuse server is in a deadlock and does not reply to the
> interrupt request. Also, as I understand it, it is optional for
> servers to reply or not to the interrupt request.

Hi Joanne,

yeah, the solution in the link above is definitely not ideal and I think
a timout based solution would be better. But I think your patch wouldn't
work either right now, unless server side sets a request timeout.
Btw, I would rename the variable 'daemon_timeout' to somethink like
req_timeout.

> 
>> either needs to wake up in intervals and check if request timeout got
>> exceeded or it needs to be an async kernel thread. I think that async
>> thread would also allow to give a timeout to background requests.
> 
> in my opinion, background requests do not need timeouts. As I
> understand it, background requests are used only for direct i/o async
> read/writes, writing back dirty pages,and readahead requests generated
> by the kernel. I don't think fuse servers would have a need for timing
> out background requests.

There is another discussion here, where timeouts are a possible although
ugly solution to avoid page copies

https://lore.kernel.org/linux-kernel/233a9fdf-13ea-488b-a593-5566fc9f5d92@fastmail.fm/T/


That is the bg writeback code path.

> 
>>
>> Or we add an async timeout to bg and interupted requests additionally?
> 
> The interrupted request will already have a timeout on it since it
> waits with a timeout again for the reply after it's interrupted.

If daemon side configures timeouts. And interrupted requests might want
to have a different timeout. I will check when I'm back if we can update
your patch a bit for that.

Your patch hooks in quite nicely and basically without overhead into fg
(sync) requests. Timing out bg requests will have a bit overhead (unless
I miss something), so maybe we need two solutions here. And if we want
to go that route at all, to avoid these extra fuse page copies.


> 
>>
>>
>> (I only basically reviewed, can't do carefully right now - on vacation
>> and as I just noticed, on a laptop that gives me electric shocks when I
>> connect it to power.)
> 
> No worries, thanks for your comments and hope you have a great
> vacation (without getting shocked :))!

Thank you! For now I'm not connecting power, 3h of battery left :)


Thanks,
Bernd