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>
Cc: Pankaj Raghav <p.raghav@samsung.com>,
	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: Mon, 6 Feb 2023 16:19:34 -0800	[thread overview]
Message-ID: <Y+GZFoHiUOQeq25d@bombadil.infradead.org> (raw)
In-Reply-To: <24b34999-8f7c-7821-0b15-fdfc3f508b13@acm.org>

On Mon, Feb 06, 2023 at 04:02:46PM -0800, Bart Van Assche wrote:
> On 2/1/23 15:50, Luis Chamberlain wrote:
> > On Mon, Jan 30, 2023 at 01:26:51PM -0800, Bart Van Assche wrote:
> > > @@ -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.
> 
> Will do.
> 
> > > +
> > > +	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.
> 
> If a block driver passes an argument to blk_queue_max_hw_sectors() or
> blk_queue_max_segment_size() that is smaller than what is supported by the
> block layer, data corruption will be triggered if the block driver or the
> hardware supported by the block driver does not support the larger values
> chosen by the block layer. Changing limits that will trigger data corruption
> into limits that may work seems like an improvement to me?

Wow, clearly! Without a doubt!

But I'm trying to do a careful review here.

The commit log did not describe what *does* happen in these situations today,
and you seem to now be suggesting in the worst case corruption can happen.
That changes the patch context quite a bit!

My question above still stands though, how many block drivers have a max
hw sector smaller than the equivalent PAGE_SIZE. If you make your
change, even if it fixes some new use case where corruption is seen, can
it regress some old use cases for some old controllers?

  Luis