On Wed, Mar 08, 2023 at 09:18:07AM +0800, Chao Yu wrote: > Quoted from manual of fstrim: > > "-m, --minimum minimum-size > ..., if it's smaller than the device's minimum, and report that > (fstrim_range.minlen) back to userspace." First of all, I'll note that the fstrim(8) man page, which is describing how the userspace fstrim application work, is a really weird place to put information about how the fstrim _ioctl_ works. I've added Lukas, who is listed as one of the authors of fstrim, and who probably had a hand in writing the man page. The text in that paragraph describing -m is extremely confusing, and probably needs to be rewritten, and factored out into a fstrim(8) for system adminsitrators, and ioctl_fitrim(2) which documents the the ioctl. > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index 5b2ae37a8b80..bd3ef29cf8a6 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -6491,6 +6491,9 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range) > discard_granularity >> sb->s_blocksize_bits); > if (minlen > EXT4_CLUSTERS_PER_GROUP(sb)) > goto out; > + > + /* Report adjusted minlen back to userspace */ > + range->minlen = minlen; > } Unfortunately, this patch is not correct. The units of struct fstrim_range's minlen (here, range->minlen) is bytes. 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); 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); Oh, and by the way, that first conversion is not correct as currently written in ext4_fs_trim(). It should be minlen = (range->minlen + EXT4_CLUSTER_SIZE(sb) - 1) >> (sb->s_blocksize_bits + EXT4_CLUSTER_BITS(sb)); The explanation of why minlen = EXT4_NUM_B2C(EXT4_SB(sb), range->minlen >> sb->s_blocksize_bits); is wrong is left as an exercise to the reader. (Hint: what is supposed to happen if a value of 1 byte is passed in fstrim_range.minlen?) Cheers, - Ted
