linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/5] Initial support for multi-actuator HDDs
@ 2021-08-12  7:50 Damien Le Moal
  2021-08-12  7:50 ` [PATCH v5 1/5] block: Add concurrent positioning ranges support Damien Le Moal
                   ` (8 more replies)
  0 siblings, 9 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-12  7:50 UTC (permalink / raw)
  To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi

Single LUN multi-actuator hard-disks are cappable to seek and execute
multiple commands in parallel. This capability is exposed to the host
using the Concurrent Positioning Ranges VPD page (SCSI) and Log (ATA).
Each positioning range describes the contiguous set of LBAs that an
actuator serves.

This series adds support the scsi disk driver to retreive this
information and advertize it to user space through sysfs. libata is also
modified to handle ATA drives.

The first patch adds the block layer plumbing to expose concurrent
sector ranges of the device through sysfs as a sub-directory of the
device sysfs queue directory. Patch 2 and 3 add support to sd and
libata. Finally patch 4 documents the sysfs queue attributed changes.
Patch 5 fixes a typo in the document file (strictly speaking, not
related to this series).

This series does not attempt in any way to optimize accesses to
multi-actuator devices (e.g. block IO scheduler or filesystems). This
initial support only exposes the actuators information to user space
through sysfs.

Changes from v4:
* Fixed kdoc comment function name mismatch for disk_register_cranges()
  in patch 1

Changes from v3:
* Modified patch 1:
  - Prefix functions that take a struct gendisk as argument with
    "disk_". Modified patch 2 accordingly.
  - Added a functional release operation for struct blk_cranges kobj to
    ensure that this structure is freed only after all references to it
    are released, including kobject_del() execution for all crange sysfs
    entries.
* Added patch 5 to separate the typo fix from the crange documentation
  addition.
* Added reviewed-by tags

Changes from v2:
* Update patch 1 to fix a compilation warning for a potential NULL
  pointer dereference of the cr argument of blk_queue_set_cranges().
  Warning reported by the kernel test robot <lkp@intel.com>).

Changes from v1:
* Moved libata-scsi hunk from patch 1 to patch 3 where it belongs
* Fixed unintialized variable in patch 2
  Reported-by: kernel test robot <lkp@intel.com>
  Reported-by: Dan Carpenter <dan.carpenter@oracle.com
* Changed patch 3 adding struct ata_cpr_log to contain both the number
  of concurrent ranges and the array of concurrent ranges.
* Added a note in the documentation (patch 4) about the unit used for
  the concurrent ranges attributes.

Damien Le Moal (5):
  block: Add concurrent positioning ranges support
  scsi: sd: add concurrent positioning ranges support
  libata: support concurrent positioning ranges log
  doc: document sysfs queue/cranges attributes
  doc: Fix typo in request queue sysfs documentation

 Documentation/block/queue-sysfs.rst |  30 ++-
 block/Makefile                      |   2 +-
 block/blk-cranges.c                 | 310 ++++++++++++++++++++++++++++
 block/blk-sysfs.c                   |  26 ++-
 block/blk.h                         |   4 +
 drivers/ata/libata-core.c           |  52 +++++
 drivers/ata/libata-scsi.c           |  48 ++++-
 drivers/scsi/sd.c                   |  81 ++++++++
 drivers/scsi/sd.h                   |   1 +
 include/linux/ata.h                 |   1 +
 include/linux/blkdev.h              |  29 +++
 include/linux/libata.h              |  15 ++
 12 files changed, 580 insertions(+), 19 deletions(-)
 create mode 100644 block/blk-cranges.c

-- 
2.31.1


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v5 1/5] block: Add concurrent positioning ranges support
  2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
@ 2021-08-12  7:50 ` Damien Le Moal
  2021-08-12  7:50 ` [PATCH v5 2/5] scsi: sd: add " Damien Le Moal
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-12  7:50 UTC (permalink / raw)
  To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi

The Concurrent Positioning Ranges VPD page (for SCSI) and Log (for ATA)
contain parameters describing the number of sets of contiguous LBAs that
can be served independently by a single LUN multi-actuator disk. This
patch provides the blk_queue_set_cranges() function allowing a device
driver to signal to the block layer that a disk has multiple actuators,
each one serving a contiguous range of sectors. To describe the set
of sector ranges representing the different actuators of a device, the
data type struct blk_cranges is introduced.

For a device with multiple actuators, a struct blk_cranges is attached
to the device request queue by the disk_set_cranges() function. The
function disk_alloc_cranges() is provided for drivers to allocate this
structure.

The blk_cranges structure contains kobjects (struct kobject) to register
with sysfs the set of sector ranges defined by a device. On initial
device scan, this registration is done from blk_register_queue() using
the block layer internal function disk_register_cranges(). If a driver
calls disk_set_cranges() for a registered queue, e.g. when a device
is revalidated, disk_set_cranges() will execute disk_register_cranges()
to update the queue sysfs attribute files.

The sysfs file structure created starts from the cranges sub-directory
and contains the start sector and number of sectors served by an
actuator, with the information for each actuator grouped in one
directory per actuator. E.g. for a dual actuator drive, we have:

$ tree /sys/block/sdk/queue/cranges/
/sys/block/sdk/queue/cranges/
|-- 0
|   |-- nr_sectors
|   `-- sector
`-- 1
    |-- nr_sectors
    `-- sector

For a regular single actuator device, the cranges directory does not
exist.

Device revalidation may lead to changes to this structure and to the
attribute values. When manipulated, the queue sysfs_lock and
sysfs_dir_lock are held for atomicity, similarly to how the blk-mq and
elevator sysfs queue sub-directories are protected.

The code related to the management of cranges is added in the new
file block/blk-cranges.c.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 block/Makefile         |   2 +-
 block/blk-cranges.c    | 310 +++++++++++++++++++++++++++++++++++++++++
 block/blk-sysfs.c      |  26 ++--
 block/blk.h            |   4 +
 include/linux/blkdev.h |  29 ++++
 5 files changed, 362 insertions(+), 9 deletions(-)
 create mode 100644 block/blk-cranges.c

diff --git a/block/Makefile b/block/Makefile
index 0d951adce796..7b8a2b969537 100644
--- a/block/Makefile
+++ b/block/Makefile
@@ -9,7 +9,7 @@ obj-$(CONFIG_BLOCK) := bio.o elevator.o blk-core.o blk-sysfs.o \
 			blk-lib.o blk-mq.o blk-mq-tag.o blk-stat.o \
 			blk-mq-sysfs.o blk-mq-cpumap.o blk-mq-sched.o ioctl.o \
 			genhd.o ioprio.o badblocks.o partitions/ blk-rq-qos.o \
-			disk-events.o
+			disk-events.o blk-cranges.o
 
 obj-$(CONFIG_BOUNCE)		+= bounce.o
 obj-$(CONFIG_BLK_SCSI_REQUEST)	+= scsi_ioctl.o
diff --git a/block/blk-cranges.c b/block/blk-cranges.c
new file mode 100644
index 000000000000..9994301414ee
--- /dev/null
+++ b/block/blk-cranges.c
@@ -0,0 +1,310 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  Block device concurrent positioning ranges.
+ *
+ *  Copyright (C) 2021 Western Digital Corporation or its Affiliates.
+ */
+#include <linux/kernel.h>
+#include <linux/blkdev.h>
+#include <linux/slab.h>
+#include <linux/init.h>
+
+#include "blk.h"
+
+static ssize_t blk_crange_sector_show(struct blk_crange *cr, char *page)
+{
+	return sprintf(page, "%llu\n", cr->sector);
+}
+
+static ssize_t blk_crange_nr_sectors_show(struct blk_crange *cr, char *page)
+{
+	return sprintf(page, "%llu\n", cr->nr_sectors);
+}
+
+struct blk_crange_sysfs_entry {
+	struct attribute attr;
+	ssize_t (*show)(struct blk_crange *cr, char *page);
+};
+
+static struct blk_crange_sysfs_entry blk_crange_sector_entry = {
+	.attr = { .name = "sector", .mode = 0444 },
+	.show = blk_crange_sector_show,
+};
+
+static struct blk_crange_sysfs_entry blk_crange_nr_sectors_entry = {
+	.attr = { .name = "nr_sectors", .mode = 0444 },
+	.show = blk_crange_nr_sectors_show,
+};
+
+static struct attribute *blk_crange_attrs[] = {
+	&blk_crange_sector_entry.attr,
+	&blk_crange_nr_sectors_entry.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(blk_crange);
+
+static ssize_t blk_crange_sysfs_show(struct kobject *kobj,
+				     struct attribute *attr, char *page)
+{
+	struct blk_crange_sysfs_entry *entry =
+		container_of(attr, struct blk_crange_sysfs_entry, attr);
+	struct blk_crange *cr = container_of(kobj, struct blk_crange, kobj);
+	ssize_t ret;
+
+	mutex_lock(&cr->queue->sysfs_lock);
+	ret = entry->show(cr, page);
+	mutex_unlock(&cr->queue->sysfs_lock);
+
+	return ret;
+}
+
+static const struct sysfs_ops blk_crange_sysfs_ops = {
+	.show	= blk_crange_sysfs_show,
+};
+
+/*
+ * crange entries are not freed individually, but alltogether with the
+ * struct blk_cranges and its array of range entries. since kobject_add()
+ * takes a reference on the parent struct blk_cranges kobj, the array of
+ * crange entries cannot be freed until kobject_del() is called for all entries.
+ * So we do not need to do anything here, but still need this nop release
+ * operation to avoid complaints from the kobject code.
+ */
+static void blk_crange_sysfs_nop_release(struct kobject *kobj)
+{
+}
+
+static struct kobj_type blk_crange_ktype = {
+	.sysfs_ops	= &blk_crange_sysfs_ops,
+	.default_groups	= blk_crange_groups,
+	.release	= blk_crange_sysfs_nop_release,
+};
+
+/*
+ * This will be executed only after all range entries are removed
+ * with kobject_del(), at which point, it is safe to free everything,
+ * including the array of range entries.
+ */
+static void blk_cranges_sysfs_release(struct kobject *kobj)
+{
+	struct blk_cranges *cranges =
+		container_of(kobj, struct blk_cranges, kobj);
+
+	kfree(cranges);
+}
+
+static struct kobj_type blk_cranges_ktype = {
+	.release	= blk_cranges_sysfs_release,
+};
+
+/**
+ * disk_register_cranges - register with sysfs a set of concurrent ranges
+ * @disk:		Target disk
+ * @new_cranges:	New set of concurrent ranges
+ *
+ * Register with sysfs a set of concurrent ranges for @disk. If @new_cranges
+ * is not NULL, this set of concurrent ranges is registered and the
+ * old set specified by q->cranges is unregistered. Otherwise, q->cranges
+ * is registered if it is not already.
+ */
+int disk_register_cranges(struct gendisk *disk, struct blk_cranges *new_cranges)
+{
+	struct request_queue *q = disk->queue;
+	struct blk_cranges *cranges;
+	int i, ret;
+
+	lockdep_assert_held(&q->sysfs_dir_lock);
+	lockdep_assert_held(&q->sysfs_lock);
+
+	/* If a new range set is specified, unregister the old one */
+	if (new_cranges) {
+		if (q->cranges)
+			disk_unregister_cranges(disk);
+		q->cranges = new_cranges;
+	}
+
+	cranges = q->cranges;
+	if (!cranges)
+		return 0;
+
+	/*
+	 * At this point, cranges is the new set of sector ranges that needs
+	 * to be registered with sysfs.
+	 */
+	WARN_ON(cranges->sysfs_registered);
+	ret = kobject_init_and_add(&cranges->kobj, &blk_cranges_ktype,
+				   &q->kobj, "%s", "cranges");
+	if (ret) {
+		q->cranges = NULL;
+		kfree(cranges);
+		return ret;
+	}
+
+	for (i = 0; i < cranges->nr_ranges; i++) {
+		cranges->ranges[i].queue = q;
+		ret = kobject_init_and_add(&cranges->ranges[i].kobj,
+					   &blk_crange_ktype, &cranges->kobj,
+					   "%d", i);
+		if (ret) {
+			while (--i >= 0)
+				kobject_del(&cranges->ranges[i].kobj);
+			kobject_del(&cranges->kobj);
+			kobject_put(&cranges->kobj);
+			return ret;
+		}
+	}
+
+	cranges->sysfs_registered = true;
+
+	return 0;
+}
+
+void disk_unregister_cranges(struct gendisk *disk)
+{
+	struct request_queue *q = disk->queue;
+	struct blk_cranges *cranges = q->cranges;
+	int i;
+
+	lockdep_assert_held(&q->sysfs_dir_lock);
+	lockdep_assert_held(&q->sysfs_lock);
+
+	if (!cranges)
+		return;
+
+	if (cranges->sysfs_registered) {
+		for (i = 0; i < cranges->nr_ranges; i++)
+			kobject_del(&cranges->ranges[i].kobj);
+		kobject_del(&cranges->kobj);
+		kobject_put(&cranges->kobj);
+	} else {
+		kfree(cranges);
+	}
+
+	q->cranges = NULL;
+}
+
+static bool disk_check_ranges(struct gendisk *disk, struct blk_cranges *cr)
+{
+	sector_t capacity = get_capacity(disk);
+	sector_t min_sector = (sector_t)-1;
+	sector_t max_sector = 0;
+	int i;
+
+	/*
+	 * Sector ranges may overlap but should overall contain all sectors
+	 * within the disk capacity.
+	 */
+	for (i = 0; i < cr->nr_ranges; i++) {
+		min_sector = min(min_sector, cr->ranges[i].sector);
+		max_sector = max(max_sector, cr->ranges[i].sector +
+					     cr->ranges[i].nr_sectors);
+	}
+
+	if (min_sector != 0 || max_sector < capacity) {
+		pr_warn("Invalid concurrent ranges: missing sectors\n");
+		return false;
+	}
+
+	if (max_sector > capacity) {
+		pr_warn("Invalid concurrent ranges: beyond capacity\n");
+		return false;
+	}
+
+	return true;
+}
+
+static bool disk_cranges_changed(struct gendisk *disk, struct blk_cranges *new)
+{
+	struct blk_cranges *old = disk->queue->cranges;
+	int i;
+
+	if (!old)
+		return true;
+
+	if (old->nr_ranges != new->nr_ranges)
+		return true;
+
+	for (i = 0; i < old->nr_ranges; i++) {
+		if (new->ranges[i].sector != old->ranges[i].sector ||
+		    new->ranges[i].nr_sectors != old->ranges[i].nr_sectors)
+			return true;
+	}
+
+	return false;
+}
+
+/**
+ * disk_alloc_cranges - Allocate a concurrent positioning range structure
+ * @disk:	target disk
+ * @nr_ranges:	Number of concurrent ranges
+ *
+ * Allocate a struct blk_cranges structure with @nr_ranges range descriptors.
+ */
+struct blk_cranges *disk_alloc_cranges(struct gendisk *disk, int nr_ranges)
+{
+	struct blk_cranges *cr;
+
+	cr = kzalloc_node(struct_size(cr, ranges, nr_ranges), GFP_KERNEL,
+			  disk->queue->node);
+	if (cr)
+		cr->nr_ranges = nr_ranges;
+	return cr;
+}
+EXPORT_SYMBOL_GPL(disk_alloc_cranges);
+
+/**
+ * disk_set_cranges - Set a disk concurrent positioning ranges
+ * @disk:	target disk
+ * @cr:		concurrent ranges structure
+ *
+ * Set the concurrant positioning ranges information of the request queue
+ * of @disk to @cr. If @cr is NULL and the concurrent ranges structure
+ * already set, if any, is cleared. If there are no differences between
+ * @cr and the concurrent ranges structure already set, @cr is freed.
+ */
+void disk_set_cranges(struct gendisk *disk, struct blk_cranges *cr)
+{
+	struct request_queue *q = disk->queue;
+
+	if (WARN_ON_ONCE(cr && !cr->nr_ranges)) {
+		kfree(cr);
+		cr = NULL;
+	}
+
+	mutex_lock(&q->sysfs_dir_lock);
+	mutex_lock(&q->sysfs_lock);
+
+	if (cr) {
+		if (!disk_check_ranges(disk, cr)) {
+			kfree(cr);
+			cr = NULL;
+			goto reg;
+		}
+
+		if (!disk_cranges_changed(disk, cr)) {
+			kfree(cr);
+			goto unlock;
+		}
+	}
+
+	/*
+	 * This may be called for a registered queue. E.g. during a device
+	 * revalidation. If that is the case, we need to unregister the old
+	 * set of concurrent ranges and register the new set. If the queue
+	 * is not registered, the device request queue registration will
+	 * register the ranges, so only swap in the new set and free the
+	 * old one.
+	 */
+reg:
+	if (blk_queue_registered(q)) {
+		disk_register_cranges(disk, cr);
+	} else {
+		swap(q->cranges, cr);
+		kfree(cr);
+	}
+
+unlock:
+	mutex_unlock(&q->sysfs_lock);
+	mutex_unlock(&q->sysfs_dir_lock);
+}
+EXPORT_SYMBOL_GPL(disk_set_cranges);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 1832587dce3a..be8e02356a26 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -897,16 +897,15 @@ int blk_register_queue(struct gendisk *disk)
 	}
 
 	mutex_lock(&q->sysfs_lock);
+
+	ret = disk_register_cranges(disk, NULL);
+	if (ret)
+		goto put_dev;
+
 	if (q->elevator) {
 		ret = elv_register_queue(q, false);
-		if (ret) {
-			mutex_unlock(&q->sysfs_lock);
-			mutex_unlock(&q->sysfs_dir_lock);
-			kobject_del(&q->kobj);
-			blk_trace_remove_sysfs(dev);
-			kobject_put(&dev->kobj);
-			return ret;
-		}
+		if (ret)
+			goto put_dev;
 	}
 
 	blk_queue_flag_set(QUEUE_FLAG_REGISTERED, q);
@@ -937,6 +936,16 @@ int blk_register_queue(struct gendisk *disk)
 		percpu_ref_switch_to_percpu(&q->q_usage_counter);
 	}
 
+	return ret;
+
+put_dev:
+	disk_unregister_cranges(disk);
+	mutex_unlock(&q->sysfs_lock);
+	mutex_unlock(&q->sysfs_dir_lock);
+	kobject_del(&q->kobj);
+	blk_trace_remove_sysfs(dev);
+	kobject_put(&dev->kobj);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(blk_register_queue);
@@ -983,6 +992,7 @@ void blk_unregister_queue(struct gendisk *disk)
 	mutex_lock(&q->sysfs_lock);
 	if (q->elevator)
 		elv_unregister_queue(q);
+	disk_unregister_cranges(disk);
 	mutex_unlock(&q->sysfs_lock);
 	mutex_unlock(&q->sysfs_dir_lock);
 
diff --git a/block/blk.h b/block/blk.h
index 56f33fbcde59..149cd5ef8eeb 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -367,4 +367,8 @@ extern struct device_attribute dev_attr_events;
 extern struct device_attribute dev_attr_events_async;
 extern struct device_attribute dev_attr_events_poll_msecs;
 
+int disk_register_cranges(struct gendisk *disk,
+			  struct blk_cranges *new_cranges);
+void disk_unregister_cranges(struct gendisk *disk);
+
 #endif /* BLK_INTERNAL_H */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 07eef02325b4..476fc5104a95 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -377,6 +377,29 @@ static inline int blkdev_zone_mgmt_ioctl(struct block_device *bdev,
 
 #endif /* CONFIG_BLK_DEV_ZONED */
 
+/*
+ * Concurrent sector ranges: struct blk_crange describes range of
+ * contiguous sectors that can be served by independent resources on the
+ * device. The set of ranges defined in struct blk_cranges must overall
+ * include all sectors within the device capacity.
+ * For a device with multiple ranges, e.g. a single LUN multi-actuator HDD,
+ * requests targeting sectors in different ranges can be executed in parallel.
+ * A request can straddle a range boundary.
+ */
+struct blk_crange {
+	struct kobject		kobj;
+	struct request_queue	*queue;
+	sector_t		sector;
+	sector_t		nr_sectors;
+};
+
+struct blk_cranges {
+	struct kobject		kobj;
+	bool			sysfs_registered;
+	unsigned int		nr_ranges;
+	struct blk_crange	ranges[];
+};
+
 struct request_queue {
 	struct request		*last_merge;
 	struct elevator_queue	*elevator;
@@ -567,6 +590,9 @@ struct request_queue {
 
 #define BLK_MAX_WRITE_HINTS	5
 	u64			write_hints[BLK_MAX_WRITE_HINTS];
+
+	/* Concurrent sector ranges */
+	struct blk_cranges	*cranges;
 };
 
 /* Keep blk_queue_flag_name[] in sync with the definitions below */
@@ -1161,6 +1187,9 @@ extern void blk_queue_required_elevator_features(struct request_queue *q,
 extern bool blk_queue_can_use_dma_map_merging(struct request_queue *q,
 					      struct device *dev);
 
+struct blk_cranges *disk_alloc_cranges(struct gendisk *disk, int nr_ranges);
+void disk_set_cranges(struct gendisk *disk, struct blk_cranges *cr);
+
 /*
  * Number of physical segments as sent to the device.
  *
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v5 2/5] scsi: sd: add concurrent positioning ranges support
  2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
  2021-08-12  7:50 ` [PATCH v5 1/5] block: Add concurrent positioning ranges support Damien Le Moal
