[PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[Ming Lei]

There isn't any reason to not support REQ_OP_ZONE_RESET_ALL given everything
is actually handled in userspace, not mention it is pretty easy to support
RESET_ALL.

So enable REQ_OP_ZONE_RESET_ALL and let userspace handle it.

Verified by 'tools/zbc_reset_zone -all /dev/ublkb0' in libzbc[1] with
libublk-rs based ublk-zoned target prototype[2], follows command line
for creating ublk-zoned:

	cargo run --example zoned -- add -1 1024	# add $dev_id $DEV_SIZE

[1] https://github.com/westerndigitalcorporation/libzbc
[2] https://github.com/ming1/libublk-rs/tree/zoned.v2

Cc: Niklas Cassel <Niklas.Cassel@wdc.com>
Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Andreas Hindborg <a.hindborg@samsung.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
V2:
	- update comment as reported by Niklas

 drivers/block/ublk_drv.c      | 7 +++++--
 include/uapi/linux/ublk_cmd.h | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index b60394fe7be6..3650ef209344 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -251,6 +251,7 @@ static int ublk_dev_param_zoned_apply(struct ublk_device *ub)
 	const struct ublk_param_zoned *p = &ub->params.zoned;
 
 	disk_set_zoned(ub->ub_disk, BLK_ZONED_HM);
+	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, ub->ub_disk->queue);
 	blk_queue_required_elevator_features(ub->ub_disk->queue,
 					     ELEVATOR_F_ZBD_SEQ_WRITE);
 	disk_set_max_active_zones(ub->ub_disk, p->max_active_zones);
