Hi Sashiko,
Thanks for the review. Both Medium findings are valid; v3 fixes them
and adds the missing Fixes tag. The v3 patch is a separate thread.
#1 (-ENOSPC can bypass aborting)
Agreed. virtio_fs_enqueue_req() drops fsvq->lock before returning
-ENOSPC, and virtio_fs_send_req() queues the request in a second
critical section. Teardown can complete in between and leave the
request stranded on queued_reqs, which fuse_chan_abort() does not walk.
v3 re-checks connected/aborting in that second critical section, and in
the dispatch worker's -ENOSPC requeue path. A request now either joins
queued_reqs before aborting is set (and is counted in in_flight), or is
rejected with -ENOTCONN and ended through end_reqs. After aborting is
set, no request can newly enter queued_reqs or the virtqueue, except
FUSE_DESTROY.
I instrumented the window: enqueue returns -ENOSPC for a FUSE_STATFS,
send_req() waits before re-checking, and conn_destroy() sets aborting
and drains in that window. Three runs:
VF_INV: q=requests.0 in_flight=0 queued=0 connected=1 aborting=1
VF_ENOSPC: rejected
VF_ENOSPC: STATFS returned -107
VF_INV: q=requests.0 in_flight=0 queued=0 connected=0 aborting=1
umount rc=0
No stranded request, no warning.
#2 (dispatch worker may_block self-deadlock)
Agreed as well. Rejecting requests earlier means a DAX release can be
ended by virtio_fs_request_dispatch_work(); its end callback issues a
synchronous removemapping request, which is rejected and needs the same
worker.
v3 offloads endings with req->args->may_block to a separate work item
that only calls fuse_request_end(). virtio_fs_request_complete()
cannot be reused: it also consumes argbuf and drops in_flight, while a
rejected request has neither.
I verified the path with a synthetic callback: a background STATFS with
may_block=true and an end callback that issues a synchronous STATFS.
VF_DEADLOCK: queue outer
VF_DEADLOCK: offload may_block end
VF_DEADLOCK: outer end error=-107
VF_DEADLOCK: inner returned -107
VF_DEADLOCK: outer end done
umount rc=0
The old inline fuse_request_end() would stop at "outer end error" and
wait for itself.
#3 (missing Fixes)
Added:
Fixes: a62a8ef9d97d ("virtio-fs: add virtiofs filesystem")
Cc: [email protected]
Thanks,
Qihang
