On 2/23/22 09:55, Luis Chamberlain wrote:
> On Thu, Feb 17, 2022 at 06:29:01PM +0530, Nitesh Shetty wrote:
>> Thu, Feb 17, 2022 at 01:07:00AM -0800, Luis Chamberlain wrote:
>>> The subject says limits for copy-offload...
>>>
>>> On Mon, Feb 14, 2022 at 01:29:52PM +0530, Nitesh Shetty wrote:
>>>> Add device limits as sysfs entries,
>>>> - copy_offload (RW)
>>>> - copy_max_bytes (RW)
>>>> - copy_max_hw_bytes (RO)
>>>> - copy_max_range_bytes (RW)
>>>> - copy_max_range_hw_bytes (RO)
>>>> - copy_max_nr_ranges (RW)
>>>> - copy_max_nr_ranges_hw (RO)
>>>
>>> Some of these seem like generic... and also I see a few more max_hw ones
>>> not listed above...
>>>
>> queue_limits and sysfs entries are differently named.
>> All sysfs entries start with copy_* prefix. Also it makes easy to lookup
>> all copy sysfs.
>> For queue limits naming, I tried to following existing queue limit
>> convention (like discard).
>
> My point was that your subject seems to indicate the changes are just
> for copy-offload, but you seem to be adding generic queue limits as
> well. Is that correct? If so then perhaps the subject should be changed
> or the patch split up.
>
>>>> +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(©_offload, page, count);
>>>> +
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + if (copy_offload && !q->limits.max_hw_copy_sectors)
>>>> + return -EINVAL;
>>>
>>>
>>> If the kernel schedules, copy_offload may still be true and
>>> max_hw_copy_sectors may be set to 0. Is that an issue?
>>>
>>
>> This check ensures that, we dont enable offload if device doesnt support
>> offload. I feel it shouldn't be an issue.
>
> My point was this:
>
> CPU1 CPU2
> Time
> 1) if (copy_offload
> 2) ---> preemption so it schedules
> 3) ---> some other high priority task Sets q->limits.max_hw_copy_sectors to 0
> 4) && !q->limits.max_hw_copy_sectors)
>
> Can something bad happen if we allow for this?
max_hw_copy_sectors describes the device capability to offload copy. So
this is read-only and "max_hw_copy_sectors != 0" means that the device
supports copy offload (this attribute should really be named
max_hw_copy_offload_sectors).
The actual loop to issue copy offload BIOs, however, must use the soft
version of the attribute: max_copy_sectors, which defaults to
max_hw_copy_sectors if copy offload is truned on and I guess to
max_sectors for the emulation case.
Now, with this in mind, I do not see how allowing max_copy_sectors to be
0 makes sense. I fail to see why that should be allowed since:
1) If copy_offload is true, we will rely on the device and chunk copy
offload BIOs up to max_copy_sectors
2) If copy_offload is false (or device does not support it), emulation
will be used by issuing read/write BIOs of up to max_copy_sectors.
Thus max_copy_sectors must always be at least equal to the device
minimum IO size, that is, the logical block size.
--
Damien Le Moal
Western Digital Research
