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(©_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
-- dm-devel mailing list dm-devel@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel