| Message ID | 20230918041106.2134250-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | io_uring/ublk: exit notifier support | expand |
On 9/17/23 10:10 PM, Ming Lei wrote: > Hello, > > In do_exit(), io_uring needs to wait pending requests. > > ublk is one uring_cmd driver, and its usage is a bit special by submitting > command for waiting incoming block IO request in advance, so if there > isn't any IO request coming, the command can't be completed. So far ublk > driver has to bind its queue with one ublk daemon server, meantime > starts one monitor work to check if this daemon is live periodically. > This way requires ublk queue to be bound one single daemon pthread, and > not flexible, meantime the monitor work is run in 3rd context, and the > implementation is a bit tricky. > > The 1st 3 patches adds io_uring task exit notifier, and the other > patches converts ublk into this exit notifier, and the implementation > becomes more robust & readable, meantime it becomes easier to relax > the ublk queue/daemon limit in future, such as not require to bind > ublk queue with single daemon. The normal approach for this is to ensure that each request is cancelable, which we need for other things too (like actual cancel support). Why can't we just do the same for ublk?
On Mon, Sep 18, 2023 at 06:54:33AM -0600, Jens Axboe wrote: > On 9/17/23 10:10 PM, Ming Lei wrote: > > Hello, > > > > In do_exit(), io_uring needs to wait pending requests. > > > > ublk is one uring_cmd driver, and its usage is a bit special by submitting > > command for waiting incoming block IO request in advance, so if there > > isn't any IO request coming, the command can't be completed. So far ublk > > driver has to bind its queue with one ublk daemon server, meantime > > starts one monitor work to check if this daemon is live periodically. > > This way requires ublk queue to be bound one single daemon pthread, and > > not flexible, meantime the monitor work is run in 3rd context, and the > > implementation is a bit tricky. > > > > The 1st 3 patches adds io_uring task exit notifier, and the other > > patches converts ublk into this exit notifier, and the implementation > > becomes more robust & readable, meantime it becomes easier to relax > > the ublk queue/daemon limit in future, such as not require to bind > > ublk queue with single daemon. > > The normal approach for this is to ensure that each request is > cancelable, which we need for other things too (like actual cancel > support) Why can't we just do the same for ublk? I guess you meant IORING_OP_ASYNC_CANCEL, which needs userspace to submit this command, but here the userspace(ublk server) may be just panic or killed, and there isn't chance to send IORING_OP_ASYNC_CANCEL. And driver doesn't have any knowledge if the io_uring ctx or io task is exiting, so can't complete issued commands, then hang in io_uring_cancel_generic() when the io task/ctx is exiting. Thanks, Ming
On 9/18/23 7:24 AM, Ming Lei wrote: > On Mon, Sep 18, 2023 at 06:54:33AM -0600, Jens Axboe wrote: >> On 9/17/23 10:10 PM, Ming Lei wrote: >>> Hello, >>> >>> In do_exit(), io_uring needs to wait pending requests. >>> >>> ublk is one uring_cmd driver, and its usage is a bit special by submitting >>> command for waiting incoming block IO request in advance, so if there >>> isn't any IO request coming, the command can't be completed. So far ublk >>> driver has to bind its queue with one ublk daemon server, meantime >>> starts one monitor work to check if this daemon is live periodically. >>> This way requires ublk queue to be bound one single daemon pthread, and >>> not flexible, meantime the monitor work is run in 3rd context, and the >>> implementation is a bit tricky. >>> >>> The 1st 3 patches adds io_uring task exit notifier, and the other >>> patches converts ublk into this exit notifier, and the implementation >>> becomes more robust & readable, meantime it becomes easier to relax >>> the ublk queue/daemon limit in future, such as not require to bind >>> ublk queue with single daemon. >> >> The normal approach for this is to ensure that each request is >> cancelable, which we need for other things too (like actual cancel >> support) Why can't we just do the same for ublk? > > I guess you meant IORING_OP_ASYNC_CANCEL, which needs userspace to > submit this command, but here the userspace(ublk server) may be just panic > or killed, and there isn't chance to send IORING_OP_ASYNC_CANCEL. Either that, or cancel done because of task exit. > And driver doesn't have any knowledge if the io_uring ctx or io task > is exiting, so can't complete issued commands, then hang in > io_uring_cancel_generic() when the io task/ctx is exiting. If you hooked into the normal cancel paths, you very much would get notified when the task is exiting. That's how the other cancelations work, eg if a task has pending poll requests and exits, they get canceled and reaped.
On Mon, Sep 18, 2023 at 08:15:07AM -0600, Jens Axboe wrote: > On 9/18/23 7:24 AM, Ming Lei wrote: > > On Mon, Sep 18, 2023 at 06:54:33AM -0600, Jens Axboe wrote: > >> On 9/17/23 10:10 PM, Ming Lei wrote: > >>> Hello, > >>> > >>> In do_exit(), io_uring needs to wait pending requests. > >>> > >>> ublk is one uring_cmd driver, and its usage is a bit special by submitting > >>> command for waiting incoming block IO request in advance, so if there > >>> isn't any IO request coming, the command can't be completed. So far ublk > >>> driver has to bind its queue with one ublk daemon server, meantime > >>> starts one monitor work to check if this daemon is live periodically. > >>> This way requires ublk queue to be bound one single daemon pthread, and > >>> not flexible, meantime the monitor work is run in 3rd context, and the > >>> implementation is a bit tricky. > >>> > >>> The 1st 3 patches adds io_uring task exit notifier, and the other > >>> patches converts ublk into this exit notifier, and the implementation > >>> becomes more robust & readable, meantime it becomes easier to relax > >>> the ublk queue/daemon limit in future, such as not require to bind > >>> ublk queue with single daemon. > >> > >> The normal approach for this is to ensure that each request is > >> cancelable, which we need for other things too (like actual cancel > >> support) Why can't we just do the same for ublk? > > > > I guess you meant IORING_OP_ASYNC_CANCEL, which needs userspace to > > submit this command, but here the userspace(ublk server) may be just panic > > or killed, and there isn't chance to send IORING_OP_ASYNC_CANCEL. > > Either that, or cancel done because of task exit. > > > And driver doesn't have any knowledge if the io_uring ctx or io task > > is exiting, so can't complete issued commands, then hang in > > io_uring_cancel_generic() when the io task/ctx is exiting. > > If you hooked into the normal cancel paths, you very much would get > notified when the task is exiting. That's how the other cancelations > work, eg if a task has pending poll requests and exits, they get > canceled and reaped. Ok, got the idea, thanks for the point! Turns out it is cancelable uring_cmd, and I will try to work towards this direction, and has got something in mind about the implementation. Thanks, Ming
On 9/18/23 10:02 AM, Ming Lei wrote: > On Mon, Sep 18, 2023 at 08:15:07AM -0600, Jens Axboe wrote: >> On 9/18/23 7:24 AM, Ming Lei wrote: >>> On Mon, Sep 18, 2023 at 06:54:33AM -0600, Jens Axboe wrote: >>>> On 9/17/23 10:10 PM, Ming Lei wrote: >>>>> Hello, >>>>> >>>>> In do_exit(), io_uring needs to wait pending requests. >>>>> >>>>> ublk is one uring_cmd driver, and its usage is a bit special by submitting >>>>> command for waiting incoming block IO request in advance, so if there >>>>> isn't any IO request coming, the command can't be completed. So far ublk >>>>> driver has to bind its queue with one ublk daemon server, meantime >>>>> starts one monitor work to check if this daemon is live periodically. >>>>> This way requires ublk queue to be bound one single daemon pthread, and >>>>> not flexible, meantime the monitor work is run in 3rd context, and the >>>>> implementation is a bit tricky. >>>>> >>>>> The 1st 3 patches adds io_uring task exit notifier, and the other >>>>> patches converts ublk into this exit notifier, and the implementation >>>>> becomes more robust & readable, meantime it becomes easier to relax >>>>> the ublk queue/daemon limit in future, such as not require to bind >>>>> ublk queue with single daemon. >>>> >>>> The normal approach for this is to ensure that each request is >>>> cancelable, which we need for other things too (like actual cancel >>>> support) Why can't we just do the same for ublk? >>> >>> I guess you meant IORING_OP_ASYNC_CANCEL, which needs userspace to >>> submit this command, but here the userspace(ublk server) may be just panic >>> or killed, and there isn't chance to send IORING_OP_ASYNC_CANCEL. >> >> Either that, or cancel done because of task exit. >> >>> And driver doesn't have any knowledge if the io_uring ctx or io task >>> is exiting, so can't complete issued commands, then hang in >>> io_uring_cancel_generic() when the io task/ctx is exiting. >> >> If you hooked into the normal cancel paths, you very much would get >> notified when the task is exiting. That's how the other cancelations >> work, eg if a task has pending poll requests and exits, they get >> canceled and reaped. > > Ok, got the idea, thanks for the point! > > Turns out it is cancelable uring_cmd, and I will try to work towards > this direction, and has got something in mind about the implementation. Perfect, thanks Ming!
Hello, In do_exit(), io_uring needs to wait pending requests. ublk is one uring_cmd driver, and its usage is a bit special by submitting command for waiting incoming block IO request in advance, so if there isn't any IO request coming, the command can't be completed. So far ublk driver has to bind its queue with one ublk daemon server, meantime starts one monitor work to check if this daemon is live periodically. This way requires ublk queue to be bound one single daemon pthread, and not flexible, meantime the monitor work is run in 3rd context, and the implementation is a bit tricky. The 1st 3 patches adds io_uring task exit notifier, and the other patches converts ublk into this exit notifier, and the implementation becomes more robust & readable, meantime it becomes easier to relax the ublk queue/daemon limit in future, such as not require to bind ublk queue with single daemon. Ming Lei (10): io_uring: allocate ctx id and build map between id and ctx io_uring: pass io_uring_ctx->id to uring_cmd io_uring: support io_uring notifier for uring_cmd ublk: don't get ublk device reference in ublk_abort_queue() ublk: make sure ublk uring cmd handling is done in submitter task context ublk: make sure that uring cmd aiming at same queue won't cross io_uring contexts ublk: rename mm_lock as lock ublk: quiesce request queue when aborting queue ublk: replace monitor work with uring_cmd exit notifier ublk: simplify aborting request drivers/block/ublk_drv.c | 216 +++++++++++++++++++-------------- include/linux/io_uring.h | 27 ++++- include/linux/io_uring_types.h | 3 + io_uring/io_uring.c | 57 +++++++++ io_uring/io_uring.h | 4 + io_uring/uring_cmd.c | 13 ++ 6 files changed, 230 insertions(+), 90 deletions(-)