Re: [PATCH v9 2/2] ublk: enable zoned storage support

[Andreas Hindborg (Samsung)]

Ming Lei <ming.lei@xxxxxxxxxx> writes:

> On Wed, Jul 19, 2023 at 05:26:11PM +0800, Ming Lei wrote:
>> On Fri, Jul 14, 2023 at 09:25:10AM +0200, Andreas Hindborg wrote:
>> > From: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>
>> > 
>> > Add zoned storage support to ublk: report_zones and operations:
>> >  - REQ_OP_ZONE_OPEN
>> >  - REQ_OP_ZONE_CLOSE
>> >  - REQ_OP_ZONE_FINISH
>> >  - REQ_OP_ZONE_RESET
>> >  - REQ_OP_ZONE_APPEND
>> > 
>> > The zone append feature uses the `addr` field of `struct ublksrv_io_cmd` to
>> > communicate ALBA back to the kernel. Therefore ublk must be used with the
>> > user copy feature (UBLK_F_USER_COPY) for zoned storage support to be
>> > available. Without this feature, ublk will not allow zoned storage support.
>> > 
>> > Signed-off-by: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>
>> > ---
>> 
>> ...
>> 
>> > +/*
>> > + * Construct a zone report. The report request is carried in `struct
>> > + * ublksrv_io_desc`. The `start_sector` field must be the first sector of a zone
>> > + * and shall indicate the first zone of the report. The `nr_sectors` shall
>> > + * indicate how many zones should be reported (divide by zone size to get number
>> > + * of zones in the report) and must be an integer multiple of the zone size. The
>> > + * report shall be delivered as a `struct blk_zone` array. To report fewer zones
>> > + * than requested, zero the last entry of the returned array.
>> > + */
>> > +#define		UBLK_IO_OP_REPORT_ZONES		18
>> 
>> Actually, I meant the following delta change in V8 comment, then the UAPI
>> looks more clean & readable wrt. reporting how many zones in UBLK_IO_OP_REPORT_ZONES
>> and reusing ublksrv_io_cmd->addr.
>> 
>> Otherwise, this patchset looks fine.
>> 
>> 
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index 5698f4575e05..454c852ed328 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -70,7 +70,7 @@ struct ublk_rq_data {
>>  	struct kref ref;
>>  	__u64 sector;
>>  	__u32 operation;
>> -	__u32 nr_sectors;
>> +	__u32 nr_zones;
>>  };
>>  
>>  struct ublk_uring_cmd_pdu {
>> @@ -335,7 +335,7 @@ static int ublk_report_zones(struct gendisk *disk, sector_t sector,
>>  		pdu = blk_mq_rq_to_pdu(req);
>>  		pdu->operation = UBLK_IO_OP_REPORT_ZONES;
>>  		pdu->sector = sector;
>> -		pdu->nr_sectors = zones_in_request * zone_size_sectors;
>> +		pdu->nr_zones = zones_in_request;
>>  
>>  		ret = blk_rq_map_kern(disk->queue, req, buffer, buffer_length,
>>  					GFP_KERNEL);
>> @@ -404,7 +404,7 @@ static blk_status_t ublk_setup_iod_zoned(struct ublk_queue *ubq,
>>  		switch (ublk_op) {
>>  		case UBLK_IO_OP_REPORT_ZONES:
>>  			iod->op_flags = ublk_op | ublk_req_build_flags(req);
>> -			iod->nr_sectors = pdu->nr_sectors;
>> +			iod->nr_zones = pdu->nr_zones;
>>  			iod->start_sector = pdu->sector;
>>  			return BLK_STS_OK;
>>  		default:
>> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
>> index 4d97eb0f7d13..602a788a650e 100644
>> --- a/include/uapi/linux/ublk_cmd.h
>> +++ b/include/uapi/linux/ublk_cmd.h
>> @@ -249,11 +249,13 @@ struct ublksrv_ctrl_dev_info {
>>  /*
>>   * Construct a zone report. The report request is carried in `struct
>>   * ublksrv_io_desc`. The `start_sector` field must be the first sector of a zone
>> - * and shall indicate the first zone of the report. The `nr_sectors` shall
>> - * indicate how many zones should be reported (divide by zone size to get number
>> - * of zones in the report) and must be an integer multiple of the zone size. The
>> - * report shall be delivered as a `struct blk_zone` array. To report fewer zones
>> - * than requested, zero the last entry of the returned array.
>> + * and shall indicate the first zone of the report. The `nr_zones` shall
>> + * indicate how many zones should be reported at most. The report shall be
>> + * delivered as a `struct blk_zone` array. To report fewer zones than
>> + * requested, zero the last entry of the returned array.
>> + *
>> + * So related definitions(blk_zone, blk_zone_cond, blk_zone_type, ...) in
>> + * include/uapi/linux/blkzoned.h are part of ublk UAPI.
>>   */
>>  #define		UBLK_IO_OP_REPORT_ZONES		18
>>  
>> @@ -276,7 +278,10 @@ struct ublksrv_io_desc {
>>  	/* op: bit 0-7, flags: bit 8-31 */
>>  	__u32		op_flags;
>>  
>> -	__u32		nr_sectors;
>> +	union {
>> +		__u32		nr_sectors;
>> +		__u32		nr_zones; /* for UBLK_IO_OP_REPORT_ZONES only */
>> +	};
>>  
>>  	/* start sector for this io */
>>  	__u64		start_sector;
>> @@ -308,6 +313,12 @@ struct ublksrv_io_cmd {
>>  	/*
>>  	 * userspace buffer address in ublksrv daemon process, valid for
>>  	 * FETCH* command only
>> +	 *
>> +	 * This field shouldn't be used if UBLK_F_USER_COPY is enabled,
>> +	 * because userspace deals with data copy by pread()/pwrite() over
>> +	 * /dev/ublkcN. But in case of UBLK_F_ZONED, 'addr' is re-used to
>> +	 * pass back the allocated LBA for UBLK_IO_OP_ZONE_APPEND which
>> +	 * actually depends on UBLK_F_USER_COPY
>>  	 */
>>  	__u64	addr;
>
> Or use union to cover zoned_append_lba, and we still need above
> document about UBLK_F_USER_COPY & UBLK_F_ZONED uses.
>
> 	union {
> 		__u64	addr;
> 		__u64	zoned_append_lba;
> 	}
>

Thanks, I'll add this to the next version.

BR Andreas