On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote: > > There is no a reason to set individual FR_ABORTED state > for every request, since fuse_abort_conn() aborts all > unlocked requests at once. FR_ABORTED bit just makes > fuse_copy_aborted() to end some of requests, which are > in the middle of fuse_dev_do_read() and fuse_dev_do_write(), > but this is not a big deal. These functions may abort > the requests themselves. > > The patch kills lock_request and unlock_request(), > and introduces generic fuse_copy_aborted() to use > instead of them. This allows next patches to kill > FR_ABORTED, FR_LOCKED and FR_PRIVATE, and simplify > fuse dev read/write function. Patch is no good: you assume that fuse_copy_args() called from fuse_dev_do_*() will finish in finite time. That's not the case if page fault on userspace buffer is hung (see e.g. Documentation/filesystems/fuse.txt #Tricky deadlock). You *are* right that this is a big wart in the current implementation, and would be really nice to get rid of. Certainly doable, just need to make sure all buffers stored in the request are properly refcounted and have lifetime bound to that of the request, i.e. all synchronous requests (not background) can be aborted asynchronously, the caller of the request can return in this case with an abort, yet the request processing (copy to/from userspace) can finish without encountering already freed memory. The fuse_simple_request() thing was a first step in this direction, so that one's pretty easy to deal with: copy arguments to a temporary buffer that is freed only when the request itself is freed... Requests which have pages need special attention. A ref is taken for those pages, so they may be OK, but in some cases they may not be. Thanks, Miklos
