On 2020/06/25 21:22, Javier González wrote:
> From: Javier González <javier.gonz@xxxxxxxxxxx>
>
> Add flag to allow selecting all zones on a single zone management
> operation
>
> Signed-off-by: Javier González <javier.gonz@xxxxxxxxxxx>
> Signed-off-by: SelvaKumar S <selvakuma.s1@xxxxxxxxxxx>
> Signed-off-by: Kanchan Joshi <joshi.k@xxxxxxxxxxx>
> Signed-off-by: Nitesh Shetty <nj.shetty@xxxxxxxxxxx>
> ---
> block/blk-zoned.c | 3 +++
> include/linux/blk_types.h | 3 ++-
> include/uapi/linux/blkzoned.h | 9 +++++++++
> 3 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index e87c60004dc5..29194388a1bb 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -420,6 +420,9 @@ int blkdev_zone_mgmt_ioctl(struct block_device *bdev, fmode_t mode,
> return -ENOTTY;
> }
>
> + if (zmgmt.flags & BLK_ZONE_SELECT_ALL)
> + op |= REQ_ZONE_ALL;
> +
> return blkdev_zone_mgmt(bdev, op, zmgmt.sector, zmgmt.nr_sectors,
> GFP_KERNEL);
> }
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index ccb895f911b1..16b57fb2b99c 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -351,6 +351,7 @@ enum req_flag_bits {
> * work item to avoid such priority inversions.
> */
> __REQ_CGROUP_PUNT,
> + __REQ_ZONE_ALL, /* apply zone operation to all zones */
>
> /* command specific flags for REQ_OP_WRITE_ZEROES: */
> __REQ_NOUNMAP, /* do not free blocks when zeroing */
> @@ -378,7 +379,7 @@ enum req_flag_bits {
> #define REQ_BACKGROUND (1ULL << __REQ_BACKGROUND)
> #define REQ_NOWAIT (1ULL << __REQ_NOWAIT)
> #define REQ_CGROUP_PUNT (1ULL << __REQ_CGROUP_PUNT)
> -
> +#define REQ_ZONE_ALL (1ULL << __REQ_ZONE_ALL)
> #define REQ_NOUNMAP (1ULL << __REQ_NOUNMAP)
> #define REQ_HIPRI (1ULL << __REQ_HIPRI)
>
> diff --git a/include/uapi/linux/blkzoned.h b/include/uapi/linux/blkzoned.h
> index 07b5fde21d9f..a8c89fe58f97 100644
> --- a/include/uapi/linux/blkzoned.h
> +++ b/include/uapi/linux/blkzoned.h
> @@ -157,6 +157,15 @@ enum blk_zone_action {
> BLK_ZONE_MGMT_RESET = 0x4,
> };
>
> +/**
> + * enum blk_zone_mgmt_flags - Flags for blk_zone_mgmt
> + *
> + * BLK_ZONE_SELECT_ALL: Select all zones for current zone action
> + */
> +enum blk_zone_mgmt_flags {
> + BLK_ZONE_SELECT_ALL = 1 << 0,
> +};
> +
> /**
> * struct blk_zone_mgmt - Extended zoned management
> *
>
NACK.
Details:
1) REQ_OP_ZONE_RESET together with REQ_ZONE_ALL is the same as
REQ_OP_ZONE_RESET_ALL, isn't it ? You are duplicating a functionality that
already exists.
2) The patch introduces REQ_ZONE_ALL at the block layer only without defining
how it ties into SCSI and NVMe driver use of it. Is REQ_ZONE_ALL indicating that
the zone management commands are to be executed with the ALL bit set ? If yes,
that will break device-mapper. See the special code for handling
REQ_OP_ZONE_RESET_ALL. That code is in place for a reason: the target block
device may not be an entire physical device. In that case, applying a zone
management command to all zones of the physical drive is wrong.
3) REQ_ZONE_ALL seems completely equivalent to specifying a sector range of [0
.. drive capacity]. So what is the point ? The current interface handles that.
That is how we chose between REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL right now.
4) Without any in-kernel user, I do not see the point. And for applications, I
do not see any good use case for doing open all, close all, offline all or
finish all. If you have any such good use case, please elaborate.
--
Damien Le Moal
Western Digital Research
