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

From: Nitesh Shetty <nj.shetty@samsung.com>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Mike Snitzer <snitzer@redhat.com>,
	djwong@kernel.org, linux-nvme@lists.infradead.org, clm@fb.com,
	dm-devel@redhat.com, Chaitanya Kulkarni <kch@nvidia.com>,
	osandov@fb.com, Alasdair Kergon <agk@redhat.com>,
	javier@javigon.com, bvanassche@acm.org,
	linux-scsi@vger.kernel.org, nitheshshetty@gmail.com,
	Smart <james.smart@broadcom.com>,
	hch@lst.de, chaitanyak@nvidia.com,
	SelvaKumar S <selvakuma.s1@samsung.com>,
	msnitzer@redhat.com, josef@toxicpanda.com,
	linux-block@vger.kernel.org, dsterba@suse.com, kbusch@kernel.org,
	Frederick.Knight@netapp.com, Sagi Grimberg <sagi@grimberg.me>,
	axboe@kernel.dk, tytso@mit.edu, joshi.k@samsung.com,
	martin.petersen@oracle.com, James, linux-kernel@vger.kernel.org,
	arnav.dawn@samsung.com, jack@suse.com,
	linux-fsdevel@vger.kernel.org, lsf-pc@lists.linux-foundation.org,
	Alexander Viro <viro@zeniv.linux.org.uk>
Subject: Re: [dm-devel] [PATCH v3 02/10] block: Introduce queue limits for copy-offload support
Date: Thu, 24 Feb 2022 17:32:18 +0530	[thread overview]
Message-ID: <20220224120218.GA9117@test-zns> (raw)
In-Reply-To: <YhWGDUyQkUcE6itt@bombadil.infradead.org>

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

On Tue, Feb 22, 2022 at 04:55:41PM -0800, 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.
>
Yeah, queue limits indicates copy offload. I think will make more
readable by adding copy_offload_* prefix.

> > > > +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 is read only for user. And inside kernel, this is set
only by driver at initialization.

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

[-- Attachment #3: Type: text/plain, Size: 97 bytes --]

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel