[PATCH 1/4] block: Add zone flags to queue zone prop.
Damien Le Moal
Damien.LeMoal at wdc.com
Thu Jul 2 04:49:38 EDT 2020
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 <javier.gonz at samsung.com>
>>>
>>> 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 <javier.gonz at samsung.com>
>>> Signed-off-by: SelvaKumar S <selvakuma.s1 at samsung.com>
>>> Signed-off-by: Kanchan Joshi <joshi.k at samsung.com>
>>> Signed-off-by: Nitesh Shetty <nj.shetty at samsung.com>
>>> ---
>>> 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
>
>
--
Damien Le Moal
Western Digital Research
More information about the Linux-nvme
mailing list