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,