@ 2021-08-12  7:50 ` Damien Le Moal
  2021-08-12  7:50 ` [PATCH v5 3/5] libata: support concurrent positioning ranges log Damien Le Moal
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-12  7:50 UTC (permalink / raw)
  To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi

Add the sd_read_cpr() function to the sd scsi disk driver to discover
if a device has multiple concurrent positioning ranges (i.e. multiple
actuators on an HDD). This new function is called from
sd_revalidate_disk() and uses the block layer functions
blk_alloc_cranges() and blk_queue_set_cranges() to set a device
cranges according to the information retrieved from log page B9h,
if the device supports it.

The format of the Concurrent Positioning Ranges VPD page B9h is defined
in section 6.6.6 of SBC-5.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/sd.h |  1 +
 2 files changed, 82 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b8d55af763f9..5480d75f4883 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3125,6 +3125,86 @@ static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer)
 		sdkp->security = 1;
 }
 
+static inline sector_t sd64_to_sectors(struct scsi_disk *sdkp, u8 *buf)
+{
+	return logical_to_sectors(sdkp->device, get_unaligned_be64(buf));
+}
+
+/**
+ * sd_read_cpr - Query concurrent positioning ranges
+ * @sdkp:	disk to query
+ */
+static void sd_read_cpr(struct scsi_disk *sdkp)
+{
+	unsigned char *buffer = NULL;
+	struct blk_cranges *cr = NULL;
+	unsigned int nr_cpr = 0;
+	int i, vpd_len, buf_len = SD_BUF_SIZE;
+	u8 *desc;
+
+	/*
+	 * We need to have the capacity set first for the block layer to be
+	 * able to check the ranges.
+	 */
+	if (sdkp->first_scan)
+		return;
+
+	if (!sdkp->capacity)
+		goto out;
+
+	/*
+	 * Concurrent Positioning Ranges VPD: there can be at most 256 ranges,
+	 * leading to a maximum page size of 64 + 256*32 bytes.
+	 */
+	buf_len = 64 + 256*32;
+	buffer = kmalloc(buf_len, GFP_KERNEL);
+	if (!buffer || scsi_get_vpd_page(sdkp->device, 0xb9, buffer, buf_len))
+		goto out;
+
+	/* We must have at least a 64B header and one 32B range descriptor */
+	vpd_len = get_unaligned_be16(&buffer[2]) + 3;
+	if (vpd_len > buf_len || vpd_len < 64 + 32 || (vpd_len & 31)) {
+		sd_printk(KERN_ERR, sdkp,
+			  "Invalid Concurrent Positioning Ranges VPD page\n");
+		goto out;
+	}
+
+	nr_cpr = (vpd_len - 64) / 32;
+	if (nr_cpr == 1) {
+		nr_cpr = 0;
+		goto out;
+	}
+
+	cr = disk_alloc_cranges(sdkp->disk, nr_cpr);
+	if (!cr) {
+		nr_cpr = 0;
+		goto out;
+	}
+
+	desc = &buffer[64];
+	for (i = 0; i < nr_cpr; i++, desc += 32) {
+		if (desc[0] != i) {
+			sd_printk(KERN_ERR, sdkp,
+				"Invalid Concurrent Positioning Range number\n");
+			nr_cpr = 0;
+			break;
+		}
+
+		cr->ranges[i].sector = sd64_to_sectors(sdkp, desc + 8);
+		cr->ranges[i].nr_sectors = sd64_to_sectors(sdkp, desc + 16);
+	}
+
+out:
+	disk_set_cranges(sdkp->disk, cr);
+	if (nr_cpr && sdkp->nr_actuators != nr_cpr) {
+		sd_printk(KERN_NOTICE, sdkp,
+			  "%u concurrent positioning ranges\n", nr_cpr);
+		sdkp->nr_actuators = nr_cpr;
+	}
+
+	kfree(buffer);
+}
+
 /*
  * Determine the device's preferred I/O size for reads and writes
  * unless the reported value is unreasonably small, large, not a
@@ -3240,6 +3320,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 		sd_read_app_tag_own(sdkp, buffer);
 		sd_read_write_same(sdkp, buffer);
 		sd_read_security(sdkp, buffer);
+		sd_read_cpr(sdkp);
 	}
 
 	/*
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index b59136c4125b..2e5932bde43d 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -106,6 +106,7 @@ struct scsi_disk {
 	u8		protection_type;/* Data Integrity Field */
 	u8		provisioning_mode;
 	u8		zeroing_mode;
