[1/4] block: Add zone flags to queue zone prop.

Commit Message

Javier González July 2, 2020, 6:54 a.m. UTC

From: Javier González <[e-mail]>

As the zoned block device will have to deal with features that are
optional for the backend device, add a flag field to inform the block
layer about supported features. This builds on top of
blk_zone_report_flags and extendes to the zone report code.

Signed-off-by: Javier González <[e-mail]>
Signed-off-by: SelvaKumar S <[e-mail]>
Signed-off-by: Kanchan Joshi <[e-mail]>
Signed-off-by: Nitesh Shetty <[e-mail]>
---
 block/blk-zoned.c              | 3 ++-
 drivers/block/null_blk_zoned.c | 2 ++
 drivers/nvme/host/zns.c        | 1 +
 drivers/scsi/sd.c              | 2 ++
 include/linux/blkdev.h         | 3 +++
 5 files changed, 10 insertions(+), 1 deletion(-)

Comments

Damien Le Moal July 2, 2020, 7:54 a.m. UTC | #1

On 2020/07/02 15:55, Javier González wrote:
> From: Javier González <[e-mail]>
> 
> As the zoned block device will have to deal with features that are
> optional for the backend device, add a flag field to inform the block
> layer about supported features. This builds on top of
> blk_zone_report_flags and extendes to the zone report code.
> 
> Signed-off-by: Javier González <[e-mail]>
> Signed-off-by: SelvaKumar S <[e-mail]>
> Signed-off-by: Kanchan Joshi <[e-mail]>
> Signed-off-by: Nitesh Shetty <[e-mail]>
> ---
>  block/blk-zoned.c              | 3 ++-
>  drivers/block/null_blk_zoned.c | 2 ++
>  drivers/nvme/host/zns.c        | 1 +
>  drivers/scsi/sd.c              | 2 ++
>  include/linux/blkdev.h         | 3 +++
>  5 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 81152a260354..0f156e96e48f 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -312,7 +312,8 @@ int blkdev_report_zones_ioctl(struct block_device *bdev, fmode_t mode,
>  		return ret;
>  
>  	rep.nr_zones = ret;
> -	rep.flags = BLK_ZONE_REP_CAPACITY;
> +	rep.flags = q->zone_flags;

zone_flags seem to be a fairly generic flags field while rep.flags is only about
the reported descriptors structure. So you may want to define a
BLK_ZONE_REP_FLAGS_MASK and have:

	rep.flags = q->zone_flags & BLK_ZONE_REP_FLAGS_MASK;

to avoid flags meaningless for the user being set.

In any case, since *all* zoned block devices now report capacity, I do not
really see the point to add BLK_ZONE_REP_FLAGS_MASK to q->zone_flags, especially
considering that you set the flag for all zoned device types, including scsi
which does not have zone capacity. This makes q->zone_flags rather confusing
instead of clearly defining the device features as you mentioned in the commit
message.

I think it may be better to just drop this patch, and if needed, introduce the
zone_flags field where it may be needed (e.g. OFFLINE zone ioctl support).

> +
>  	if (copy_to_user(argp, &rep, sizeof(struct blk_zone_report)))
>  		return -EFAULT;
>  	return 0;
> diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
> index b05832eb21b2..957c2103f240 100644
> --- a/drivers/block/null_blk_zoned.c
> +++ b/drivers/block/null_blk_zoned.c
> @@ -78,6 +78,8 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
>  	}
>  
>  	q->limits.zoned = BLK_ZONED_HM;
> +	q->zone_flags = BLK_ZONE_REP_CAPACITY;
> +
>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>  	blk_queue_required_elevator_features(q, ELEVATOR_F_ZBD_SEQ_WRITE);
>  
> diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
> index 0642d3c54e8f..888264261ba3 100644
> --- a/drivers/nvme/host/zns.c
> +++ b/drivers/nvme/host/zns.c
> @@ -81,6 +81,7 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
>  	}
>  
>  	q->limits.zoned = BLK_ZONED_HM;
> +	q->zone_flags = BLK_ZONE_REP_CAPACITY;
>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>  free_data:
>  	kfree(id);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d90fefffe31b..b9c920bace28 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2967,6 +2967,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>  	if (sdkp->device->type == TYPE_ZBC) {
>  		/* Host-managed */
>  		q->limits.zoned = BLK_ZONED_HM;
> +		q->zone_flags = BLK_ZONE_REP_CAPACITY;
>  	} else {
>  		sdkp->zoned = (buffer[8] >> 4) & 3;
>  		if (sdkp->zoned == 1 && !disk_has_partitions(sdkp->disk)) {
> @@ -2983,6 +2984,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>  					  "Drive-managed SMR disk\n");
>  		}
>  	}
> +
>  	if (blk_queue_is_zoned(q) && sdkp->first_scan)
>  		sd_printk(KERN_NOTICE, sdkp, "Host-%s zoned block device\n",
>  		      q->limits.zoned == BLK_ZONED_HM ? "managed" : "aware");
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 8fd900998b4e..3f2e3425fa53 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -512,12 +512,15 @@ struct request_queue {
>  	 * Stacking drivers (device mappers) may or may not initialize
>  	 * these fields.
>  	 *
> +	 * Flags represent features as described by blk_zone_report_flags in blkzoned.h
> +	 *
>  	 * Reads of this information must be protected with blk_queue_enter() /
>  	 * blk_queue_exit(). Modifying this information is only allowed while
>  	 * no requests are being processed. See also blk_mq_freeze_queue() and
>  	 * blk_mq_unfreeze_queue().
>  	 */
>  	unsigned int		nr_zones;
> +	unsigned int		zone_flags;
>  	unsigned long		*conv_zones_bitmap;
>  	unsigned long		*seq_zones_wlock;
>  #endif /* CONFIG_BLK_DEV_ZONED */
> 

And you are missing device-mapper support. DM target devices have a request
queue that would need to set the zone_flags too.

Javier González July 2, 2020, 8:34 a.m. UTC | #2

On 02.07.2020 07:54, Damien Le Moal wrote:
>On 2020/07/02 15:55, Javier González wrote:
>> From: Javier González <[e-mail]>
>>
>> As the zoned block device will have to deal with features that are
>> optional for the backend device, add a flag field to inform the block
>> layer about supported features. This builds on top of
>> blk_zone_report_flags and extendes to the zone report code.
>>
>> Signed-off-by: Javier González <[e-mail]>
>> Signed-off-by: SelvaKumar S <[e-mail]>
>> Signed-off-by: Kanchan Joshi <[e-mail]>
>> Signed-off-by: Nitesh Shetty <[e-mail]>
>> ---
>>  block/blk-zoned.c              | 3 ++-
>>  drivers/block/null_blk_zoned.c | 2 ++
>>  drivers/nvme/host/zns.c        | 1 +
>>  drivers/scsi/sd.c              | 2 ++
>>  include/linux/blkdev.h         | 3 +++
>>  5 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>> index 81152a260354..0f156e96e48f 100644
>> --- a/block/blk-zoned.c
>> +++ b/block/blk-zoned.c
>> @@ -312,7 +312,8 @@ int blkdev_report_zones_ioctl(struct block_device *bdev, fmode_t mode,
>>  		return ret;
>>
>>  	rep.nr_zones = ret;
>> -	rep.flags = BLK_ZONE_REP_CAPACITY;
>> +	rep.flags = q->zone_flags;
>
>zone_flags seem to be a fairly generic flags field while rep.flags is only about
>the reported descriptors structure. So you may want to define a
>BLK_ZONE_REP_FLAGS_MASK and have:
>
>	rep.flags = q->zone_flags & BLK_ZONE_REP_FLAGS_MASK;
>
>to avoid flags meaningless for the user being set.
>
>In any case, since *all* zoned block devices now report capacity, I do not
>really see the point to add BLK_ZONE_REP_FLAGS_MASK to q->zone_flags, especially
>considering that you set the flag for all zoned device types, including scsi
>which does not have zone capacity. This makes q->zone_flags rather confusing
>instead of clearly defining the device features as you mentioned in the commit
>message.
>
>I think it may be better to just drop this patch, and if needed, introduce the
>zone_flags field where it may be needed (e.g. OFFLINE zone ioctl support).

I am using this as a way to pass the OFFLINE support flag to the block
layer. I used this too for the attributes. Are you thinking of a
different way to send this?

I believe this fits too for capacity, but we can just pass it in
all report as before if you prefer.

>
>> +
>>  	if (copy_to_user(argp, &rep, sizeof(struct blk_zone_report)))
>>  		return -EFAULT;
>>  	return 0;
>> diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
>> index b05832eb21b2..957c2103f240 100644
>> --- a/drivers/block/null_blk_zoned.c
>> +++ b/drivers/block/null_blk_zoned.c
>> @@ -78,6 +78,8 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
>>  	}
>>
>>  	q->limits.zoned = BLK_ZONED_HM;
>> +	q->zone_flags = BLK_ZONE_REP_CAPACITY;
>> +
>>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>>  	blk_queue_required_elevator_features(q, ELEVATOR_F_ZBD_SEQ_WRITE);
>>
>> diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
>> index 0642d3c54e8f..888264261ba3 100644
>> --- a/drivers/nvme/host/zns.c
>> +++ b/drivers/nvme/host/zns.c
>> @@ -81,6 +81,7 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
>>  	}
>>
>>  	q->limits.zoned = BLK_ZONED_HM;
>> +	q->zone_flags = BLK_ZONE_REP_CAPACITY;
>>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>>  free_data:
>>  	kfree(id);
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index d90fefffe31b..b9c920bace28 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -2967,6 +2967,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>>  	if (sdkp->device->type == TYPE_ZBC) {
>>  		/* Host-managed */
>>  		q->limits.zoned = BLK_ZONED_HM;
>> +		q->zone_flags = BLK_ZONE_REP_CAPACITY;
>>  	} else {
>>  		sdkp->zoned = (buffer[8] >> 4) & 3;
>>  		if (sdkp->zoned == 1 && !disk_has_partitions(sdkp->disk)) {
>> @@ -2983,6 +2984,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>>  					  "Drive-managed SMR disk\n");
>>  		}
>>  	}
>> +
>>  	if (blk_queue_is_zoned(q) && sdkp->first_scan)
>>  		sd_printk(KERN_NOTICE, sdkp, "Host-%s zoned block device\n",
>>  		      q->limits.zoned == BLK_ZONED_HM ? "managed" : "aware");
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 8fd900998b4e..3f2e3425fa53 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -512,12 +512,15 @@ struct request_queue {
>>  	 * Stacking drivers (device mappers) may or may not initialize
>>  	 * these fields.
>>  	 *
>> +	 * Flags represent features as described by blk_zone_report_flags in blkzoned.h
>> +	 *
>>  	 * Reads of this information must be protected with blk_queue_enter() /
>>  	 * blk_queue_exit(). Modifying this information is only allowed while
>>  	 * no requests are being processed. See also blk_mq_freeze_queue() and
>>  	 * blk_mq_unfreeze_queue().
>>  	 */
>>  	unsigned int		nr_zones;
>> +	unsigned int		zone_flags;
>>  	unsigned long		*conv_zones_bitmap;
>>  	unsigned long		*seq_zones_wlock;
>>  #endif /* CONFIG_BLK_DEV_ZONED */
>>
>
>And you are missing device-mapper support. DM target devices have a request
>queue that would need to set the zone_flags too.

Ok. I looked at it and I thought that this would be inherited by the
underlying device. I will add it in V3.

Javier

Damien Le Moal July 2, 2020, 8:49 a.m. UTC | #3

On 2020/07/02 17:34, Javier González wrote:
> On 02.07.2020 07:54, Damien Le Moal wrote:
>> On 2020/07/02 15:55, Javier González wrote:
>>> From: Javier González <[e-mail]>
>>>
>>> As the zoned block device will have to deal with features that are
>>> optional for the backend device, add a flag field to inform the block
>>> layer about supported features. This builds on top of
>>> blk_zone_report_flags and extendes to the zone report code.
>>>
>>> Signed-off-by: Javier González <[e-mail]>
>>> Signed-off-by: SelvaKumar S <[e-mail]>
>>> Signed-off-by: Kanchan Joshi <[e-mail]>
>>> Signed-off-by: Nitesh Shetty <[e-mail]>
>>> ---
>>>  block/blk-zoned.c              | 3 ++-
>>>  drivers/block/null_blk_zoned.c | 2 ++
>>>  drivers/nvme/host/zns.c        | 1 +
>>>  drivers/scsi/sd.c              | 2 ++
>>>  include/linux/blkdev.h         | 3 +++
>>>  5 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>>> index 81152a260354..0f156e96e48f 100644
>>> --- a/block/blk-zoned.c
>>> +++ b/block/blk-zoned.c
>>> @@ -312,7 +312,8 @@ int blkdev_report_zones_ioctl(struct block_device *bdev, fmode_t mode,
>>>  		return ret;
>>>
>>>  	rep.nr_zones = ret;
>>> -	rep.flags = BLK_ZONE_REP_CAPACITY;
>>> +	rep.flags = q->zone_flags;
>>
>> zone_flags seem to be a fairly generic flags field while rep.flags is only about
>> the reported descriptors structure. So you may want to define a
>> BLK_ZONE_REP_FLAGS_MASK and have:
>>
>> 	rep.flags = q->zone_flags & BLK_ZONE_REP_FLAGS_MASK;
>>
>> to avoid flags meaningless for the user being set.
>>
>> In any case, since *all* zoned block devices now report capacity, I do not
>> really see the point to add BLK_ZONE_REP_FLAGS_MASK to q->zone_flags, especially
>> considering that you set the flag for all zoned device types, including scsi
>> which does not have zone capacity. This makes q->zone_flags rather confusing
>> instead of clearly defining the device features as you mentioned in the commit
>> message.
>>
>> I think it may be better to just drop this patch, and if needed, introduce the
>> zone_flags field where it may be needed (e.g. OFFLINE zone ioctl support).
> 
> I am using this as a way to pass the OFFLINE support flag to the block
> layer. I used this too for the attributes. Are you thinking of a
> different way to send this?
> 
> I believe this fits too for capacity, but we can just pass it in
> all report as before if you prefer.

The point is that this patch as is does nothing and is needed as a preparatory
patch if we want to have the offline flag set in the report. But:
1) As commented in the offline ioctl patch, I am not sure the flag makes a lot
of sense. sysfs or nothing at all may be OK as well. When we introduced the new
open/close/finish ioctls, we did not add flags to signal that the device
supports them. Granted, it was for nullblk and scsi, and both had the support.
But running an application using these on an old kernel, and you will get
-ENOTTY, meaning, not supported. So simply introducing the offline ioctl without
any flag would be OK I think.

Since DM support for this would be nice too and we now are in a situation where
not all devices support the  ioctl, instead of a report flag (a little out of
place), a queue limit exported through sysfs may be a cleaner way to both
support DM correctly (stacked limits) and signal the support to the user. If you
choose this approach, then this patch is not needed. The zoned_flags, or a
regular queue flag like for RESET_ALL can be added in the offline ioctl patch.

> 
>>
>>> +
>>>  	if (copy_to_user(argp, &rep, sizeof(struct blk_zone_report)))
>>>  		return -EFAULT;
>>>  	return 0;
>>> diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
>>> index b05832eb21b2..957c2103f240 100644
>>> --- a/drivers/block/null_blk_zoned.c
>>> +++ b/drivers/block/null_blk_zoned.c
>>> @@ -78,6 +78,8 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
>>>  	}
>>>
>>>  	q->limits.zoned = BLK_ZONED_HM;
>>> +	q->zone_flags = BLK_ZONE_REP_CAPACITY;
>>> +
>>>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>>>  	blk_queue_required_elevator_features(q, ELEVATOR_F_ZBD_SEQ_WRITE);
>>>
>>> diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
>>> index 0642d3c54e8f..888264261ba3 100644
>>> --- a/drivers/nvme/host/zns.c
>>> +++ b/drivers/nvme/host/zns.c
>>> @@ -81,6 +81,7 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
>>>  	}
>>>
>>>  	q->limits.zoned = BLK_ZONED_HM;
>>> +	q->zone_flags = BLK_ZONE_REP_CAPACITY;
>>>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>>>  free_data:
>>>  	kfree(id);
>>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>>> index d90fefffe31b..b9c920bace28 100644
>>> --- a/drivers/scsi/sd.c
>>> +++ b/drivers/scsi/sd.c
>>> @@ -2967,6 +2967,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>>>  	if (sdkp->device->type == TYPE_ZBC) {
>>>  		/* Host-managed */
>>>  		q->limits.zoned = BLK_ZONED_HM;
>>> +		q->zone_flags = BLK_ZONE_REP_CAPACITY;
>>>  	} else {
>>>  		sdkp->zoned = (buffer[8] >> 4) & 3;
>>>  		if (sdkp->zoned == 1 && !disk_has_partitions(sdkp->disk)) {
>>> @@ -2983,6 +2984,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>>>  					  "Drive-managed SMR disk\n");
>>>  		}
>>>  	}
>>> +
>>>  	if (blk_queue_is_zoned(q) && sdkp->first_scan)
>>>  		sd_printk(KERN_NOTICE, sdkp, "Host-%s zoned block device\n",
>>>  		      q->limits.zoned == BLK_ZONED_HM ? "managed" : "aware");
>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>> index 8fd900998b4e..3f2e3425fa53 100644
>>> --- a/include/linux/blkdev.h
>>> +++ b/include/linux/blkdev.h
>>> @@ -512,12 +512,15 @@ struct request_queue {
>>>  	 * Stacking drivers (device mappers) may or may not initialize
>>>  	 * these fields.
>>>  	 *
>>> +	 * Flags represent features as described by blk_zone_report_flags in blkzoned.h
>>> +	 *
>>>  	 * Reads of this information must be protected with blk_queue_enter() /
>>>  	 * blk_queue_exit(). Modifying this information is only allowed while
>>>  	 * no requests are being processed. See also blk_mq_freeze_queue() and
>>>  	 * blk_mq_unfreeze_queue().
>>>  	 */
>>>  	unsigned int		nr_zones;
>>> +	unsigned int		zone_flags;
>>>  	unsigned long		*conv_zones_bitmap;
>>>  	unsigned long		*seq_zones_wlock;
>>>  #endif /* CONFIG_BLK_DEV_ZONED */
>>>
>>
>> And you are missing device-mapper support. DM target devices have a request
>> queue that would need to set the zone_flags too.
> 
> Ok. I looked at it and I thought that this would be inherited by the
> underlying device. I will add it in V3.
> 
> Javier
> 
>

Javier González July 2, 2020, 10:27 a.m. UTC | #4

On 02.07.2020 08:49, Damien Le Moal wrote:
>On 2020/07/02 17:34, Javier González wrote:
>> On 02.07.2020 07:54, Damien Le Moal wrote:
>>> On 2020/07/02 15:55, Javier González wrote:
>>>> From: Javier González <[e-mail]>
>>>>
>>>> As the zoned block device will have to deal with features that are
>>>> optional for the backend device, add a flag field to inform the block
>>>> layer about supported features. This builds on top of
>>>> blk_zone_report_flags and extendes to the zone report code.
>>>>
>>>> Signed-off-by: Javier González <[e-mail]>
>>>> Signed-off-by: SelvaKumar S <[e-mail]>
>>>> Signed-off-by: Kanchan Joshi <[e-mail]>
>>>> Signed-off-by: Nitesh Shetty <[e-mail]>
>>>> ---
>>>>  block/blk-zoned.c              | 3 ++-
>>>>  drivers/block/null_blk_zoned.c | 2 ++
>>>>  drivers/nvme/host/zns.c        | 1 +
>>>>  drivers/scsi/sd.c              | 2 ++
>>>>  include/linux/blkdev.h         | 3 +++
>>>>  5 files changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>>>> index 81152a260354..0f156e96e48f 100644
>>>> --- a/block/blk-zoned.c
>>>> +++ b/block/blk-zoned.c
>>>> @@ -312,7 +312,8 @@ int blkdev_report_zones_ioctl(struct block_device *bdev, fmode_t mode,
>>>>  		return ret;
>>>>
>>>>  	rep.nr_zones = ret;
>>>> -	rep.flags = BLK_ZONE_REP_CAPACITY;
>>>> +	rep.flags = q->zone_flags;
>>>
>>> zone_flags seem to be a fairly generic flags field while rep.flags is only about
>>> the reported descriptors structure. So you may want to define a
>>> BLK_ZONE_REP_FLAGS_MASK and have:
>>>
>>> 	rep.flags = q->zone_flags & BLK_ZONE_REP_FLAGS_MASK;
>>>
>>> to avoid flags meaningless for the user being set.
>>>
>>> In any case, since *all* zoned block devices now report capacity, I do not
>>> really see the point to add BLK_ZONE_REP_FLAGS_MASK to q->zone_flags, especially
>>> considering that you set the flag for all zoned device types, including scsi
>>> which does not have zone capacity. This makes q->zone_flags rather confusing
>>> instead of clearly defining the device features as you mentioned in the commit
>>> message.
>>>
>>> I think it may be better to just drop this patch, and if needed, introduce the
>>> zone_flags field where it may be needed (e.g. OFFLINE zone ioctl support).
>>
>> I am using this as a way to pass the OFFLINE support flag to the block
>> layer. I used this too for the attributes. Are you thinking of a
>> different way to send this?
>>
>> I believe this fits too for capacity, but we can just pass it in
>> all report as before if you prefer.
>
>The point is that this patch as is does nothing and is needed as a preparatory
>patch if we want to have the offline flag set in the report. But:
>1) As commented in the offline ioctl patch, I am not sure the flag makes a lot
>of sense. sysfs or nothing at all may be OK as well. When we introduced the new
>open/close/finish ioctls, we did not add flags to signal that the device
>supports them. Granted, it was for nullblk and scsi, and both had the support.
>But running an application using these on an old kernel, and you will get
>-ENOTTY, meaning, not supported. So simply introducing the offline ioctl without
>any flag would be OK I think.

I see. My understanding after some comments from Christoph was that we
should use these bits to signal any optional features / capabilities
that would depend on the underlying driver, just as it is done with the
capacity flag today.

If not for the offline transition, for the attributes, I see it exactly
as the same use case as capacity, where we signal that a new field is
reported in the report structure.

Am I missing something here?

>
>Since DM support for this would be nice too and we now are in a situation where
>not all devices support the  ioctl, instead of a report flag (a little out of
>place), a queue limit exported through sysfs may be a cleaner way to both
>support DM correctly (stacked limits) and signal the support to the user. If you
>choose this approach, then this patch is not needed. The zoned_flags, or a
>regular queue flag like for RESET_ALL can be added in the offline ioctl patch.

I see. If we can reach an agreement that the above is a bad
understanding on my side, I will be happy to translate this into a sysfs
entry. If it is OK, I'll give it this week in the mailing list and send
a V4 next week.

>
>>
>>>
>>>> +
>>>>  	if (copy_to_user(argp, &rep, sizeof(struct blk_zone_report)))
>>>>  		return -EFAULT;
>>>>  	return 0;
>>>> diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
>>>> index b05832eb21b2..957c2103f240 100644
>>>> --- a/drivers/block/null_blk_zoned.c
>>>> +++ b/drivers/block/null_blk_zoned.c
>>>> @@ -78,6 +78,8 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
>>>>  	}
>>>>
>>>>  	q->limits.zoned = BLK_ZONED_HM;
>>>> +	q->zone_flags = BLK_ZONE_REP_CAPACITY;
>>>> +
>>>>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>>>>  	blk_queue_required_elevator_features(q, ELEVATOR_F_ZBD_SEQ_WRITE);
>>>>
>>>> diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
>>>> index 0642d3c54e8f..888264261ba3 100644
>>>> --- a/drivers/nvme/host/zns.c
>>>> +++ b/drivers/nvme/host/zns.c
>>>> @@ -81,6 +81,7 @@ int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
>>>>  	}
>>>>
>>>>  	q->limits.zoned = BLK_ZONED_HM;
>>>> +	q->zone_flags = BLK_ZONE_REP_CAPACITY;
>>>>  	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
>>>>  free_data:
>>>>  	kfree(id);
>>>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>>>> index d90fefffe31b..b9c920bace28 100644
>>>> --- a/drivers/scsi/sd.c
>>>> +++ b/drivers/scsi/sd.c
>>>> @@ -2967,6 +2967,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>>>>  	if (sdkp->device->type == TYPE_ZBC) {
>>>>  		/* Host-managed */
>>>>  		q->limits.zoned = BLK_ZONED_HM;
>>>> +		q->zone_flags = BLK_ZONE_REP_CAPACITY;
>>>>  	} else {
>>>>  		sdkp->zoned = (buffer[8] >> 4) & 3;
>>>>  		if (sdkp->zoned == 1 && !disk_has_partitions(sdkp->disk)) {
>>>> @@ -2983,6 +2984,7 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
>>>>  					  "Drive-managed SMR disk\n");
>>>>  		}
>>>>  	}
>>>> +
>>>>  	if (blk_queue_is_zoned(q) && sdkp->first_scan)
>>>>  		sd_printk(KERN_NOTICE, sdkp, "Host-%s zoned block device\n",
>>>>  		      q->limits.zoned == BLK_ZONED_HM ? "managed" : "aware");
>>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>>> index 8fd900998b4e..3f2e3425fa53 100644
>>>> --- a/include/linux/blkdev.h
>>>> +++ b/include/linux/blkdev.h
>>>> @@ -512,12 +512,15 @@ struct request_queue {
>>>>  	 * Stacking drivers (device mappers) may or may not initialize
>>>>  	 * these fields.
>>>>  	 *
>>>> +	 * Flags represent features as described by blk_zone_report_flags in blkzoned.h
>>>> +	 *
>>>>  	 * Reads of this information must be protected with blk_queue_enter() /
>>>>  	 * blk_queue_exit(). Modifying this information is only allowed while
>>>>  	 * no requests are being processed. See also blk_mq_freeze_queue() and
>>>>  	 * blk_mq_unfreeze_queue().
>>>>  	 */
>>>>  	unsigned int		nr_zones;
>>>> +	unsigned int		zone_flags;
>>>>  	unsigned long		*conv_zones_bitmap;
>>>>  	unsigned long		*seq_zones_wlock;
>>>>  #endif /* CONFIG_BLK_DEV_ZONED */
>>>>
>>>
>>> And you are missing device-mapper support. DM target devices have a request
>>> queue that would need to set the zone_flags too.

Yes. As mentioned, I did not want to introduce more changes to this
series, just fix the mistake I made with some added debug code.

>>
>> Ok. I looked at it and I thought that this would be inherited by the
>> underlying device. I will add it in V3.
>>
>> Javier
>>
>>

Damien Le Moal July 3, 2020, 5:20 a.m. UTC | #5

On 2020/07/02 19:27, Javier González wrote:
> On 02.07.2020 08:49, Damien Le Moal wrote:
>> On 2020/07/02 17:34, Javier González wrote:
>>> On 02.07.2020 07:54, Damien Le Moal wrote:
>>>> On 2020/07/02 15:55, Javier González wrote:
>>>>> From: Javier González <[e-mail]>
>>>>>
>>>>> As the zoned block device will have to deal with features that are
>>>>> optional for the backend device, add a flag field to inform the block
>>>>> layer about supported features. This builds on top of
>>>>> blk_zone_report_flags and extendes to the zone report code.
>>>>>
>>>>> Signed-off-by: Javier González <[e-mail]>
>>>>> Signed-off-by: SelvaKumar S <[e-mail]>
>>>>> Signed-off-by: Kanchan Joshi <[e-mail]>
>>>>> Signed-off-by: Nitesh Shetty <[e-mail]>
>>>>> ---
>>>>>  block/blk-zoned.c              | 3 ++-
>>>>>  drivers/block/null_blk_zoned.c | 2 ++
>>>>>  drivers/nvme/host/zns.c        | 1 +
>>>>>  drivers/scsi/sd.c              | 2 ++
>>>>>  include/linux/blkdev.h         | 3 +++
>>>>>  5 files changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>>>>> index 81152a260354..0f156e96e48f 100644
>>>>> --- a/block/blk-zoned.c
>>>>> +++ b/block/blk-zoned.c
>>>>> @@ -312,7 +312,8 @@ int blkdev_report_zones_ioctl(struct block_device *bdev, fmode_t mode,
>>>>>  		return ret;
>>>>>
>>>>>  	rep.nr_zones = ret;
>>>>> -	rep.flags = BLK_ZONE_REP_CAPACITY;
>>>>> +	rep.flags = q->zone_flags;
>>>>
>>>> zone_flags seem to be a fairly generic flags field while rep.flags is only about
>>>> the reported descriptors structure. So you may want to define a
>>>> BLK_ZONE_REP_FLAGS_MASK and have:
>>>>
>>>> 	rep.flags = q->zone_flags & BLK_ZONE_REP_FLAGS_MASK;
>>>>
>>>> to avoid flags meaningless for the user being set.
>>>>
>>>> In any case, since *all* zoned block devices now report capacity, I do not
>>>> really see the point to add BLK_ZONE_REP_FLAGS_MASK to q->zone_flags, especially
>>>> considering that you set the flag for all zoned device types, including scsi
>>>> which does not have zone capacity. This makes q->zone_flags rather confusing
>>>> instead of clearly defining the device features as you mentioned in the commit
>>>> message.
>>>>
>>>> I think it may be better to just drop this patch, and if needed, introduce the
>>>> zone_flags field where it may be needed (e.g. OFFLINE zone ioctl support).
>>>
>>> I am using this as a way to pass the OFFLINE support flag to the block
>>> layer. I used this too for the attributes. Are you thinking of a
>>> different way to send this?
>>>
>>> I believe this fits too for capacity, but we can just pass it in
>>> all report as before if you prefer.
>>
>> The point is that this patch as is does nothing and is needed as a preparatory
>> patch if we want to have the offline flag set in the report. But:
>> 1) As commented in the offline ioctl patch, I am not sure the flag makes a lot
>> of sense. sysfs or nothing at all may be OK as well. When we introduced the new
>> open/close/finish ioctls, we did not add flags to signal that the device
>> supports them. Granted, it was for nullblk and scsi, and both had the support.
>> But running an application using these on an old kernel, and you will get
>> -ENOTTY, meaning, not supported. So simply introducing the offline ioctl without
>> any flag would be OK I think.
> 
> I see. My understanding after some comments from Christoph was that we
> should use these bits to signal any optional features / capabilities
> that would depend on the underlying driver, just as it is done with the
> capacity flag today.
> 
> If not for the offline transition, for the attributes, I see it exactly
> as the same use case as capacity, where we signal that a new field is
> reported in the report structure.
> 
> Am I missing something here?

Using the report flags for reporting supported operations/features is fine, but
as already mentioned, then document that by changing the description of enum
blk_zone_report_flags. Right now, it still says:

 * enum blk_zone_report_flags - Feature flags of reported zone descriptors.

Which kind of really point to things the zone descriptor itself has compared to
earlier versions of the descriptor rather than what the device can do or not.

And as I argued already, using a flag is fine only for letting a user know about
a supported feature, but that is far from ideal to have device-mapper easily
discover what a target can support or not. For that, stacked queue limits are
much simpler. Which leads to exporting that limit in sysfs rather than using a
flag for the user interface.

>> Since DM support for this would be nice too and we now are in a situation where
>> not all devices support the  ioctl, instead of a report flag (a little out of
>> place), a queue limit exported through sysfs may be a cleaner way to both
>> support DM correctly (stacked limits) and signal the support to the user. If you
>> choose this approach, then this patch is not needed. The zoned_flags, or a
>> regular queue flag like for RESET_ALL can be added in the offline ioctl patch.
> 
> I see. If we can reach an agreement that the above is a bad
> understanding on my side, I will be happy to translate this into a sysfs
> entry. If it is OK, I'll give it this week in the mailing list and send
> a V4 next week.

It is all about device mapper support... How are you planning to do it using a
queue flag without said flags being stackable as queue limits ?

Javier González July 3, 2020, 6:28 a.m. UTC | #6

On 03.07.2020 05:20, Damien Le Moal wrote:
>On 2020/07/02 19:27, Javier González wrote:
>> On 02.07.2020 08:49, Damien Le Moal wrote:
>>> On 2020/07/02 17:34, Javier González wrote:
>>>> On 02.07.2020 07:54, Damien Le Moal wrote:
>>>>> On 2020/07/02 15:55, Javier González wrote:
>>>>>> From: Javier González <[e-mail]>
>>>>>>
>>>>>> As the zoned block device will have to deal with features that are
>>>>>> optional for the backend device, add a flag field to inform the block
>>>>>> layer about supported features. This builds on top of
>>>>>> blk_zone_report_flags and extendes to the zone report code.
>>>>>>
>>>>>> Signed-off-by: Javier González <[e-mail]>
>>>>>> Signed-off-by: SelvaKumar S <[e-mail]>
>>>>>> Signed-off-by: Kanchan Joshi <[e-mail]>
>>>>>> Signed-off-by: Nitesh Shetty <[e-mail]>
>>>>>> ---
>>>>>>  block/blk-zoned.c              | 3 ++-
>>>>>>  drivers/block/null_blk_zoned.c | 2 ++
>>>>>>  drivers/nvme/host/zns.c        | 1 +
>>>>>>  drivers/scsi/sd.c              | 2 ++
>>>>>>  include/linux/blkdev.h         | 3 +++
>>>>>>  5 files changed, 10 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>>>>>> index 81152a260354..0f156e96e48f 100644
>>>>>> --- a/block/blk-zoned.c
>>>>>> +++ b/block/blk-zoned.c
>>>>>> @@ -312,7 +312,8 @@ int blkdev_report_zones_ioctl(struct block_device *bdev, fmode_t mode,
>>>>>>  		return ret;
>>>>>>
>>>>>>  	rep.nr_zones = ret;
>>>>>> -	rep.flags = BLK_ZONE_REP_CAPACITY;
>>>>>> +	rep.flags = q->zone_flags;
>>>>>
>>>>> zone_flags seem to be a fairly generic flags field while rep.flags is only about
>>>>> the reported descriptors structure. So you may want to define a
>>>>> BLK_ZONE_REP_FLAGS_MASK and have:
>>>>>
>>>>> 	rep.flags = q->zone_flags & BLK_ZONE_REP_FLAGS_MASK;
>>>>>
>>>>> to avoid flags meaningless for the user being set.
>>>>>
>>>>> In any case, since *all* zoned block devices now report capacity, I do not
>>>>> really see the point to add BLK_ZONE_REP_FLAGS_MASK to q->zone_flags, especially
>>>>> considering that you set the flag for all zoned device types, including scsi
>>>>> which does not have zone capacity. This makes q->zone_flags rather confusing
>>>>> instead of clearly defining the device features as you mentioned in the commit
>>>>> message.
>>>>>
>>>>> I think it may be better to just drop this patch, and if needed, introduce the
>>>>> zone_flags field where it may be needed (e.g. OFFLINE zone ioctl support).
>>>>
>>>> I am using this as a way to pass the OFFLINE support flag to the block
>>>> layer. I used this too for the attributes. Are you thinking of a
>>>> different way to send this?
>>>>
>>>> I believe this fits too for capacity, but we can just pass it in
>>>> all report as before if you prefer.
>>>
>>> The point is that this patch as is does nothing and is needed as a preparatory
>>> patch if we want to have the offline flag set in the report. But:
>>> 1) As commented in the offline ioctl patch, I am not sure the flag makes a lot
>>> of sense. sysfs or nothing at all may be OK as well. When we introduced the new
>>> open/close/finish ioctls, we did not add flags to signal that the device
>>> supports them. Granted, it was for nullblk and scsi, and both had the support.
>>> But running an application using these on an old kernel, and you will get
>>> -ENOTTY, meaning, not supported. So simply introducing the offline ioctl without
>>> any flag would be OK I think.
>>
>> I see. My understanding after some comments from Christoph was that we
>> should use these bits to signal any optional features / capabilities
>> that would depend on the underlying driver, just as it is done with the
>> capacity flag today.
>>
>> If not for the offline transition, for the attributes, I see it exactly
>> as the same use case as capacity, where we signal that a new field is
>> reported in the report structure.
>>
>> Am I missing something here?
>
>Using the report flags for reporting supported operations/features is fine, but
>as already mentioned, then document that by changing the description of enum
>blk_zone_report_flags. Right now, it still says:
>
> * enum blk_zone_report_flags - Feature flags of reported zone descriptors.
>
>Which kind of really point to things the zone descriptor itself has compared to
>earlier versions of the descriptor rather than what the device can do or not.

I see how the offline transition collides with this model. But can we
agree that the zone attributes is something that fits here?

>
>And as I argued already, using a flag is fine only for letting a user know about
>a supported feature, but that is far from ideal to have device-mapper easily
>discover what a target can support or not. For that, stacked queue limits are
>much simpler. Which leads to exporting that limit in sysfs rather than using a
>flag for the user interface.

See below

>
>>> Since DM support for this would be nice too and we now are in a situation where
>>> not all devices support the  ioctl, instead of a report flag (a little out of
>>> place), a queue limit exported through sysfs may be a cleaner way to both
>>> support DM correctly (stacked limits) and signal the support to the user. If you
>>> choose this approach, then this patch is not needed. The zoned_flags, or a
>>> regular queue flag like for RESET_ALL can be added in the offline ioctl patch.
>>
>> I see. If we can reach an agreement that the above is a bad
>> understanding on my side, I will be happy to translate this into a sysfs
>> entry. If it is OK, I'll give it this week in the mailing list and send
>> a V4 next week.
>
>It is all about device mapper support... How are you planning to do it using a
>queue flag without said flags being stackable as queue limits ?

I guess what I am struggle with here is that you see the capacity
feature as a different extension than the attributes and the offline
transition.

The way I see it, there are things the device supports and things that
the driver supports. ZAC/ZBC does not support a size != capacity, but
the driver sets these values to be the same. One thing I struggle with
is that saying that we support capacity and setting size = capacity puts
the burden in the user to check these values across all zones to
determine if they can support the driver or not. I think it would be
clear to have a feature explicitly stating that capacity != size.

For the attributes, this is very similar - some apply, some don't, and
we only report the ones that make sense for each driver. I do not see
how device mappers are treated differently here than in the existing
capacity features.

For the offline transition, I see that the current patches do not set
flags properly and we would need to inherit the underlying device if we
want to do it this way. I can understand from your comments that this is
not a good way of doing it. I see in one of your comments from V1 that
we could deal with this transition in the scsi driver and keep it in
memory (i.e., device is still read-only, but driver transitions to
offline) - can you comment on this?

Javier

Damien Le Moal July 3, 2020, 8:35 a.m. UTC | #7

On 2020/07/03 15:28, Javier González wrote:
> On 03.07.2020 05:20, Damien Le Moal wrote:
>> On 2020/07/02 19:27, Javier González wrote:
>>> On 02.07.2020 08:49, Damien Le Moal wrote:
>>>> On 2020/07/02 17:34, Javier González wrote:
>>>>> On 02.07.2020 07:54, Damien Le Moal wrote:
>>>>>> On 2020/07/02 15:55, Javier González wrote:
>>>>>>> From: Javier González <[e-mail]>
>>>>>>>
>>>>>>> As the zoned block device will have to deal with features that are
>>>>>>> optional for the backend device, add a flag field to inform the block
>>>>>>> layer about supported features. This builds on top of
>>>>>>> blk_zone_report_flags and extendes to the zone report code.
>>>>>>>
>>>>>>> Signed-off-by: Javier González <[e-mail]>
>>>>>>> Signed-off-by: SelvaKumar S <[e-mail]>
>>>>>>> Signed-off-by: Kanchan Joshi <[e-mail]>
>>>>>>> Signed-off-by: Nitesh Shetty <[e-mail]>
>>>>>>> ---
>>>>>>>  block/blk-zoned.c              | 3 ++-
>>>>>>>  drivers/block/null_blk_zoned.c | 2 ++
>>>>>>>  drivers/nvme/host/zns.c        | 1 +
>>>>>>>  drivers/scsi/sd.c              | 2 ++
>>>>>>>  include/linux/blkdev.h         | 3 +++
>>>>>>>  5 files changed, 10 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>>>>>>> index 81152a260354..0f156e96e48f 100644
>>>>>>> --- a/block/blk-zoned.c
>>>>>>> +++ b/block/blk-zoned.c
>>>>>>> @@ -312,7 +312,8 @@ int blkdev_report_zones_ioctl(struct block_device *bdev, fmode_t mode,
>>>>>>>  		return ret;
>>>>>>>
>>>>>>>  	rep.nr_zones = ret;
>>>>>>> -	rep.flags = BLK_ZONE_REP_CAPACITY;
>>>>>>> +	rep.flags = q->zone_flags;
>>>>>>
>>>>>> zone_flags seem to be a fairly generic flags field while rep.flags is only about
>>>>>> the reported descriptors structure. So you may want to define a
>>>>>> BLK_ZONE_REP_FLAGS_MASK and have:
>>>>>>
>>>>>> 	rep.flags = q->zone_flags & BLK_ZONE_REP_FLAGS_MASK;
>>>>>>
>>>>>> to avoid flags meaningless for the user being set.
>>>>>>
>>>>>> In any case, since *all* zoned block devices now report capacity, I do not
>>>>>> really see the point to add BLK_ZONE_REP_FLAGS_MASK to q->zone_flags, especially
>>>>>> considering that you set the flag for all zoned device types, including scsi
>>>>>> which does not have zone capacity. This makes q->zone_flags rather confusing
>>>>>> instead of clearly defining the device features as you mentioned in the commit
>>>>>> message.
>>>>>>
>>>>>> I think it may be better to just drop this patch, and if needed, introduce the
>>>>>> zone_flags field where it may be needed (e.g. OFFLINE zone ioctl support).
>>>>>
>>>>> I am using this as a way to pass the OFFLINE support flag to the block
>>>>> layer. I used this too for the attributes. Are you thinking of a
>>>>> different way to send this?
>>>>>
>>>>> I believe this fits too for capacity, but we can just pass it in
>>>>> all report as before if you prefer.
>>>>
>>>> The point is that this patch as is does nothing and is needed as a preparatory
>>>> patch if we want to have the offline flag set in the report. But:
>>>> 1) As commented in the offline ioctl patch, I am not sure the flag makes a lot
>>>> of sense. sysfs or nothing at all may be OK as well. When we introduced the new
>>>> open/close/finish ioctls, we did not add flags to signal that the device
>>>> supports them. Granted, it was for nullblk and scsi, and both had the support.
>>>> But running an application using these on an old kernel, and you will get
>>>> -ENOTTY, meaning, not supported. So simply introducing the offline ioctl without
>>>> any flag would be OK I think.
>>>
>>> I see. My understanding after some comments from Christoph was that we
>>> should use these bits to signal any optional features / capabilities
>>> that would depend on the underlying driver, just as it is done with the
>>> capacity flag today.
>>>
>>> If not for the offline transition, for the attributes, I see it exactly
>>> as the same use case as capacity, where we signal that a new field is
>>> reported in the report structure.
>>>
>>> Am I missing something here?
>>
>> Using the report flags for reporting supported operations/features is fine, but
>> as already mentioned, then document that by changing the description of enum
>> blk_zone_report_flags. Right now, it still says:
>>
>> * enum blk_zone_report_flags - Feature flags of reported zone descriptors.
>>
>> Which kind of really point to things the zone descriptor itself has compared to
>> earlier versions of the descriptor rather than what the device can do or not.
> 
> I see how the offline transition collides with this model. But can we
> agree that the zone attributes is something that fits here?

Flags may be fine. I still do not see it though due to your patches missing
device mapper support. Please send patches that cover all zoned block device
types, including device mapper to show that this is a good approach.

>> And as I argued already, using a flag is fine only for letting a user know about
>> a supported feature, but that is far from ideal to have device-mapper easily
>> discover what a target can support or not. For that, stacked queue limits are
>> much simpler. Which leads to exporting that limit in sysfs rather than using a
>> flag for the user interface.
> 
> See below
> 
>>
>>>> Since DM support for this would be nice too and we now are in a situation where
>>>> not all devices support the  ioctl, instead of a report flag (a little out of
>>>> place), a queue limit exported through sysfs may be a cleaner way to both
>>>> support DM correctly (stacked limits) and signal the support to the user. If you
>>>> choose this approach, then this patch is not needed. The zoned_flags, or a
>>>> regular queue flag like for RESET_ALL can be added in the offline ioctl patch.
>>>
>>> I see. If we can reach an agreement that the above is a bad
>>> understanding on my side, I will be happy to translate this into a sysfs
>>> entry. If it is OK, I'll give it this week in the mailing list and send
>>> a V4 next week.
>>
>> It is all about device mapper support... How are you planning to do it using a
>> queue flag without said flags being stackable as queue limits ?
> 
> I guess what I am struggle with here is that you see the capacity
> feature as a different extension than the attributes and the offline
> transition.
> 
> The way I see it, there are things the device supports and things that
> the driver supports. ZAC/ZBC does not support a size != capacity, but
> the driver sets these values to be the same. One thing I struggle with
> is that saying that we support capacity and setting size = capacity puts
> the burden in the user to check these values across all zones to
> determine if they can support the driver or not. I think it would be
> clear to have a feature explicitly stating that capacity != size.

From the user (application or in-kernel) perspective, *all* zoned device now
have a zone capacity. I do not think it matters if that comes from the device or
from the driver. It is unconditionally supported. The report flag
BLK_ZONE_REP_CAPACITY exist to signal to user applications that the zone
descriptor has the capactiy field, to allow for backward compatibility with
older kernels. See patches for fio or util-linux blkzone posted to see its use.

Having a flag that explicitly states that zone capacity != zone size would be
useful only and only if *all* zones of the device have that property. What if
only some zones have a capacity lower than the zone size ? The user would still
need a zone report to sort things out.

> For the attributes, this is very similar - some apply, some don't, and
> we only report the ones that make sense for each driver. I do not see
> how device mappers are treated differently here than in the existing
> capacity features.

The capacity feature is supported by *all* zoned block device. Support coming
from the device or from the driver does not matter. The API is consistent for
all device types. So in my opinion, there is no need to consider zone capacity
as an attribute. It is supported by everything, it is now part of the API,
signaled with BLK_ZONE_REP_CAPACITY for backward compatibility handling.

You are mixing up the report flags describing the structure of the zone
descriptor API with optional device features attributes. Adding an attribute
field to the report zones descriptors is fine. The presence of that field can be
signaled with a BLK_ZONE_REP_ATTR report flag as you already proposed. No problem.

But then, imagine a device mapper target using multiple devices with different
optional features. How do you propose that the device mapper target sorts this
out and come up with a coherent set of attributes for its target report zones ?
Please propose a solution for that. Your patches so far are all missing that.

> For the offline transition, I see that the current patches do not set
> flags properly and we would need to inherit the underlying device if we
> want to do it this way. I can understand from your comments that this is
> not a good way of doing it. I see in one of your comments from V1 that
> we could deal with this transition in the scsi driver and keep it in
> memory (i.e., device is still read-only, but driver transitions to
> offline) - can you comment on this?

That would not be persistent across reboot, unlike for ZNS drive. So forget
about this. That was wishful thinking from me. This will not work.

Explicit zone offlining is truly an optional feature of the device. Signaling
its support to the user is necessary, but again, I would like that to be done in
a way that does not force device mapper targets to forever report nothing for
the optional features. A flag field can certainly be used, but it has to  be
handled in blk_queue_stack_limits(). That may be as simple as taking a bitwise
AND of each device zone attribute flags.

Patch

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 81152a260354..0f156e96e48f 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -312,7 +312,8 @@  int blkdev_report_zones_ioctl(struct block_device *bdev, fmode_t mode,
 		return ret;
 
 	rep.nr_zones = ret;
-	rep.flags = BLK_ZONE_REP_CAPACITY;
+	rep.flags = q->zone_flags;
+
 	if (copy_to_user(argp, &rep, sizeof(struct blk_zone_report)))
 		return -EFAULT;
 	return 0;
diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c
index b05832eb21b2..957c2103f240 100644
--- a/drivers/block/null_blk_zoned.c
+++ b/drivers/block/null_blk_zoned.c
@@ -78,6 +78,8 @@  int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q)
 	}
 
 	q->limits.zoned = BLK_ZONED_HM;
