Re: [PATCH v13 1/9] block: Introduce queue limits for copy-offload support

[Nitesh Shetty <nj.shetty@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 6418 bytes --]

On 23/06/28 03:40PM, Damien Le Moal wrote:
>On 6/28/23 03:36, Nitesh Shetty wrote:
>> Add device limits as sysfs entries,
>>         - copy_offload (RW)
>>         - copy_max_bytes (RW)
>>         - copy_max_bytes_hw (RO)
>>
>> Above limits help to split the copy payload in block layer.
>> copy_offload: used for setting copy offload(1) or emulation(0).
>> copy_max_bytes: maximum total length of copy in single payload.
>> copy_max_bytes_hw: Reflects the device supported maximum limit.
>>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>> Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
>> ---
>>  Documentation/ABI/stable/sysfs-block | 33 +++++++++++++++
>>  block/blk-settings.c                 | 24 +++++++++++
>>  block/blk-sysfs.c                    | 63 ++++++++++++++++++++++++++++
>>  include/linux/blkdev.h               | 12 ++++++
>>  include/uapi/linux/fs.h              |  3 ++
>>  5 files changed, 135 insertions(+)
>>
>> diff --git a/Documentation/ABI/stable/sysfs-block b/Documentation/ABI/stable/sysfs-block
>> index c57e5b7cb532..3c97303f658b 100644
>> --- a/Documentation/ABI/stable/sysfs-block
>> +++ b/Documentation/ABI/stable/sysfs-block
>> @@ -155,6 +155,39 @@ Description:
>>  		last zone of the device which may be smaller.
>>
>>
>> +What:		/sys/block/<disk>/queue/copy_offload
>> +Date:		June 2023
>> +Contact:	linux-block@vger.kernel.org
>> +Description:
>> +		[RW] When read, this file shows whether offloading copy to a
>> +		device is enabled (1) or disabled (0). Writing '0' to this
>> +		file will disable offloading copies for this device.
>> +		Writing any '1' value will enable this feature. If the device
>> +		does not support offloading, then writing 1, will result in an
>> +		error.
>
>I am still not convinced that this one is really necessary. copy_max_bytes_hw !=
>0 indicates that the devices supports copy offload. And setting copy_max_bytes
>to 0 can be used to disable copy offload (which probably should be the default
>for now).
>

Agreed, we will do this in next iteration.

>> +
>> +
>> +What:		/sys/block/<disk>/queue/copy_max_bytes
>> +Date:		June 2023
>> +Contact:	linux-block@vger.kernel.org
>> +Description:
>> +		[RW] This is the maximum number of bytes that the block layer
>> +		will allow for a copy request. This will is always smaller or
>
>will is -> is
>

acked

>> +		equal to the maximum size allowed by the hardware, indicated by
>> +		'copy_max_bytes_hw'. An attempt to set a value higher than
>> +		'copy_max_bytes_hw' will truncate this to 'copy_max_bytes_hw'.
>> +
>> +
>> +What:		/sys/block/<disk>/queue/copy_max_bytes_hw
>
>Nit: In keeping with the spirit of attributes like
>max_hw_sectors_kb/max_sectors_kb, I would call this one copy_max_hw_bytes.
>

acked, will update in next iteration.

>> +Date:		June 2023
>> +Contact:	linux-block@vger.kernel.org
>> +Description:
>> +		[RO] This is the maximum number of bytes that the hardware
>> +		will allow for single data copy request.
>> +		A value of 0 means that the device does not support
>> +		copy offload.
>> +
>> +
>>  What:		/sys/block/<disk>/queue/crypto/
>>  Date:		February 2022
>>  Contact:	linux-block@vger.kernel.org
>> diff --git a/block/blk-settings.c b/block/blk-settings.c
>> index 4dd59059b788..738cd3f21259 100644
>> --- a/block/blk-settings.c
>> +++ b/block/blk-settings.c
>> @@ -59,6 +59,8 @@ void blk_set_default_limits(struct queue_limits *lim)
>>  	lim->zoned = BLK_ZONED_NONE;
>>  	lim->zone_write_granularity = 0;
>>  	lim->dma_alignment = 511;
>> +	lim->max_copy_sectors_hw = 0;
>> +	lim->max_copy_sectors = 0;
>>  }
>>
>>  /**
>> @@ -82,6 +84,8 @@ void blk_set_stacking_limits(struct queue_limits *lim)
>>  	lim->max_dev_sectors = UINT_MAX;
>>  	lim->max_write_zeroes_sectors = UINT_MAX;
>>  	lim->max_zone_append_sectors = UINT_MAX;
>> +	lim->max_copy_sectors_hw = UINT_MAX;
>> +	lim->max_copy_sectors = UINT_MAX;
>>  }
>>  EXPORT_SYMBOL(blk_set_stacking_limits);
>>
>> @@ -183,6 +187,22 @@ void blk_queue_max_discard_sectors(struct request_queue *q,
>>  }
>>  EXPORT_SYMBOL(blk_queue_max_discard_sectors);
>>
>> +/**
>> + * blk_queue_max_copy_sectors_hw - set max sectors for a single copy payload
>> + * @q:  the request queue for the device
>> + * @max_copy_sectors: maximum number of sectors to copy
>> + **/
>> +void blk_queue_max_copy_sectors_hw(struct request_queue *q,
>> +		unsigned int max_copy_sectors)
>> +{
>> +	if (max_copy_sectors > (COPY_MAX_BYTES >> SECTOR_SHIFT))
>> +		max_copy_sectors = COPY_MAX_BYTES >> SECTOR_SHIFT;
>> +
>> +	q->limits.max_copy_sectors_hw = max_copy_sectors;
>> +	q->limits.max_copy_sectors = max_copy_sectors;
>> +}
>> +EXPORT_SYMBOL_GPL(blk_queue_max_copy_sectors_hw);
>> +
>>  /**
>>   * blk_queue_max_secure_erase_sectors - set max sectors for a secure erase
>>   * @q:  the request queue for the device
>> @@ -578,6 +598,10 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
>>  	t->max_segment_size = min_not_zero(t->max_segment_size,
>>  					   b->max_segment_size);
>>
>> +	t->max_copy_sectors = min(t->max_copy_sectors, b->max_copy_sectors);
>> +	t->max_copy_sectors_hw = min(t->max_copy_sectors_hw,
>> +						b->max_copy_sectors_hw);
>> +
>>  	t->misaligned |= b->misaligned;
>>
>>  	alignment = queue_limit_alignment_offset(b, start);
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index afc797fb0dfc..43551778d035 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -199,6 +199,62 @@ static ssize_t queue_discard_zeroes_data_show(struct request_queue *q, char *pag
>>  	return queue_var_show(0, page);
>>  }
>>
>> +static ssize_t queue_copy_offload_show(struct request_queue *q, char *page)
>> +{
>> +	return queue_var_show(blk_queue_copy(q), page);
>> +}
>> +
>> +static ssize_t queue_copy_offload_store(struct request_queue *q,
>> +				       const char *page, size_t count)
>> +{
>> +	unsigned long copy_offload;
>> +	ssize_t ret = queue_var_store(&copy_offload, page, count);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (copy_offload && !q->limits.max_copy_sectors_hw)
>> +		return -EINVAL;
>> +
>> +	if (copy_offload)
>> +		blk_queue_flag_set(QUEUE_FLAG_COPY, q);
>> +	else
>> +		blk_queue_flag_clear(QUEUE_FLAG_COPY, q);
>> +
>> +	return count;
>> +}
>
>See above. I think we can drop this attribute.
>
acked

Thank you, 
Nitesh Shetty

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]