+	u8		nr_actuators;		/* Number of actuators */
 	unsigned	ATO : 1;	/* state of disk ATO bit */
 	unsigned	cache_override : 1; /* temp override of WCE,RCD */
 	unsigned	WCE : 1;	/* state of disk WCE bit */
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v5 3/5] libata: support concurrent positioning ranges log
  2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
  2021-08-12  7:50 ` [PATCH v5 1/5] block: Add concurrent positioning ranges support Damien Le Moal
  2021-08-12  7:50 ` [PATCH v5 2/5] scsi: sd: add " Damien Le Moal
@ 2021-08-12  7:50 ` Damien Le Moal
  2021-08-12  7:50 ` [PATCH v5 4/5] doc: document sysfs queue/cranges attributes Damien Le Moal
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-12  7:50 UTC (permalink / raw)
  To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi

Add support to discover if an ATA device supports the Concurrent
Positioning Ranges Log (address 0x47), indicating that the device is
capable of seeking to multiple different locations in parallel using
multiple actuators serving different LBA ranges.

Also add support to translate the concurrent positioning ranges log
into its equivalent Concurrent Positioning Ranges VPD page B9h in
libata-scsi.c.

The format of the Concurrent Positioning Ranges Log is defined in ACS-5
r9.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libata-core.c | 52 +++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-scsi.c | 48 +++++++++++++++++++++++++++++-------
 include/linux/ata.h       |  1 +
 include/linux/libata.h    | 15 +++++++++++
 4 files changed, 107 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 61c762961ca8..ab3f61ea743e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2363,6 +2363,57 @@ static void ata_dev_config_trusted(struct ata_device *dev)
 		dev->flags |= ATA_DFLAG_TRUSTED;
 }
 
+static void ata_dev_config_cpr(struct ata_device *dev)
+{
+	unsigned int err_mask;
+	size_t buf_len;
+	int i, nr_cpr = 0;
+	struct ata_cpr_log *cpr_log = NULL;
+	u8 *desc, *buf = NULL;
+
+	if (!ata_identify_page_supported(dev,
+				 ATA_LOG_CONCURRENT_POSITIONING_RANGES))
+		goto out;
+
+	/*
+	 * Read IDENTIFY DEVICE data log, page 0x47
+	 * (concurrent positioning ranges). We can have at most 255 32B range
+	 * descriptors plus a 64B header.
+	 */
+	buf_len = (64 + 255 * 32 + 511) & ~511;
+	buf = kzalloc(buf_len, GFP_KERNEL);
+	if (!buf)
+		goto out;
+
+	err_mask = ata_read_log_page(dev, ATA_LOG_IDENTIFY_DEVICE,
+				     ATA_LOG_CONCURRENT_POSITIONING_RANGES,
+				     buf, buf_len >> 9);
+	if (err_mask)
+		goto out;
+
+	nr_cpr = buf[0];
+	if (!nr_cpr)
+		goto out;
+
+	cpr_log = kzalloc(struct_size(cpr_log, cpr, nr_cpr), GFP_KERNEL);
+	if (!cpr_log)
+		goto out;
+
+	cpr_log->nr_cpr = nr_cpr;
+	desc = &buf[64];
+	for (i = 0; i < nr_cpr; i++, desc += 32) {
+		cpr_log->cpr[i].num = desc[0];
+		cpr_log->cpr[i].num_storage_elements = desc[1];
+		cpr_log->cpr[i].start_lba = get_unaligned_le64(&desc[8]);
+		cpr_log->cpr[i].num_lbas = get_unaligned_le64(&desc[16]);
+	}
+
+out:
+	swap(dev->cpr_log, cpr_log);
+	kfree(cpr_log);
+	kfree(buf);
+}
+
 /**
  *	ata_dev_configure - Configure the specified ATA/ATAPI device
  *	@dev: Target device to configure
@@ -2591,6 +2642,7 @@ int ata_dev_configure(struct ata_device *dev)
 		ata_dev_config_sense_reporting(dev);
 		ata_dev_config_zac(dev);
 		ata_dev_config_trusted(dev);
+		ata_dev_config_cpr(dev);
 		dev->cdb_len = 32;
 	}
 
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b9588c52815d..5cadbb9a8bf2 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1937,7 +1937,7 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
  */
 static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf)
 {
-	int num_pages;
+	int i, num_pages = 0;
 	static const u8 pages[] = {
 		0x00,	/* page 0x00, this page */
 		0x80,	/* page 0x80, unit serial no page */
@@ -1947,13 +1947,17 @@ static unsigned int ata_scsiop_inq_00(struct ata_scsi_args *args, u8 *rbuf)
 		0xb1,	/* page 0xb1, block device characteristics page */
 		0xb2,	/* page 0xb2, thin provisioning page */
 		0xb6,	/* page 0xb6, zoned block device characteristics */
+		0xb9,	/* page 0xb9, concurrent positioning ranges */
 	};
 
-	num_pages = sizeof(pages);
-	if (!(args->dev->flags & ATA_DFLAG_ZAC))
-		num_pages--;
+	for (i = 0; i < sizeof(pages); i++) {
+		if (pages[i] == 0xb6 &&
+		    !(args->dev->flags & ATA_DFLAG_ZAC))
+			continue;
+		rbuf[num_pages + 4] = pages[i];
+		num_pages++;
+	}
 	rbuf[3] = num_pages;	/* number of supported VPD pages */
-	memcpy(rbuf + 4, pages, num_pages);
 	return 0;
 }
 
@@ -2163,6 +2167,26 @@ static unsigned int ata_scsiop_inq_b6(struct ata_scsi_args *args, u8 *rbuf)
 	return 0;
 }
 
+static unsigned int ata_scsiop_inq_b9(struct ata_scsi_args *args, u8 *rbuf)
+{
+	struct ata_cpr_log *cpr_log = args->dev->cpr_log;
+	u8 *desc = &rbuf[64];
+	int i;
+
+	/* SCSI Concurrent Positioning Ranges VPD page: SBC-5 rev 1 or later */
+	rbuf[1] = 0xb9;
+	put_unaligned_be16(64 + (int)cpr_log->nr_cpr * 32 - 4, &rbuf[3]);
+
+	for (i = 0; i < cpr_log->nr_cpr; i++, desc += 32) {
+		desc[0] = cpr_log->cpr[i].num;
+		desc[1] = cpr_log->cpr[i].num_storage_elements;
+		put_unaligned_be64(cpr_log->cpr[i].start_lba, &desc[8]);
+		put_unaligned_be64(cpr_log->cpr[i].num_lbas, &desc[16]);
+	}
+
+	return 0;
+}
+
 /**
  *	modecpy - Prepare response for MODE SENSE
  *	@dest: output buffer
@@ -4162,11 +4186,17 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
 			ata_scsi_rbuf_fill(&args, ata_scsiop_inq_b2);
 			break;
 		case 0xb6:
-			if (dev->flags & ATA_DFLAG_ZAC) {
+			if (dev->flags & ATA_DFLAG_ZAC)
 				ata_scsi_rbuf_fill(&args, ata_scsiop_inq_b6);
-				break;
-			}
-			fallthrough;
+			else
+				ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
+			break;
+		case 0xb9:
+			if (dev->cpr_log)
+				ata_scsi_rbuf_fill(&args, ata_scsiop_inq_b9);
+			else
+				ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
+			break;
 		default:
 			ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
 			break;
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 1b44f40c7700..199e47e97d64 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -329,6 +329,7 @@ enum {
 	ATA_LOG_SECURITY	  = 0x06,
 	ATA_LOG_SATA_SETTINGS	  = 0x08,
 	ATA_LOG_ZONED_INFORMATION = 0x09,
+	ATA_LOG_CONCURRENT_POSITIONING_RANGES = 0x47,
 
 	/* Identify device SATA settings log:*/
 	ATA_LOG_DEVSLP_OFFSET	  = 0x30,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3fcd24236793..b159a245d88c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -670,6 +670,18 @@ struct ata_ering {
 	struct ata_ering_entry	ring[ATA_ERING_SIZE];
 };
 
