Re: [PATCH 11/11] xfs: add media verification ioctl

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jan 14, 2026 at 07:02:14AM +0100, Christoph Hellwig wrote:
> On Tue, Jan 13, 2026 at 03:21:13PM -0800, Darrick J. Wong wrote:
> > > > +#define XFS_VERIFY_TO_EOD	(~0ULL)	/* end of disk */
> > > 
> > > Is there much of a point in this flag?  scrub/healer really should
> > > know the device size, shouldn't they?
> > 
> > Yes, scrub and healer both know the size they want to verify.  I put
> > that in for the sake of xfs_io so that it wouldn't have to figure out
> > the device size, but as the ioctl always decreases @end_daddr to the
> > actual EOD, I think it'd be ok if xfs_io blindly wrote in ~0ULL.
> 
> That's the best of both worlds.
> 
> > > > +	const unsigned int	iosize = BIO_MAX_VECS << PAGE_SHIFT;
> > > > +	unsigned int		bufsize = iosize;
> > > 
> > > That's a pretty gigantic buffer size.  In general a low number of
> > > MB should max out most current devices, and for a background scrub
> > > you generally do not want to actually max out the device..
> > 
> > 256 * 4k (= 1MB) is too large a buffer?
> 
> No, my reading comprehension just sucks :)  And of course the way
> it's written isn't very helpful either.
> 
> > I guess that /is/ 16M on a 64k-page system.
> 
> Yeah, just stick to SZ_1M.

Ok, will do.

> > > > +				min(nr_sects, bufsize >> SECTOR_SHIFT);
> > > > +
> > > > +			bio_add_folio_nofail(bio, folio,
> > > > +					vec_sects << SECTOR_SHIFT, 0);
> > > > +
> > > > +			bio_daddr += vec_sects;
> > > > +			bio_bbcount -= vec_sects;
> > > > +			bio_submitted += vec_sects;
> > > > +		}
> > > 
> > > A single folio is always just a single vetor in the bio.  No need
> > > for any of the looping here.
> > 
> > If we have to fall back to a single base page, shouldn't we still try to
> > create a larger bio?
> 
> How do you create a larger bio if you only have a single bio available?
> 
> > A subtle assumption here is that it's ok to have
> > all the bvecs pointing to the same memory, and that the device won't
> > screw up if someone asks it to DMA to the same page simultaneously.
> 
> Ooooh.  Yes, that will screw up badly when using PI.

DOH!  I forgot about that.

Yep, that's correct.  I'll change the code to one folio, one bio. 

> > > > +		/* Don't let too many IOs accumulate */
> > > > +		if (bio_submitted > SZ_256M >> SECTOR_SHIFT) {
> > > > +			blk_finish_plug(&plug);
> > > > +			error = submit_bio_wait(bio);
> > > 
> > > Also the building up and chaining here seems harmful.  If you're
> > > on SSDs you want to fire things off ASAP if you have large I/O.
> > > On a HDD we'll take care of it below, but the bios will usually
> > > actually be split, not merged anyway as they are beyond the
> > > supported I/O size of the HBAs.
> > 
> > Hrm, maybe I should query the block device for max_sectors_kb then?
> 
> No.  max_sectors_kb is kida stupid.  I think a sensible default and
> a tunable is a better choice here at least for now.

<nod> I'll set iosize to 1MB by default and userspace can decrease it if
it so desires.

Also it occurs to me that max_hw_sectors_kb seems to be 128K for all of
my consumer nvme devices, and the fancy Intel ones too.  Funny that the
sata ssds set it to 4MB, but I guess capping at 128k is one way to
reduce the command latency...?  (Or the kernel's broken?  I can't figure
out how to get mpsmin with nvme-cli...)

> > However, in the case where memory is fragmented and we can only get
> > (say) a single base page, it'll still try to load up the bio with as
> > many vecs as it can to try to keep the io size large, because issuing
> > 256x 4k IOs is a lot slower than issuing 1x 1M IO with the same page
> > added 256 times.
> 
> Yeah.  But seriously, if the MM is pretty good and is getting better
> at finding large allocations.  We need to start relying on that.
> 
> > I wonder if nr_vecs ought to be capped by queue_max_segments?
> 
> No, leave all that splitting to the block layer.  max_segments is
> an implementation detail.

Will do.  Only using the folio once per bio will make the control loop
easier to understand too.

--D