On 20/05/24 03:42PM, Bart Van Assche wrote:
On 5/20/24 03:20, Nitesh Shetty wrote:
+static ssize_t queue_copy_max_show(struct request_queue *q, char *page)
+{
+ return sprintf(page, "%llu\n", (unsigned long long)
+ q->limits.max_copy_sectors << SECTOR_SHIFT);
+}
+
+static ssize_t queue_copy_max_store(struct request_queue *q, const char *page,
+ size_t count)
+{
+ unsigned long max_copy_bytes;
+ struct queue_limits lim;
+ ssize_t ret;
+ int err;
+
+ ret = queue_var_store(&max_copy_bytes, page, count);
+ if (ret < 0)
+ return ret;
+
+ if (max_copy_bytes & (queue_logical_block_size(q) - 1))
+ return -EINVAL;
Wouldn't it be more user-friendly if this check would be left out? Does any code
depend on max_copy_bytes being a multiple of the logical block size?
In block layer, we use max_copy_bytes to split larger copy into
device supported copy size.
Simple copy spec requires length to be logical block size aligned.
Hence this check.
+ blk_mq_freeze_queue(q);
+ lim = queue_limits_start_update(q);
+ lim.max_user_copy_sectors = max_copy_bytes >> SECTOR_SHIFT;
+ err = queue_limits_commit_update(q, &lim);
+ blk_mq_unfreeze_queue(q);
+
+ if (err)
+ return err;
+ return count;
+}
queue_copy_max_show() shows max_copy_sectors while queue_copy_max_store()
modifies max_user_copy_sectors. Is that perhaps a bug?
This follows discard implementaion[1].
max_copy_sectors gets updated in queue_limits_commits_update.
[1] https://lore.kernel.org/linux-block/20240213073425.1621680-7-hch@xxxxxx/
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index aefdda9f4ec7..109d9f905c3c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -309,6 +309,10 @@ struct queue_limits {
unsigned int discard_alignment;
unsigned int zone_write_granularity;
+ unsigned int max_copy_hw_sectors;
+ unsigned int max_copy_sectors;
+ unsigned int max_user_copy_sectors;
Two new limits are documented in Documentation/ABI/stable/sysfs-block while three
new parameters are added in struct queue_limits. Why three new limits instead of
two? Please add a comment above the new parameters that explains the role of the
new parameters.
Similar to discard, only 2 limits are exposed to user.
+/* maximum copy offload length, this is set to 128MB based on current testing */
+#define BLK_COPY_MAX_BYTES (1 << 27)
"current testing" sounds vague. Why is this limit required? Why to cap what the
driver reports instead of using the value reported by the driver without modifying it?
Here we are expecting BLK_COPY_MAX_BYTES >= driver supported limit.
We do support copy length larger than device supported limit.
In block later(blkdev_copy_offload), we split larger copy into device
supported limit and send down.
We added this check to make sure userspace doesn't consume all the
kernel resources[2].
We can remove/expand this arbitary limit moving forward.
[2] https://lore.kernel.org/linux-block/YRu1WFImFulfpk7s@xxxxxxxxx/
Additionally, since this constant is only used in source code that occurs in the
block/ directory, please move the definition of this constant into a source or header
file in the block/ directory.
We are using this in null block driver as well, so we need to keep it
here.
Thank You,
Nitesh Shetty