+struct ata_cpr {
+	u8			num;
+	u8			num_storage_elements;
+	u64			start_lba;
+	u64			num_lbas;
+};
+
+struct ata_cpr_log {
+	u8			nr_cpr;
+	struct ata_cpr		cpr[];
+};
+
 struct ata_device {
 	struct ata_link		*link;
 	unsigned int		devno;		/* 0 or 1 */
@@ -729,6 +741,9 @@ struct ata_device {
 	u32			zac_zones_optimal_nonseq;
 	u32			zac_zones_max_open;
 
+	/* Concurrent positioning ranges */
+	struct ata_cpr_log	*cpr_log;
+
 	/* error history */
 	int			spdn_cnt;
 	/* ering is CLEAR_END, read comment above CLEAR_END */
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v5 4/5] doc: document sysfs queue/cranges attributes
  2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
                   ` (2 preceding siblings ...)
  2021-08-12  7:50 ` [PATCH v5 3/5] libata: support concurrent positioning ranges log Damien Le Moal
@ 2021-08-12  7:50 ` Damien Le Moal
  2021-08-12  7:50 ` [PATCH v5 5/5] doc: Fix typo in request queue sysfs documentation Damien Le Moal
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-12  7:50 UTC (permalink / raw)
  To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi

Update the file Documentation/block/queue-sysfs.rst to add a description
of a device queue sysfs entries related to concurrent sector ranges
(e.g. concurrent positioning ranges for multi-actuator hard-disks).

While at it, also fix a typo in this file introduction paragraph.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/block/queue-sysfs.rst | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/block/queue-sysfs.rst b/Documentation/block/queue-sysfs.rst
index 4dc7f0d499a8..757609bbb1e2 100644
--- a/Documentation/block/queue-sysfs.rst
+++ b/Documentation/block/queue-sysfs.rst
@@ -286,4 +286,32 @@ sequential zones of zoned block devices (devices with a zoned attributed
 that reports "host-managed" or "host-aware"). This value is always 0 for
 regular block devices.
 
+cranges (RO)
+------------
+
+The presence of this sub-directory of the /sys/block/xxx/queue/ directory
+indicates that the device is capable of executing requests targeting
+different sector ranges in parallel. For instance, single LUN multi-actuator
+hard-disks will likely have a cranges directory if the device correctly
+advertizes the sector ranges of its actuators.
+
+The cranges directory contains one directory per concurrent range, with each
+range described using the sector (RO) attribute file to indicate the first
+sector of the range and the nr_sectors (RO) attribute file to indicate the
+total number of sector in the range starting from the first sector.
+For example, a dual-actuator hard disk will have the following cranges
+entries.::
+
+        $ tree /sys/block/<device>/queue/cranges/
+        /sys/block/<device>/queue/cranges/
+        |-- 0
+        |   |-- nr_sectors
+        |   `-- sector
+        `-- 1
+            |-- nr_sectors
+            `-- sector
+
+The sector and nr_sectors attributes use 512B sector unit, regardless of
+the actual block size of the device.
+
 Jens Axboe <jens.axboe@oracle.com>, February 2009
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v5 5/5] doc: Fix typo in request queue sysfs documentation
  2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
                   ` (3 preceding siblings ...)
  2021-08-12  7:50 ` [PATCH v5 4/5] doc: document sysfs queue/cranges attributes Damien Le Moal
@ 2021-08-12  7:50 ` Damien Le Moal
  2021-08-12 17:10 ` [PATCH v5 0/5] Initial support for multi-actuator HDDs Jens Axboe
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-12  7:50 UTC (permalink / raw)
  To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi

Fix a typo (are -> as) in the introduction paragraph of
Documentation/block/queue-sysfs.rst.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 Documentation/block/queue-sysfs.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/block/queue-sysfs.rst b/Documentation/block/queue-sysfs.rst
index 757609bbb1e2..25f4a768f450 100644
--- a/Documentation/block/queue-sysfs.rst
+++ b/Documentation/block/queue-sysfs.rst
@@ -4,7 +4,7 @@ Queue sysfs files
 
 This text file will detail the queue files that are located in the sysfs tree
 for each block device. Note that stacked devices typically do not export
-any settings, since their queue merely functions are a remapping target.
+any settings, since their queue merely functions as a remapping target.
 These files are the ones found in the /sys/block/xxx/queue/ directory.
 
 Files denoted with a RO postfix are readonly and the RW postfix means
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
                   ` (4 preceding siblings ...)
  2021-08-12  7:50 ` [PATCH v5 5/5] doc: Fix typo in request queue sysfs documentation Damien Le Moal
@ 2021-08-12 17:10 ` Jens Axboe
  2021-08-12 17:21   ` Martin K. Petersen
  2021-08-16 17:10 ` Martin K. Petersen
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 25+ messages in thread
From: Jens Axboe @ 2021-08-12 17:10 UTC (permalink / raw)
  To: Damien Le Moal, linux-block, Martin K . Petersen, linux-scsi

On 8/12/21 1:50 AM, Damien Le Moal wrote:
> Single LUN multi-actuator hard-disks are cappable to seek and execute
> multiple commands in parallel. This capability is exposed to the host
> using the Concurrent Positioning Ranges VPD page (SCSI) and Log (ATA).
> Each positioning range describes the contiguous set of LBAs that an
> actuator serves.
> 
> This series adds support the scsi disk driver to retreive this
> information and advertize it to user space through sysfs. libata is also
> modified to handle ATA drives.
> 
> The first patch adds the block layer plumbing to expose concurrent
> sector ranges of the device through sysfs as a sub-directory of the
> device sysfs queue directory. Patch 2 and 3 add support to sd and
> libata. Finally patch 4 documents the sysfs queue attributed changes.
> Patch 5 fixes a typo in the document file (strictly speaking, not
> related to this series).
> 
> This series does not attempt in any way to optimize accesses to
> multi-actuator devices (e.g. block IO scheduler or filesystems). This
> initial support only exposes the actuators information to user space
> through sysfs.

This looks good to me now - are we taking this through the block tree?
If so, would be nice to get a SCSI signoff on patch 2.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-12 17:10 ` [PATCH v5 0/5] Initial support for multi-actuator HDDs Jens Axboe
@ 2021-08-12 17:21   ` Martin K. Petersen
  2021-08-12 17:25     ` Jens Axboe
  0 siblings, 1 reply; 25+ messages in thread
From: Martin K. Petersen @ 2021-08-12 17:21 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Damien Le Moal, linux-block, Martin K . Petersen, linux-scsi


Jens,

> This looks good to me now - are we taking this through the block tree?
> If so, would be nice to get a SCSI signoff on patch 2.

I'd like to do one more review pass. Also, it may be best to take patch
2 through my tree since I suspect it will interfere with my VPD shuffle.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-12 17:21   ` Martin K. Petersen
@ 2021-08-12 17:25     ` Jens Axboe
  2021-08-13  0:30       ` Damien Le Moal
  0 siblings, 1 reply; 25+ messages in thread
From: Jens Axboe @ 2021-08-12 17:25 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: Damien Le Moal, linux-block, linux-scsi

On 8/12/21 11:21 AM, Martin K. Petersen wrote:
> 
> Jens,
> 
>> This looks good to me now - are we taking this through the block tree?
>> If so, would be nice to get a SCSI signoff on patch 2.
> 
> I'd like to do one more review pass. Also, it may be best to take patch
> 2 through my tree since I suspect it will interfere with my VPD shuffle.

That's fine, we can do it like that. I'll wait a day or two and pending
any last minute changes, take 1,3-5 through block.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-12 17:25     ` Jens Axboe
@ 2021-08-13  0:30       ` Damien Le Moal
  2021-08-16 17:12         ` Martin K. Petersen
  0 siblings, 1 reply; 25+ messages in thread
From: Damien Le Moal @ 2021-08-13  0:30 UTC (permalink / raw)
  To: Jens Axboe, Martin K. Petersen; +Cc: linux-block, linux-scsi

On 2021/08/13 2:25, Jens Axboe wrote:
> On 8/12/21 11:21 AM, Martin K. Petersen wrote:
>>
>> Jens,
>>
>>> This looks good to me now - are we taking this through the block tree?
>>> If so, would be nice to get a SCSI signoff on patch 2.
>>
>> I'd like to do one more review pass. Also, it may be best to take patch
>> 2 through my tree since I suspect it will interfere with my VPD shuffle.
> 
> That's fine, we can do it like that. I'll wait a day or two and pending
> any last minute changes, take 1,3-5 through block.

Jens, Martin,

Thanks.

Without patch 1, patch 2 will cause compilation errors (disk_alloc_cranges() and
disk_register_cranges() will be undefined). So may be both patch 1 & 2 should
got through the scsi tree ?


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
                   ` (5 preceding siblings ...)
  2021-08-12 17:10 ` [PATCH v5 0/5] Initial support for multi-actuator HDDs Jens Axboe
@ 2021-08-16 17:10 ` Martin K. Petersen
  2021-08-17  4:06   ` Damien Le Moal
  2021-08-23  1:27 ` Damien Le Moal
  2021-08-26  2:07 ` Damien Le Moal
  8 siblings, 1 reply; 25+ messages in thread
From: Martin K. Petersen @ 2021-08-16 17:10 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi


Hi Damien!

> Single LUN multi-actuator hard-disks are cappable to seek and execute
> multiple commands in parallel. This capability is exposed to the host
> using the Concurrent Positioning Ranges VPD page (SCSI) and Log (ATA).
> Each positioning range describes the contiguous set of LBAs that an
> actuator serves.

I am not a big fan of the Concurrent Positioning Range terminology since
it is very specific to the implementation of multi-actuator disk drives.
With other types of media, "positioning" doesn't any sense. It is
unfortunate that CPR is the term that ended up in a spec that covers a
wide variety of devices and media types.

I also think that the "concurrent positioning" emphasizes the
performance aspect but not so much the fault domain which in many ways
is the more interesting part.

The purpose of exposing this information to the filesystems must be to
encourage them to use it. And therefore I think it is important that the
semantics and information conveyed is applicable outside of the
multi-actuator use case. It would be easy to expose this kind of
information for concatenated LVM devices, etc.

Anyway. I don't really have any objections to the series from an
implementation perspective. I do think "cpr" as you used in patch #2 is
a better name than "crange". But again, I wish we could come up with a
more accurate and less disk actuator centric terminology for the block
layer plumbing.

I would have voted for "fault_domain" but that ignores the performance
aspect. "independent_block_range", maybe? Why is naming always so hard?
:(

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-13  0:30       ` Damien Le Moal
@ 2021-08-16 17:12         ` Martin K. Petersen
  2021-08-17  4:06           ` Damien Le Moal
  0 siblings, 1 reply; 25+ messages in thread
From: Martin K. Petersen @ 2021-08-16 17:12 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: Jens Axboe, Martin K. Petersen, linux-block, linux-scsi


Damien,

> Without patch 1, patch 2 will cause compilation errors
> (disk_alloc_cranges() and disk_register_cranges() will be
> undefined). So may be both patch 1 & 2 should got through the scsi
> tree ?

The sd patch would go into -rc2.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-16 17:10 ` Martin K. Petersen
@ 2021-08-17  4:06   ` Damien Le Moal
  2021-08-18  2:24     ` Martin K. Petersen
  0 siblings, 1 reply; 25+ messages in thread
From: Damien Le Moal @ 2021-08-17  4:06 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: Jens Axboe, linux-block, linux-scsi

On 2021/08/17 2:10, Martin K. Petersen wrote:
> 
> Hi Damien!
> 
>> Single LUN multi-actuator hard-disks are cappable to seek and execute
>> multiple commands in parallel. This capability is exposed to the host
>> using the Concurrent Positioning Ranges VPD page (SCSI) and Log (ATA).
>> Each positioning range describes the contiguous set of LBAs that an
>> actuator serves.
> 
> I am not a big fan of the Concurrent Positioning Range terminology since
> it is very specific to the implementation of multi-actuator disk drives.
> With other types of media, "positioning" doesn't any sense. It is
> unfortunate that CPR is the term that ended up in a spec that covers a
> wide variety of devices and media types.
> 
> I also think that the "concurrent positioning" emphasizes the
> performance aspect but not so much the fault domain which in many ways
> is the more interesting part.
> 
> The purpose of exposing this information to the filesystems must be to
> encourage them to use it. And therefore I think it is important that the
> semantics and information conveyed is applicable outside of the
> multi-actuator use case. It would be easy to expose this kind of
> information for concatenated LVM devices, etc.
> 
> Anyway. I don't really have any objections to the series from an
> implementation perspective. I do think "cpr" as you used in patch #2 is
> a better name than "crange". But again, I wish we could come up with a
> more accurate and less disk actuator centric terminology for the block
> layer plumbing.
> 
> I would have voted for "fault_domain" but that ignores the performance
> aspect. "independent_block_range", maybe? Why is naming always so hard?
> :(

I did struggle with the naming too and crange was the best I could come up with
given the specs wording.

With the single LUN approach, the fault domain does not really change from a
regular device. The typical use in case of bad heads would be to replace the
drive or reformat it at lower capacity with head depop. That could be avoided
with dm-linear on top (one DM per actuator) though.

As for the independent_block_range idea, I thought of that too, but the problem
is that the tags are still shared between the 2 actuators, so the ranges are not
really independent at all. One can starve the other depending on the host
workload without FS and/or IO scheduler optimization distributing the commands
between the actuators.

The above point led me to this informational only implementation. Without
optimization, we get the same as today. No changes in performance and use.
Better IOPS is gain for lucky workloads (typically random ones). Going forward,
more reliable IOPS & throughput gains are possible with some additional changes.


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-16 17:12         ` Martin K. Petersen
@ 2021-08-17  4:06           ` Damien Le Moal
  0 siblings, 0 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-17  4:06 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: Jens Axboe, linux-block, linux-scsi

On 2021/08/17 2:12, Martin K. Petersen wrote:
> 
> Damien,
> 
>> Without patch 1, patch 2 will cause compilation errors
>> (disk_alloc_cranges() and disk_register_cranges() will be
>> undefined). So may be both patch 1 & 2 should got through the scsi
>> tree ?
> 
> The sd patch would go into -rc2.

Makes sense. Thanks !


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-17  4:06   ` Damien Le Moal
@ 2021-08-18  2:24     ` Martin K. Petersen
  2021-08-18  2:45       ` Damien Le Moal
  0 siblings, 1 reply; 25+ messages in thread
From: Martin K. Petersen @ 2021-08-18  2:24 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: Martin K. Petersen, Jens Axboe, linux-block, linux-scsi


Damien,

> With the single LUN approach, the fault domain does not really change
> from a regular device. The typical use in case of bad heads would be
> to replace the drive or reformat it at lower capacity with head
> depop. That could be avoided with dm-linear on top (one DM per
> actuator) though.

I was more thinking along the lines of btrfs making sure to place
dup metadata on different actuators or similar.

> The above point led me to this informational only implementation.
> Without optimization, we get the same as today. No changes in
> performance and use.  Better IOPS is gain for lucky workloads
> (typically random ones). Going forward, more reliable IOPS &
> throughput gains are possible with some additional changes.

Sure, but that means the ranges need to affect both I/O scheduling and
data placement. We need to make sure that the data placement interface
semantics are applicable to other types of media than multi actuator
drives. Filesystems shouldn't have to look at different queue limits if
they sit on top of dm-linear instead of sd. From an I/O scheduling
perspective I concur that device implementation details are pertinent.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-18  2:24     ` Martin K. Petersen
@ 2021-08-18  2:45       ` Damien Le Moal
  0 siblings, 0 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-18  2:45 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: Jens Axboe, linux-block, linux-scsi

On 2021/08/18 11:24, Martin K. Petersen wrote:
> 
> Damien,
> 
>> With the single LUN approach, the fault domain does not really change
>> from a regular device. The typical use in case of bad heads would be
>> to replace the drive or reformat it at lower capacity with head
>> depop. That could be avoided with dm-linear on top (one DM per
>> actuator) though.
> 
> I was more thinking along the lines of btrfs making sure to place
> dup metadata on different actuators or similar.
> 
>> The above point led me to this informational only implementation.
>> Without optimization, we get the same as today. No changes in
>> performance and use.  Better IOPS is gain for lucky workloads
>> (typically random ones). Going forward, more reliable IOPS &
>> throughput gains are possible with some additional changes.
> 
> Sure, but that means the ranges need to affect both I/O scheduling and
> data placement. We need to make sure that the data placement interface
> semantics are applicable to other types of media than multi actuator
> drives. Filesystems shouldn't have to look at different queue limits if
> they sit on top of dm-linear instead of sd. From an I/O scheduling
> perspective I concur that device implementation details are pertinent.

Currently, FSes cannot know the dm-linear config. The queue crange interface can
actually unify this for split/dual actuator and dm-linear like logical disks.

I need to send patches to dm-devel for that though as currently, dm-linear does
not expose its config as cranges. If I recall correctly, Hannes was also
interested in playing with that too :)


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
                   ` (6 preceding siblings ...)
  2021-08-16 17:10 ` Martin K. Petersen
