On Wed, Jul 17, 2024 at 3:23 PM Bernd Schubert <bernd.schubert@xxxxxxxxxxx> 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@xxxxxxxxxx/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. > 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. > > 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. > > > (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 :))! Thanks, Joanne > > > Thanks, > Bernd > > >