@@ -393,6 +394,9 @@ static blk_status_t ublk_setup_iod_zoned(struct ublk_queue *ubq,
 	case REQ_OP_ZONE_APPEND:
 		ublk_op = UBLK_IO_OP_ZONE_APPEND;
 		break;
+	case REQ_OP_ZONE_RESET_ALL:
+		ublk_op = UBLK_IO_OP_ZONE_RESET_ALL;
+		break;
 	case REQ_OP_DRV_IN:
 		ublk_op = pdu->operation;
 		switch (ublk_op) {
@@ -404,9 +408,8 @@ static blk_status_t ublk_setup_iod_zoned(struct ublk_queue *ubq,
 		default:
 			return BLK_STS_IOERR;
 		}
-	case REQ_OP_ZONE_RESET_ALL:
 	case REQ_OP_DRV_OUT:
-		/* We do not support reset_all and drv_out */
+		/* We do not support drv_out */
 		return BLK_STS_NOTSUPP;
 	default:
 		return BLK_STS_IOERR;
diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
index 2685e53e4752..b9cfc5c96268 100644
--- a/include/uapi/linux/ublk_cmd.h
+++ b/include/uapi/linux/ublk_cmd.h
@@ -245,6 +245,7 @@ struct ublksrv_ctrl_dev_info {
 #define		UBLK_IO_OP_ZONE_CLOSE		11
 #define		UBLK_IO_OP_ZONE_FINISH		12
 #define		UBLK_IO_OP_ZONE_APPEND		13
+#define		UBLK_IO_OP_ZONE_RESET_ALL	14
 #define		UBLK_IO_OP_ZONE_RESET		15
 /*
  * Construct a zone report. The report request is carried in `struct
-- 
2.40.1

Re: [PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[Niklas Cassel]

On Thu, Aug 10, 2023 at 08:43:26PM +0800, Ming Lei wrote:
> There isn't any reason to not support REQ_OP_ZONE_RESET_ALL given everything
> is actually handled in userspace, not mention it is pretty easy to support
> RESET_ALL.
> 
> So enable REQ_OP_ZONE_RESET_ALL and let userspace handle it.
> 
> Verified by 'tools/zbc_reset_zone -all /dev/ublkb0' in libzbc[1] with
> libublk-rs based ublk-zoned target prototype[2], follows command line
> for creating ublk-zoned:
> 
> 	cargo run --example zoned -- add -1 1024	# add $dev_id $DEV_SIZE
> 
> [1] https://github.com/westerndigitalcorporation/libzbc
> [2] https://github.com/ming1/libublk-rs/tree/zoned.v2
> 
> Cc: Niklas Cassel <Niklas.Cassel@wdc.com>
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> V2:
> 	- update comment as reported by Niklas
> 
>  drivers/block/ublk_drv.c      | 7 +++++--
>  include/uapi/linux/ublk_cmd.h | 1 +
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index b60394fe7be6..3650ef209344 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -251,6 +251,7 @@ static int ublk_dev_param_zoned_apply(struct ublk_device *ub)
>  	const struct ublk_param_zoned *p = &ub->params.zoned;
>  
>  	disk_set_zoned(ub->ub_disk, BLK_ZONED_HM);
> +	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, ub->ub_disk->queue);
>  	blk_queue_required_elevator_features(ub->ub_disk->queue,
>  					     ELEVATOR_F_ZBD_SEQ_WRITE);
>  	disk_set_max_active_zones(ub->ub_disk, p->max_active_zones);
> @@ -393,6 +394,9 @@ static blk_status_t ublk_setup_iod_zoned(struct ublk_queue *ubq,
>  	case REQ_OP_ZONE_APPEND:
>  		ublk_op = UBLK_IO_OP_ZONE_APPEND;
>  		break;
> +	case REQ_OP_ZONE_RESET_ALL:
> +		ublk_op = UBLK_IO_OP_ZONE_RESET_ALL;
> +		break;
>  	case REQ_OP_DRV_IN:
>  		ublk_op = pdu->operation;
>  		switch (ublk_op) {
> @@ -404,9 +408,8 @@ static blk_status_t ublk_setup_iod_zoned(struct ublk_queue *ubq,
>  		default:
>  			return BLK_STS_IOERR;
>  		}
> -	case REQ_OP_ZONE_RESET_ALL:
>  	case REQ_OP_DRV_OUT:
> -		/* We do not support reset_all and drv_out */
> +		/* We do not support drv_out */
>  		return BLK_STS_NOTSUPP;
>  	default:
>  		return BLK_STS_IOERR;
> diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> index 2685e53e4752..b9cfc5c96268 100644
> --- a/include/uapi/linux/ublk_cmd.h
> +++ b/include/uapi/linux/ublk_cmd.h
> @@ -245,6 +245,7 @@ struct ublksrv_ctrl_dev_info {
>  #define		UBLK_IO_OP_ZONE_CLOSE		11
>  #define		UBLK_IO_OP_ZONE_FINISH		12
>  #define		UBLK_IO_OP_ZONE_APPEND		13
> +#define		UBLK_IO_OP_ZONE_RESET_ALL	14

For some reason, it seems like the UBLK_IO_OP_ZONE_* values
are identical to the REQ_OP_ZONE_* values in enum req_op:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/blk_types.h?h=v6.5-rc5#n371

I don't see any obvious advantage of keeping them the same,
but if you want to keep this pattern, then perhaps you want
to define UBLK_IO_OP_ZONE_RESET_ALL to 17.


Kind regards,
Niklas

Re: [PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[Ming Lei]

On Thu, Aug 10, 2023 at 01:10:30PM +0000, Niklas Cassel wrote:
> On Thu, Aug 10, 2023 at 08:43:26PM +0800, Ming Lei wrote:
> > There isn't any reason to not support REQ_OP_ZONE_RESET_ALL given everything
> > is actually handled in userspace, not mention it is pretty easy to support
> > RESET_ALL.
> > 
> > So enable REQ_OP_ZONE_RESET_ALL and let userspace handle it.
> > 
> > Verified by 'tools/zbc_reset_zone -all /dev/ublkb0' in libzbc[1] with
> > libublk-rs based ublk-zoned target prototype[2], follows command line
> > for creating ublk-zoned:
> > 
> > 	cargo run --example zoned -- add -1 1024	# add $dev_id $DEV_SIZE
> > 
> > [1] https://github.com/westerndigitalcorporation/libzbc
> > [2] https://github.com/ming1/libublk-rs/tree/zoned.v2
> > 
> > Cc: Niklas Cassel <Niklas.Cassel@wdc.com>
> > Cc: Damien Le Moal <dlemoal@kernel.org>
> > Cc: Andreas Hindborg <a.hindborg@samsung.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > V2:
> > 	- update comment as reported by Niklas
> > 
> >  drivers/block/ublk_drv.c      | 7 +++++--
> >  include/uapi/linux/ublk_cmd.h | 1 +
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index b60394fe7be6..3650ef209344 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -251,6 +251,7 @@ static int ublk_dev_param_zoned_apply(struct ublk_device *ub)
> >  	const struct ublk_param_zoned *p = &ub->params.zoned;
> >  
> >  	disk_set_zoned(ub->ub_disk, BLK_ZONED_HM);
> > +	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, ub->ub_disk->queue);
> >  	blk_queue_required_elevator_features(ub->ub_disk->queue,
> >  					     ELEVATOR_F_ZBD_SEQ_WRITE);
> >  	disk_set_max_active_zones(ub->ub_disk, p->max_active_zones);
> > @@ -393,6 +394,9 @@ static blk_status_t ublk_setup_iod_zoned(struct ublk_queue *ubq,
> >  	case REQ_OP_ZONE_APPEND:
> >  		ublk_op = UBLK_IO_OP_ZONE_APPEND;
> >  		break;
> > +	case REQ_OP_ZONE_RESET_ALL:
> > +		ublk_op = UBLK_IO_OP_ZONE_RESET_ALL;
> > +		break;
> >  	case REQ_OP_DRV_IN:
> >  		ublk_op = pdu->operation;
> >  		switch (ublk_op) {
> > @@ -404,9 +408,8 @@ static blk_status_t ublk_setup_iod_zoned(struct ublk_queue *ubq,
> >  		default:
> >  			return BLK_STS_IOERR;
> >  		}
> > -	case REQ_OP_ZONE_RESET_ALL:
> >  	case REQ_OP_DRV_OUT:
> > -		/* We do not support reset_all and drv_out */
> > +		/* We do not support drv_out */
> >  		return BLK_STS_NOTSUPP;
> >  	default:
> >  		return BLK_STS_IOERR;
> > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> > index 2685e53e4752..b9cfc5c96268 100644
> > --- a/include/uapi/linux/ublk_cmd.h
> > +++ b/include/uapi/linux/ublk_cmd.h
> > @@ -245,6 +245,7 @@ struct ublksrv_ctrl_dev_info {
> >  #define		UBLK_IO_OP_ZONE_CLOSE		11
> >  #define		UBLK_IO_OP_ZONE_FINISH		12
> >  #define		UBLK_IO_OP_ZONE_APPEND		13
> > +#define		UBLK_IO_OP_ZONE_RESET_ALL	14
> 
> For some reason, it seems like the UBLK_IO_OP_ZONE_* values
> are identical to the REQ_OP_ZONE_* values in enum req_op:

Yeah.

I think that is zoned interface abstraction, which should be
generic enough to use linux's definition for ublk's UAPI in 1:1.

The same mapping can be found in virtio-blk zoned spec.

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/blk_types.h?h=v6.5-rc5#n371
> 
> I don't see any obvious advantage of keeping them the same,

Not sure I follow your idea, and can you share your exact suggestion?

UBLK_IO_OP_ZONE_* is part of ublk UAPI, but REQ_OP_ZONE_* is just kernel
internal definition which may be changed time by time, so we can't use
REQ_OP_ZONE_* directly.

Here you can think of UBLK_IO_OP_ZONE_* as interface between driver and
hardware, so UBLK_IO_OP_ZONE_* has to be defined independently.

> but if you want to keep this pattern, then perhaps you want
> to define UBLK_IO_OP_ZONE_RESET_ALL to 17.

Why do you think that 17 is better than 14?

I'd rather use 14 to fill the hole, meantime the two ZONE_RESET OPs
can be kept together.

Thanks,
Ming

Re: [PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[Niklas Cassel]

On Thu, Aug 10, 2023 at 10:00:14PM +0800, Ming Lei wrote:
> On Thu, Aug 10, 2023 at 01:10:30PM +0000, Niklas Cassel wrote:
> > On Thu, Aug 10, 2023 at 08:43:26PM +0800, Ming Lei wrote:

(snip)
 
> UBLK_IO_OP_ZONE_* is part of ublk UAPI, but REQ_OP_ZONE_* is just kernel
> internal definition which may be changed time by time, so we can't use
> REQ_OP_ZONE_* directly.
> 
> Here you can think of UBLK_IO_OP_ZONE_* as interface between driver and
> hardware, so UBLK_IO_OP_ZONE_* has to be defined independently.
> 
> > but if you want to keep this pattern, then perhaps you want
> > to define UBLK_IO_OP_ZONE_RESET_ALL to 17.
> 
> Why do you think that 17 is better than 14?

I never said that it was better :)
I even said: "I don't see any obvious advantage of keeping them the same" :)

Just that it would follow the existing pattern of keeping
UBLK_IO_OP_ZONE_* in sync with REQ_OP_ZONE_*.


> 
> I'd rather use 14 to fill the hole, meantime the two ZONE_RESET OPs
> can be kept together.

Ok, but then, considering that UBLK_IO_OP_ZONE_* is not part of any official
kernel release, and that the highest UBLK_IO_OP is currently defined as 5:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/ublk_cmd.h?h=v6.5-rc5#n237

why not define:
+#define		UBLK_IO_OP_ZONE_OPEN		6
+#define		UBLK_IO_OP_ZONE_CLOSE		7
+#define		UBLK_IO_OP_ZONE_FINISH		8
+#define		UBLK_IO_OP_ZONE_APPEND		9
+#define		UBLK_IO_OP_ZONE_RESET		10
+#define		UBLK_IO_OP_ZONE_RESET_ALL	11

instead of, like it currently is in linux-block/for-next (this patch included):

+#define		UBLK_IO_OP_ZONE_OPEN		10
+#define		UBLK_IO_OP_ZONE_CLOSE		11
+#define		UBLK_IO_OP_ZONE_FINISH		12
+#define		UBLK_IO_OP_ZONE_APPEND		13
+#define		UBLK_IO_OP_ZONE_RESET_ALL	14
+#define		UBLK_IO_OP_ZONE_RESET		15

Because, even after this patch, you would still have a hole between
UBLK_IO_OP_ value 5 and 10.


Kind regards,
Niklas

Re: [PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[Ming Lei]

On Thu, Aug 10, 2023 at 02:22:50PM +0000, Niklas Cassel wrote:
> On Thu, Aug 10, 2023 at 10:00:14PM +0800, Ming Lei wrote:
> > On Thu, Aug 10, 2023 at 01:10:30PM +0000, Niklas Cassel wrote:
> > > On Thu, Aug 10, 2023 at 08:43:26PM +0800, Ming Lei wrote:
> 
> (snip)
>  
> > UBLK_IO_OP_ZONE_* is part of ublk UAPI, but REQ_OP_ZONE_* is just kernel
> > internal definition which may be changed time by time, so we can't use
> > REQ_OP_ZONE_* directly.
> > 
> > Here you can think of UBLK_IO_OP_ZONE_* as interface between driver and
> > hardware, so UBLK_IO_OP_ZONE_* has to be defined independently.
> > 
> > > but if you want to keep this pattern, then perhaps you want
> > > to define UBLK_IO_OP_ZONE_RESET_ALL to 17.
> > 
> > Why do you think that 17 is better than 14?
> 
> I never said that it was better :)
> I even said: "I don't see any obvious advantage of keeping them the same" :)

OK, maybe I misunderstood your point.

> 
> Just that it would follow the existing pattern of keeping
> UBLK_IO_OP_ZONE_* in sync with REQ_OP_ZONE_*.

No matter 14 or 17, it does sync with REQ_OP_ZONE_*, even though it
isn't necessary to do so.

Then your all comment on this patch should be addressed, right?

> 
> 
> > 
> > I'd rather use 14 to fill the hole, meantime the two ZONE_RESET OPs
> > can be kept together.
> 
> Ok, but then, considering that UBLK_IO_OP_ZONE_* is not part of any official
> kernel release, and that the highest UBLK_IO_OP is currently defined as 5:

I don't think it is necessary, see below.

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/ublk_cmd.h?h=v6.5-rc5#n237
> 
> why not define:
> +#define		UBLK_IO_OP_ZONE_OPEN		6
> +#define		UBLK_IO_OP_ZONE_CLOSE		7
> +#define		UBLK_IO_OP_ZONE_FINISH		8
> +#define		UBLK_IO_OP_ZONE_APPEND		9
> +#define		UBLK_IO_OP_ZONE_RESET		10
> +#define		UBLK_IO_OP_ZONE_RESET_ALL	11
> 
> instead of, like it currently is in linux-block/for-next (this patch included):
> 
> +#define		UBLK_IO_OP_ZONE_OPEN		10
> +#define		UBLK_IO_OP_ZONE_CLOSE		11
> +#define		UBLK_IO_OP_ZONE_FINISH		12
> +#define		UBLK_IO_OP_ZONE_APPEND		13
> +#define		UBLK_IO_OP_ZONE_RESET_ALL	14
> +#define		UBLK_IO_OP_ZONE_RESET		15
> 
> Because, even after this patch, you would still have a hole between
> UBLK_IO_OP_ value 5 and 10.

(5, 10) can be reserved for some more generic normal IOs, maybe new kind
of READ/WRITE, or whatever.

We have 256 OP available, which is big enough for future extend, and OP
code can be used from any offset, but still more readable to group them
according to their category. And it isn't big deal to use which number
for which command, what matters is that we can extend, and keep
compatible.


Thanks,
Ming

Re: [PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[Niklas Cassel]

On Thu, Aug 10, 2023 at 10:47:14PM +0800, Ming Lei wrote:
> On Thu, Aug 10, 2023 at 02:22:50PM +0000, Niklas Cassel wrote:
> > On Thu, Aug 10, 2023 at 10:00:14PM +0800, Ming Lei wrote:
> > > On Thu, Aug 10, 2023 at 01:10:30PM +0000, Niklas Cassel wrote:
> > > > On Thu, Aug 10, 2023 at 08:43:26PM +0800, Ming Lei wrote:
> 
> We have 256 OP available, which is big enough for future extend, and OP
> code can be used from any offset, but still more readable to group them
> according to their category. And it isn't big deal to use which number
> for which command, what matters is that we can extend, and keep
> compatible.

I see your point, cheers!


Kind regards,
Niklas

Re: [PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[Ming Lei]

On Thu, Aug 10, 2023 at 08:43:26PM +0800, Ming Lei wrote:
> There isn't any reason to not support REQ_OP_ZONE_RESET_ALL given everything
> is actually handled in userspace, not mention it is pretty easy to support
> RESET_ALL.
> 
> So enable REQ_OP_ZONE_RESET_ALL and let userspace handle it.
> 
> Verified by 'tools/zbc_reset_zone -all /dev/ublkb0' in libzbc[1] with
> libublk-rs based ublk-zoned target prototype[2], follows command line
> for creating ublk-zoned:
> 
> 	cargo run --example zoned -- add -1 1024	# add $dev_id $DEV_SIZE
> 
> [1] https://github.com/westerndigitalcorporation/libzbc
> [2] https://github.com/ming1/libublk-rs/tree/zoned.v2
> 
> Cc: Niklas Cassel <Niklas.Cassel@wdc.com>
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Andreas Hindborg <a.hindborg@samsung.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> V2:
> 	- update comment as reported by Niklas

Hello Jens,

Can you merge this one since V2 addresses Niklas's concern.

Thanks,
Ming

Re: [PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[Ming Lei]

On Mon, Aug 14, 2023 at 10:01:25AM +0800, Ming Lei wrote:
> On Thu, Aug 10, 2023 at 08:43:26PM +0800, Ming Lei wrote:
> > There isn't any reason to not support REQ_OP_ZONE_RESET_ALL given everything
> > is actually handled in userspace, not mention it is pretty easy to support
> > RESET_ALL.
> > 
> > So enable REQ_OP_ZONE_RESET_ALL and let userspace handle it.
> > 
> > Verified by 'tools/zbc_reset_zone -all /dev/ublkb0' in libzbc[1] with
> > libublk-rs based ublk-zoned target prototype[2], follows command line
> > for creating ublk-zoned:
> > 
> > 	cargo run --example zoned -- add -1 1024	# add $dev_id $DEV_SIZE
> > 
> > [1] https://github.com/westerndigitalcorporation/libzbc
> > [2] https://github.com/ming1/libublk-rs/tree/zoned.v2
> > 
> > Cc: Niklas Cassel <Niklas.Cassel@wdc.com>
> > Cc: Damien Le Moal <dlemoal@kernel.org>
> > Cc: Andreas Hindborg <a.hindborg@samsung.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > V2:
> > 	- update comment as reported by Niklas
> 
> Hello Jens,
> 
> Can you merge this one since V2 addresses Niklas's concern.

Ping...

Thanks,
Ming

Re: [PATCH V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL

[Jens Axboe]

On Thu, 10 Aug 2023 20:43:26 +0800, Ming Lei wrote:
> There isn't any reason to not support REQ_OP_ZONE_RESET_ALL given everything
> is actually handled in userspace, not mention it is pretty easy to support
> RESET_ALL.
> 
> So enable REQ_OP_ZONE_RESET_ALL and let userspace handle it.
> 
> Verified by 'tools/zbc_reset_zone -all /dev/ublkb0' in libzbc[1] with
> libublk-rs based ublk-zoned target prototype[2], follows command line
> for creating ublk-zoned:
> 
> [...]

Applied, thanks!

[1/1] ublk: zoned: support REQ_OP_ZONE_RESET_ALL
      commit: 851e06297f20bbd85c93bbf09469f2150d1db218

Best regards,
-- 
Jens Axboe