+	q->zone_flags = BLK_ZONE_REP_CAPACITY;
+
 	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
 	blk_queue_required_elevator_features(q, ELEVATOR_F_ZBD_SEQ_WRITE);
 
diff --git a/drivers/nvme/host/zns.c b/drivers/nvme/host/zns.c
index 0642d3c54e8f..888264261ba3 100644
--- a/drivers/nvme/host/zns.c
+++ b/drivers/nvme/host/zns.c
@@ -81,6 +81,7 @@  int nvme_update_zone_info(struct gendisk *disk, struct nvme_ns *ns,
 	}
 
 	q->limits.zoned = BLK_ZONED_HM;
+	q->zone_flags = BLK_ZONE_REP_CAPACITY;
 	blk_queue_flag_set(QUEUE_FLAG_ZONE_RESETALL, q);
 free_data:
 	kfree(id);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d90fefffe31b..b9c920bace28 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2967,6 +2967,7 @@  static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 	if (sdkp->device->type == TYPE_ZBC) {
 		/* Host-managed */
 		q->limits.zoned = BLK_ZONED_HM;
+		q->zone_flags = BLK_ZONE_REP_CAPACITY;
 	} else {
 		sdkp->zoned = (buffer[8] >> 4) & 3;
 		if (sdkp->zoned == 1 && !disk_has_partitions(sdkp->disk)) {
@@ -2983,6 +2984,7 @@  static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 					  "Drive-managed SMR disk\n");
 		}
 	}
+
 	if (blk_queue_is_zoned(q) && sdkp->first_scan)
 		sd_printk(KERN_NOTICE, sdkp, "Host-%s zoned block device\n",
 		      q->limits.zoned == BLK_ZONED_HM ? "managed" : "aware");
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 8fd900998b4e..3f2e3425fa53 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -512,12 +512,15 @@  struct request_queue {
 	 * Stacking drivers (device mappers) may or may not initialize
 	 * these fields.
 	 *
+	 * Flags represent features as described by blk_zone_report_flags in blkzoned.h
+	 *
 	 * Reads of this information must be protected with blk_queue_enter() /
 	 * blk_queue_exit(). Modifying this information is only allowed while
 	 * no requests are being processed. See also blk_mq_freeze_queue() and
 	 * blk_mq_unfreeze_queue().
 	 */
 	unsigned int		nr_zones;
+	unsigned int		zone_flags;
 	unsigned long		*conv_zones_bitmap;
 	unsigned long		*seq_zones_wlock;
 #endif /* CONFIG_BLK_DEV_ZONED */