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 <javier.gonz@xxxxxxxxxxx>
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@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 ++-
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
