Re: [PATCH v4 2/7] block: Support configuring limits below the page size

From: Luis Chamberlain <mcgrof@kernel.org>
To: Bart Van Assche <bvanassche@acm.org>,
	Pankaj Raghav <p.raghav@samsung.com>
Cc: Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	Jaegeuk Kim <jaegeuk@kernel.org>,
	Avri Altman <avri.altman@wdc.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Christoph Hellwig <hch@lst.de>, Ming Lei <ming.lei@redhat.com>,
	Keith Busch <kbusch@kernel.org>
Subject: Re: [PATCH v4 2/7] block: Support configuring limits below the page size
Date: Wed, 1 Feb 2023 15:50:38 -0800	[thread overview]
Message-ID: <20230201235038.nnayavxpadq5yj34@garbanzo> (raw)
In-Reply-To: <20230130212656.876311-3-bvanassche@acm.org>

On Mon, Jan 30, 2023 at 01:26:51PM -0800, Bart Van Assche wrote:
> Allow block drivers to configure the following:
> * Maximum number of hardware sectors values smaller than
>   PAGE_SIZE >> SECTOR_SHIFT. With PAGE_SIZE = 4096 this means that values
>   below 8 are supported.
> * A maximum segment size below the page size. This is most useful
>   for page sizes above 4096 bytes.
> 
> The blk_sub_page_segments static branch will be used in later patches to
> prevent that performance of block drivers that support segments >=
> PAGE_SIZE and max_hw_sectors >= PAGE_SIZE >> SECTOR_SHIFT would be affected.

This commit log seems to not make it clear if there is a functional
change or not.

> @@ -100,6 +106,55 @@ void blk_queue_bounce_limit(struct request_queue *q, enum blk_bounce bounce)
>  }
>  EXPORT_SYMBOL(blk_queue_bounce_limit);
>  
> +/* For debugfs. */
> +int blk_sub_page_limit_queues_get(void *data, u64 *val)
> +{
> +	*val = READ_ONCE(blk_nr_sub_page_limit_queues);
> +
> +	return 0;
> +}
> +
> +/**
> + * blk_enable_sub_page_limits - enable support for max_segment_size values smaller than PAGE_SIZE and for max_hw_sectors values below PAGE_SIZE >> SECTOR_SHIFT

That's a really long line for kdoc, can't we just trim it?

And why not update the docs to reflect the change?

> @@ -122,12 +177,17 @@ EXPORT_SYMBOL(blk_queue_bounce_limit);
>  void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_sectors)
>  {
>  	struct queue_limits *limits = &q->limits;
> +	unsigned int min_max_hw_sectors = PAGE_SIZE >> SECTOR_SHIFT;
>  	unsigned int max_sectors;
>  
> -	if ((max_hw_sectors << 9) < PAGE_SIZE) {
> -		max_hw_sectors = 1 << (PAGE_SHIFT - 9);
> -		printk(KERN_INFO "%s: set to minimum %d\n",
> -		       __func__, max_hw_sectors);
> +	if (max_hw_sectors < min_max_hw_sectors) {
> +		blk_enable_sub_page_limits(limits);
> +		min_max_hw_sectors = 1;
> +	}

Up to this part this a non-functional update, and so why not a
separate patch to clarify that.

> +
> +	if (max_hw_sectors < min_max_hw_sectors) {
> +		max_hw_sectors = min_max_hw_sectors;
> +		pr_info("%s: set to minimum %u\n", __func__, max_hw_sectors);

But if I understand correctly here we're now changing
max_hw_sectors from 1 to whatever the driver set on
blk_queue_max_hw_sectors() if its smaller than PAGE_SIZE.

To determine if this is a functional change it begs the
question as to how many block drivers have a max hw sector
smaller than the equivalent PAGE_SIZE and wonder if that
could regress.

>  	}
>  
>  	max_hw_sectors = round_down(max_hw_sectors,
> @@ -282,10 +342,16 @@ EXPORT_SYMBOL_GPL(blk_queue_max_discard_segments);
>   **/
>  void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
>  {
> -	if (max_size < PAGE_SIZE) {
> -		max_size = PAGE_SIZE;
> -		printk(KERN_INFO "%s: set to minimum %d\n",
> -		       __func__, max_size);
> +	unsigned int min_max_segment_size = PAGE_SIZE;
> +
> +	if (max_size < min_max_segment_size) {
> +		blk_enable_sub_page_limits(&q->limits);
> +		min_max_segment_size = SECTOR_SIZE;
> +	}
> +
> +	if (max_size < min_max_segment_size) {
> +		max_size = min_max_segment_size;
> +		pr_info("%s: set to minimum %u\n", __func__, max_size);

And similar thing here.

  Luis