@ 2021-08-23  1:27 ` Damien Le Moal
  2021-08-26  2:07 ` Damien Le Moal
  8 siblings, 0 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-23  1:27 UTC (permalink / raw)
  To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi

On 2021/08/12 16:50, Damien Le Moal wrote:
> Single LUN multi-actuator hard-disks are cappable to seek and execute
> multiple commands in parallel. This capability is exposed to the host
> using the Concurrent Positioning Ranges VPD page (SCSI) and Log (ATA).
> Each positioning range describes the contiguous set of LBAs that an
> actuator serves.
> 
> This series adds support the scsi disk driver to retreive this
> information and advertize it to user space through sysfs. libata is also
> modified to handle ATA drives.
> 
> The first patch adds the block layer plumbing to expose concurrent
> sector ranges of the device through sysfs as a sub-directory of the
> device sysfs queue directory. Patch 2 and 3 add support to sd and
> libata. Finally patch 4 documents the sysfs queue attributed changes.
> Patch 5 fixes a typo in the document file (strictly speaking, not
> related to this series).
> 
> This series does not attempt in any way to optimize accesses to
> multi-actuator devices (e.g. block IO scheduler or filesystems). This
> initial support only exposes the actuators information to user space
> through sysfs.

Hi Jens,

I do not see this queued in for-5.15/block. Anything holding this series up ?

