Re: [PATCH 2/2] xfs: don't extend the FITRIM range if the rt device does not support discard

[Christoph Hellwig <hch@lst.de>]

On Tue, Aug 13, 2024 at 10:41:18PM -0700, Darrick J. Wong wrote:
> Does this still return EOPNOTSUPP if there's no rt device and the data
> device doesn't support discard?

Yes, I'll need to fix that.

> > +	if (rt_bdev) {
> > +		max_blocks += mp->m_sb.sb_rblocks;
> 
> I think this will break xfs_scrub, which (unlike fstrim) breaks up its
> FITRIM requests into smaller pieces.

No breakage noticed during my testing, but I'm not sure how that
would have materialized anyway..

> The (afwul) FITRIM interface says
> that [0, dblocks) trims the data device, and [dblocks, dblocks +
> rblocks) trims the realtime device.
> 
> If the data device doesn't support discard, max_blocks will be rblocks,
> and that's what we use to validate the @start parameter.  For example,
> if the data device is 10T spinning rust and the rt device is a 10G SSD,
> max_blocks will be 10G.  A FITRIM request for just the rt device will be
> [10T, 10G), which now fails with EINVAL.
> 
> I don't have a fix to suggest for this yet, but let me play around with
> this tomorrow and see if I can come up with something better, or figure
> out how I'm being thick. ;)
> 
> My guess is that what we really want is if either device supports
> discard we allow the full range, but if a specific device doesn't
> support discard then we skip it and don't add anything to the outgoing
> range.len.  But that's what I thought the current code does. <shrug>

The problem is that if we allow the full range, but return a smaller
around generic/260 fails.  That is with your patches to not fail if
FITRIM for the RT device is supported, without them it always fails
as soon as FITRIM is supported on the RT device.