Re: [PATCH v20 01/12] block: Introduce queue limits and sysfs for copy-offload support

[Nitesh Shetty]

On 01/06/24 07:53AM, Christoph Hellwig wrote:
> On Mon, May 20, 2024 at 03:50:14PM +0530, Nitesh Shetty wrote:
> > Add device limits as sysfs entries,
> > 	- copy_max_bytes (RW)
> > 	- copy_max_hw_bytes (RO)
> >
> > Above limits help to split the copy payload in block layer.
> > copy_max_bytes: maximum total length of copy in single payload.
> > copy_max_hw_bytes: Reflects the device supported maximum limit.
>
> That's a bit of a weird way to phrase the commit log as the queue_limits
> are the main thing (and there are three of them as required for the
> scheme to work).  The sysfs attributes really are just an artifact.
Acked, we will update commit log.

> > @@ -231,10 +237,11 @@ int blk_set_default_limits(struct queue_limits *lim)
> >  {
> >  	/*
> >  	 * Most defaults are set by capping the bounds in blk_validate_limits,
> > -	 * but max_user_discard_sectors is special and needs an explicit
> > -	 * initialization to the max value here.
> > +	 * but max_user_discard_sectors and max_user_copy_sectors are special
> > +	 * and needs an explicit initialization to the max value here.
>
> s/needs/need/
Acked.

> > +/*
> > + * blk_queue_max_copy_hw_sectors - 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_hw_sectors(struct request_queue *q,
> > +				   unsigned int max_copy_sectors)
> > +{
> > +	struct queue_limits *lim = &q->limits;
> > +
> > +	if (max_copy_sectors > (BLK_COPY_MAX_BYTES >> SECTOR_SHIFT))
> > +		max_copy_sectors = BLK_COPY_MAX_BYTES >> SECTOR_SHIFT;
> > +
> > +	lim->max_copy_hw_sectors = max_copy_sectors;
> > +	lim->max_copy_sectors =
> > +		min(max_copy_sectors, lim->max_user_copy_sectors);
> > +}
> > +EXPORT_SYMBOL_GPL(blk_queue_max_copy_hw_sectors);
>
> Please don't add new blk_queue_* helpers, everything should go through
> the atomic queue limits API now.  Also capping the hardware limit
> here looks odd.
This was a mistake, we are not using this function. We will remove this
function in next version.

> > +	if (max_copy_bytes & (queue_logical_block_size(q) - 1))
> > +		return -EINVAL;
>
> This should probably go into blk_validate_limits and just round down.
Bart also pointed out similar thing. We do same check before issuing
copy offload, so we will remove this check.

> Also most block limits are in kb.  Not that I really know why we are
> doing that, but is there a good reason to deviate from that scheme?
We followed discard as a reference, but we can move to kb, if that helps
with overall readability.

Thank you,
Nitesh Shetty