Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 18:08:18 +0800
From:   Ming Lei <ming.lei@redhat.com>
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, io-uring@vger.kernel.org,
        ZiyangZhang <ZiyangZhang@linux.alibaba.com>,
        Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>,
        Oleg Nesterov <oleg@redhat.com>, ming.lei@redhat.com
Subject: Re: [PATCH V4 2/2] ublk_drv: add UBLK_IO_REFETCH_REQ for supporting
 to build as module
Message-ID: <Ys1IEiIs2Xlp5iAk@T590>
References: <20220711022024.217163-1-ming.lei@redhat.com>
 <20220711022024.217163-3-ming.lei@redhat.com>
 <87lesze7o3.fsf@collabora.com>
 <YszdfgTbmHWveFjW@T590>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YszdfgTbmHWveFjW@T590>
Precedence: bulk

On Tue, Jul 12, 2022 at 10:33:34AM +0800, Ming Lei wrote:
> Hi Gabriel,
> 
> On Mon, Jul 11, 2022 at 04:06:04PM -0400, Gabriel Krisman Bertazi wrote:
> > Ming Lei <ming.lei@redhat.com> writes:
> > 
> > > Add UBLK_IO_REFETCH_REQ command to fetch the incoming io request in
> > > ubq daemon context, so we can avoid to call task_work_add(), then
> > > it is fine to build ublk driver as module.
> > >
> > > In this way, iops is affected a bit, but just by ~5% on ublk/null,
> > > given io_uring provides pretty good batching issuing & completing.
> > >
> > > One thing to be careful is race between ->queue_rq() and handling
> > > abort, which is avoided by quiescing queue when aborting queue.
> > > Except for that, handling abort becomes much easier with
> > > UBLK_IO_REFETCH_REQ since aborting handler is strictly exclusive with
> > > anything done in ubq daemon kernel context.
> > 
> > Hi Ming,
> > 
> > FWIW, I'm not very fond this change.  It adds complexity to the kernel
> > driver and to the userspace server implementation, who now have to deal
> 
> IMO, this way just adds dozens line of code, no much complexity. The only
> complexity in ublk driver should be in aborting code, which is actually
> originated from concurrent aborting work and running task work which may be
> run after task is exiting. But any storage driver's aborting/error
> handling code is complicated.
> 
> Using REFETCH_REQ actually becomes much easier for handling abort which is
> run exclusively with any code running in ubq daemon context, but with
> performance cost.
> 
> > with different interface semantics just because the driver was built-in
> > or built as a module.  I don't think the tristate support warrants such
> > complexity.  I was hoping we might get away with exporting that symbol
> > or adding a built-in ubd-specific wrapper that can be exported and
> > invokes task_work_add.
> 
> If task_work_add can be exported, that would be very great.

Another choice is to use io_uring_cmd_complete_in_task which is actually
exported, now we can build ublk_drv as module by using io_uring_cmd_complete_in_task
without needing one new command.

Thanks,
Ming