| Message ID | 20230810124326.321472-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] ublk: zoned: support REQ_OP_ZONE_RESET_ALL | expand |
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
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
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
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
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
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
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
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,
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
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(-)