Re: [PATCH v3 02/10] block: Introduce queue limits for copy-offload support

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

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

On Wed, Feb 23, 2022 at 10:29:18AM +0900, Damien Le Moal wrote:
> 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(&copy_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).
>
Yes, it does make sense to change prefix to copy_offload_*, but downside
being sysfs attributes becomes too long.

> 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.

Agreed, if device doesn't suppport offload, soft limit should be based on
limits of READ/WRITE IOs.

--
Nitesh Shetty

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