Thanks !




-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
                   ` (7 preceding siblings ...)
  2021-08-23  1:27 ` Damien Le Moal
@ 2021-08-26  2:07 ` Damien Le Moal
  2021-08-26  2:41   ` Martin K. Petersen
  8 siblings, 1 reply; 25+ messages in thread
From: Damien Le Moal @ 2021-08-26  2:07 UTC (permalink / raw)
  To: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi

On 2021/08/12 16:50, Damien Le Moal wrote:
> Single LUN multi-actuator hard-disks are cappable to seek and execute
> multiple commands in parallel. This capability is exposed to the host
> using the Concurrent Positioning Ranges VPD page (SCSI) and Log (ATA).
> Each positioning range describes the contiguous set of LBAs that an
> actuator serves.
> 
> This series adds support the scsi disk driver to retreive this
> information and advertize it to user space through sysfs. libata is also
> modified to handle ATA drives.
> 
> The first patch adds the block layer plumbing to expose concurrent
> sector ranges of the device through sysfs as a sub-directory of the
> device sysfs queue directory. Patch 2 and 3 add support to sd and
> libata. Finally patch 4 documents the sysfs queue attributed changes.
> Patch 5 fixes a typo in the document file (strictly speaking, not
> related to this series).
> 
> This series does not attempt in any way to optimize accesses to
> multi-actuator devices (e.g. block IO scheduler or filesystems). This
> initial support only exposes the actuators information to user space
> through sysfs.

Jens,

Re-ping ? Anything against this series ? Can we get it queued for 5.15 ?



> 
> Changes from v4:
> * Fixed kdoc comment function name mismatch for disk_register_cranges()
>   in patch 1
> 
> Changes from v3:
> * Modified patch 1:
>   - Prefix functions that take a struct gendisk as argument with
>     "disk_". Modified patch 2 accordingly.
>   - Added a functional release operation for struct blk_cranges kobj to
>     ensure that this structure is freed only after all references to it
>     are released, including kobject_del() execution for all crange sysfs
>     entries.
> * Added patch 5 to separate the typo fix from the crange documentation
>   addition.
> * Added reviewed-by tags
> 
> Changes from v2:
> * Update patch 1 to fix a compilation warning for a potential NULL
>   pointer dereference of the cr argument of blk_queue_set_cranges().
>   Warning reported by the kernel test robot <lkp@intel.com>).
> 
> Changes from v1:
> * Moved libata-scsi hunk from patch 1 to patch 3 where it belongs
> * Fixed unintialized variable in patch 2
>   Reported-by: kernel test robot <lkp@intel.com>
>   Reported-by: Dan Carpenter <dan.carpenter@oracle.com
> * Changed patch 3 adding struct ata_cpr_log to contain both the number
>   of concurrent ranges and the array of concurrent ranges.
> * Added a note in the documentation (patch 4) about the unit used for
>   the concurrent ranges attributes.
> 
> Damien Le Moal (5):
>   block: Add concurrent positioning ranges support
>   scsi: sd: add concurrent positioning ranges support
>   libata: support concurrent positioning ranges log
>   doc: document sysfs queue/cranges attributes
>   doc: Fix typo in request queue sysfs documentation
> 
>  Documentation/block/queue-sysfs.rst |  30 ++-
>  block/Makefile                      |   2 +-
>  block/blk-cranges.c                 | 310 ++++++++++++++++++++++++++++
>  block/blk-sysfs.c                   |  26 ++-
>  block/blk.h                         |   4 +
>  drivers/ata/libata-core.c           |  52 +++++
>  drivers/ata/libata-scsi.c           |  48 ++++-
>  drivers/scsi/sd.c                   |  81 ++++++++
>  drivers/scsi/sd.h                   |   1 +
>  include/linux/ata.h                 |   1 +
>  include/linux/blkdev.h              |  29 +++
>  include/linux/libata.h              |  15 ++
>  12 files changed, 580 insertions(+), 19 deletions(-)
>  create mode 100644 block/blk-cranges.c
> 


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-26  2:07 ` Damien Le Moal
@ 2021-08-26  2:41   ` Martin K. Petersen
  2021-08-26  3:09     ` Damien Le Moal
  0 siblings, 1 reply; 25+ messages in thread
From: Martin K. Petersen @ 2021-08-26  2:41 UTC (permalink / raw)
  To: Damien Le Moal; +Cc: Jens Axboe, linux-block, Martin K . Petersen, linux-scsi


Damien,

> Re-ping ? Anything against this series ? Can we get it queued for 5.15 ?

Wrt. the choice of 'crange':

https://lore.kernel.org/linux-fsdevel/CAHk-=wiZ=wwa4oAA0y=Kztafgp0n+BDTEV6ybLoH2nvLBeJBLA@mail.gmail.com/

I share Linus' sentiment.

I really think 'crange' is a horribly non-descriptive name compared to
logical_block_size, max_sectors_kb, discard_max_bytes, and the other
things we export.

In addition, the recently posted copy offload patches also used
'crange', in that context to describe a 'copy range'.

Anyway. Just my opinion.

Jens: Feel free to add my Acked-by: to the sd pieces. My SCSI discovery
rework won't make 5.15.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-26  2:41   ` Martin K. Petersen
@ 2021-08-26  3:09     ` Damien Le Moal
  2021-08-26  3:43       ` Martin K. Petersen
  0 siblings, 1 reply; 25+ messages in thread
From: Damien Le Moal @ 2021-08-26  3:09 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: Jens Axboe, linux-block, linux-scsi, Hannes Reinecke

On 2021/08/26 11:42, Martin K. Petersen wrote:
> 
> Damien,
> 
>> Re-ping ? Anything against this series ? Can we get it queued for 5.15 ?
> 
> Wrt. the choice of 'crange':
> 
> https://lore.kernel.org/linux-fsdevel/CAHk-=wiZ=wwa4oAA0y=Kztafgp0n+BDTEV6ybLoH2nvLBeJBLA@mail.gmail.com/

Yes, I have been reading this thread. Very good one.

> 
> I share Linus' sentiment.
> 
> I really think 'crange' is a horribly non-descriptive name compared to
> logical_block_size, max_sectors_kb, discard_max_bytes, and the other
> things we export.
> 
> In addition, the recently posted copy offload patches also used
> 'crange', in that context to describe a 'copy range'.

Yes, saw that too.

> Anyway. Just my opinion.

Thanks for sharing.

I am not super happy with the name either. I used this one as the least worst of
possibilities I thought of.
seek_range/srange ? -> that is very HDD centric and as we can reuse this for
things like dm-linear on top of SSDs, that does not really work.
I would prefer something that convey the idea of "parallel command execution",
since this is the main point of the interface. prange ? cdm_range ? req_range ?

Naming is really hard...

