Re: [PATCH] ext4: fix to report fstrim.minlen back to userspace

[Chao Yu]

On 2023/3/11 11:18, Theodore Ts'o wrote:
> Unfortunately, this patch is not correct.  The units of struct
> fstrim_range's minlen (here, range->minlen) is bytes.

Oh, that's right, sorry for the mistake.

> However the minlen variable in ext4_trim_fs is in units of *clusters*.
> And so it gets rounded up two places.  The first time is when it is
> converted into units of a cluster:
>
> 	minlen = EXT4_NUM_B2C(EXT4_SB(sb),
> 			      range->minlen >> sb->s_blocksize_bits);
IIUC, if range->minlen is smaller than block size of ext4, above calculation
may return a wrong value, due to it looks EXT4_NUM_B2C() expects a non-zero
in-parameter.

So it needs to round up minlen to block size first and then round up block
size to cluster size:

	minlen =  EXT4_NUM_B2C(EXT4_SB(sb),
		EXT4_BLOCK_ALIGN(range->minlen, sb->s_blocksize_bits));

Or do the conversion at a time as you reminded:

	minlen = (range->minlen + EXT4_CLUSTER_SIZE(sb) - 1) >>
		(sb->s_blocksize_bits + EXT4_CLUSTER_BITS(sb));

> And the second time is when it is rounded up to the block device's
> discard granularity.
>
> So after that if statement, we need to convert minlen from clusters to
> bytes, like so:
>
> 	range->minlen = EXT4_C2B(EXT4_SB(sb), minlen) << sb->s_blocksize_bits);

Thanks for the detailed explanation and reminder. :)

Thanks,