Re: [PATCH v20 01/12] block: Introduce queue limits and sysfs for copy-offload support

[Nitesh Shetty]

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