> Jens: Feel free to add my Acked-by: to the sd pieces. My SCSI discovery
> rework won't make 5.15.
> 


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-26  3:09     ` Damien Le Moal
@ 2021-08-26  3:43       ` Martin K. Petersen
  2021-08-26  3:50         ` Damien Le Moal
  0 siblings, 1 reply; 25+ messages in thread
From: Martin K. Petersen @ 2021-08-26  3:43 UTC (permalink / raw)
  To: Damien Le Moal
  Cc: Martin K. Petersen, Jens Axboe, linux-block, linux-scsi, Hannes Reinecke


Damien,

> I am not super happy with the name either. I used this one as the
> least worst of possibilities I thought of.  seek_range/srange ? ->
> that is very HDD centric and as we can reuse this for things like
> dm-linear on top of SSDs, that does not really work.  I would prefer
> something that convey the idea of "parallel command execution", since
> this is the main point of the interface. prange ? cdm_range ?
> req_range ?

How about independent_access_range? That doesn't imply head positioning
and can also be used to describe a fault domain. And it is less
disk-centric than concurrent_positioning_range.

I concur that those names are a bit unwieldy but at least they are
somewhat descriptive.

I consulted the thesaurus and didn't really like the other options
(discrete, disjoint, separate, etc.). I think 'independent' is more
accurate for this and better than 'concurrent' and 'parallel'.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-26  3:43       ` Martin K. Petersen
@ 2021-08-26  3:50         ` Damien Le Moal
  2021-08-26  6:28           ` Hannes Reinecke
  2021-08-27  3:03           ` Martin K. Petersen
  0 siblings, 2 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-26  3:50 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: Jens Axboe, linux-block, linux-scsi, Hannes Reinecke

On 2021/08/26 12:43, Martin K. Petersen wrote:
> 
> Damien,
> 
>> I am not super happy with the name either. I used this one as the
>> least worst of possibilities I thought of.  seek_range/srange ? ->
>> that is very HDD centric and as we can reuse this for things like
>> dm-linear on top of SSDs, that does not really work.  I would prefer
>> something that convey the idea of "parallel command execution", since
>> this is the main point of the interface. prange ? cdm_range ?
>> req_range ?
> 
> How about independent_access_range? That doesn't imply head positioning
> and can also be used to describe a fault domain. And it is less
> disk-centric than concurrent_positioning_range.

I like it, but a bit long-ish. Do you think shortening to access_range would be
acceptable ?

we would have:

/sys/block/XYZ/queue/access_ranges/...

and

struct blk_access_range {
	...
	sector_t sector;
	sector_t nr_sectors;
}

struct blk_access_range *arange;

Adding independent does make everything even more obvious, but names become
rather long. Not an issue for the sysfs directory I think, but

struct blk_independent_access_range {
	...
	sector_t sector;
	sector_t nr_sectors;
}

is rather a long struct name. Shortening independent to "ind" could very easily
be confused with "indirection", so that is not an option. And "iaccess" is
obscure...

> 
> I concur that those names are a bit unwieldy but at least they are
> somewhat descriptive.
> 
> I consulted the thesaurus and didn't really like the other options
> (discrete, disjoint, separate, etc.). I think 'independent' is more
> accurate for this and better than 'concurrent' and 'parallel'.
> 


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-26  3:50         ` Damien Le Moal
@ 2021-08-26  6:28           ` Hannes Reinecke
  2021-08-27  3:03           ` Martin K. Petersen
  1 sibling, 0 replies; 25+ messages in thread
From: Hannes Reinecke @ 2021-08-26  6:28 UTC (permalink / raw)
  To: Damien Le Moal, Martin K. Petersen; +Cc: Jens Axboe, linux-block, linux-scsi

On 8/26/21 5:50 AM, Damien Le Moal wrote:
> On 2021/08/26 12:43, Martin K. Petersen wrote:
>>
>> Damien,
>>
>>> I am not super happy with the name either. I used this one as the
>>> least worst of possibilities I thought of.  seek_range/srange ? ->
>>> that is very HDD centric and as we can reuse this for things like
>>> dm-linear on top of SSDs, that does not really work.  I would prefer
>>> something that convey the idea of "parallel command execution", since
>>> this is the main point of the interface. prange ? cdm_range ?
>>> req_range ?
>>
>> How about independent_access_range? That doesn't imply head positioning
>> and can also be used to describe a fault domain. And it is less
>> disk-centric than concurrent_positioning_range.
> 
> I like it, but a bit long-ish. Do you think shortening to access_range would be
> acceptable ?
> 
> we would have:
> 
> /sys/block/XYZ/queue/access_ranges/...
> 
> and
> 
> struct blk_access_range {
> 	...
> 	sector_t sector;
> 	sector_t nr_sectors;
> }
> 
> struct blk_access_range *arange;
> 
> Adding independent does make everything even more obvious, but names become
> rather long. Not an issue for the sysfs directory I think, but
> 
> struct blk_independent_access_range {
> 	...
> 	sector_t sector;
> 	sector_t nr_sectors;
> }
> 
> is rather a long struct name. Shortening independent to "ind" could very easily
> be confused with "indirection", so that is not an option. And "iaccess" is
> obscure...
> 
I'd vote for access_range.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-26  3:50         ` Damien Le Moal
  2021-08-26  6:28           ` Hannes Reinecke
@ 2021-08-27  3:03           ` Martin K. Petersen
  2021-08-27  3:11             ` Damien Le Moal
  1 sibling, 1 reply; 25+ messages in thread
From: Martin K. Petersen @ 2021-08-27  3:03 UTC (permalink / raw)
  To: Damien Le Moal
  Cc: Martin K. Petersen, Jens Axboe, linux-block, linux-scsi, Hannes Reinecke


Damien,

> I like it, but a bit long-ish. Do you think shortening to access_range
> would be acceptable ?

But doesn't 'access_range' imply that there are ranges that you can't
access? I think 'independent' is more important and 'access' is just a
clarification.

> Adding independent does make everything even more obvious, but names become
> rather long. Not an issue for the sysfs directory I think, but

I do think it's important that the sysfs directory in particular is the
full thing. It's a user-visible interface.

If the internal interfaces have a shorthand I guess that's OK.

> struct blk_independent_access_range {
> 	...
> 	sector_t sector;
> 	sector_t nr_sectors;
> }
>
> is rather a long struct name.

True, but presumably you'd do:

	struct blk_independent_access_range *iar;

in a variable declaration and be done with it. So I don't think the type
is a big deal. Where it becomes unwieldy is:

	blk_rq_independent_access_range_frobnicate();

Anyway. Running out of ideas. autonomous_range? sequestered_range? 

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v5 0/5] Initial support for multi-actuator HDDs
  2021-08-27  3:03           ` Martin K. Petersen
@ 2021-08-27  3:11             ` Damien Le Moal
  0 siblings, 0 replies; 25+ messages in thread
From: Damien Le Moal @ 2021-08-27  3:11 UTC (permalink / raw)
  To: Martin K. Petersen; +Cc: Jens Axboe, linux-block, linux-scsi, Hannes Reinecke

On 2021/08/27 12:03, Martin K. Petersen wrote:
> 
> Damien,
> 
>> I like it, but a bit long-ish. Do you think shortening to access_range
>> would be acceptable ?
> 
> But doesn't 'access_range' imply that there are ranges that you can't
> access? I think 'independent' is more important and 'access' is just a
> clarification.
> 
>> Adding independent does make everything even more obvious, but names become
>> rather long. Not an issue for the sysfs directory I think, but
> 
> I do think it's important that the sysfs directory in particular is the
> full thing. It's a user-visible interface.

Totally agree on that.

> If the internal interfaces have a shorthand I guess that's OK.
> 
>> struct blk_independent_access_range {
>> 	...
>> 	sector_t sector;
>> 	sector_t nr_sectors;
>> }
>>
>> is rather a long struct name.
> 
> True, but presumably you'd do:
> 
> 	struct blk_independent_access_range *iar;
> 
> in a variable declaration and be done with it. So I don't think the type
> is a big deal. Where it becomes unwieldy is:
> 
> 	blk_rq_independent_access_range_frobnicate();

OK. Let me rework the patches with the full blk_independent_access_range type
name to see what everything becomes.

> 
> Anyway. Running out of ideas. autonomous_range? sequestered_range? 

Arg. I prefer independent over autonomous. And "sequestered" is used in the
context of SCSI/ATA command duration limits so I would rather not use that.
I am out of ideas too. Let me try to see with "independent".


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2021-08-27  3:11 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-12  7:50 [PATCH v5 0/5] Initial support for multi-actuator HDDs Damien Le Moal
2021-08-12  7:50 ` [PATCH v5 1/5] block: Add concurrent positioning ranges support Damien Le Moal
2021-08-12  7:50 ` [PATCH v5 2/5] scsi: sd: add " Damien Le Moal
2021-08-12  7:50 ` [PATCH v5 3/5] libata: support concurrent positioning ranges log Damien Le Moal
2021-08-12  7:50 ` [PATCH v5 4/5] doc: document sysfs queue/cranges attributes Damien Le Moal
2021-08-12  7:50 ` [PATCH v5 5/5] doc: Fix typo in request queue sysfs documentation Damien Le Moal
2021-08-12 17:10 ` [PATCH v5 0/5] Initial support for multi-actuator HDDs Jens Axboe
2021-08-12 17:21   ` Martin K. Petersen
2021-08-12 17:25     ` Jens Axboe
2021-08-13  0:30       ` Damien Le Moal
2021-08-16 17:12         ` Martin K. Petersen
2021-08-17  4:06           ` Damien Le Moal
2021-08-16 17:10 ` Martin K. Petersen
2021-08-17  4:06   ` Damien Le Moal
2021-08-18  2:24     ` Martin K. Petersen
2021-08-18  2:45       ` Damien Le Moal
2021-08-23  1:27 ` Damien Le Moal
2021-08-26  2:07 ` Damien Le Moal
2021-08-26  2:41   ` Martin K. Petersen
2021-08-26  3:09     ` Damien Le Moal
2021-08-26  3:43       ` Martin K. Petersen
2021-08-26  3:50         ` Damien Le Moal
2021-08-26  6:28           ` Hannes Reinecke
2021-08-27  3:03           ` Martin K. Petersen
2021-08-27  3:11             ` Damien Le Moal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).