Re: [PATCH V4 2/2] ublk_drv: add UBLK_IO_REFETCH_REQ for supporting to build as module

[Ziyang Zhang]

On 2022/7/12 04:06, Gabriel Krisman Bertazi wrote:
> Ming Lei <ming.lei@xxxxxxxxxx> 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
> 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.
> 
> Either way, Alibaba seems to consider this feature useful, and if that
> is the case, we can just not use it on our side.

Our app handles IOs itself with network(RPC) and internal memory pool
so UBLK_IO_REFETCH_REQ
(actually I think it is like NEED_GET_DATA in the earlist version :) )
is helpful to us because we can assign data buffer address AFTER the app
gets one IO requests(WRITE, with data size) and we avoid PRE-allocating buffers.

Besides, adding UBLK_IO_REFETCH_REQ is helpful to build ublk driver as module
It seems like kernel developers do not want a built-in driver. :)

Maybe your app is different from ours(you may not need to handle IOs by yourelf).

Thanks， 
Ziyang Zhang


> 
> That said, the patch looks good to me, just a minor comment inline.
> 
> Thanks,
> 
>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
>> ---
>>  drivers/block/Kconfig         |   2 +-
>>  drivers/block/ublk_drv.c      | 121 ++++++++++++++++++++++++++--------
>>  include/uapi/linux/ublk_cmd.h |  17 +++++
>>  3 files changed, 113 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
>> index d218089cdbec..2ba77fd960c2 100644
>> --- a/drivers/block/Kconfig
>> +++ b/drivers/block/Kconfig
>> @@ -409,7 +409,7 @@ config BLK_DEV_RBD
>>  	  If unsure, say N.
>>  
>>  config BLK_DEV_UBLK
>> -	bool "Userspace block driver"
>> +	tristate "Userspace block driver"
>>  	select IO_URING
>>  	help
>>            io uring based userspace block driver.
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index 0076418e6fad..98482f8d1a77 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -92,6 +92,7 @@ struct ublk_queue {
>>  	int q_id;
>>  	int q_depth;
>>  
>> +	unsigned long flags;
>>  	struct task_struct	*ubq_daemon;
>>  	char *io_cmd_buf;
>>  
>> @@ -141,6 +142,15 @@ struct ublk_device {
>>  	struct work_struct	stop_work;
>>  };
>>  
>> +#define ublk_use_task_work(ubq)						\
>> +({                                                                      \
>> +	bool ret = false;						\
>> +	if (IS_BUILTIN(CONFIG_BLK_DEV_UBLK) &&                          \
>> +			!((ubq)->flags & UBLK_F_NEED_REFETCH))		\
>> +		ret = true;						\
>> +	ret;								\
>> +})
>> +
> 
> This should be an inline function, IMO.
> 
>