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

[Theodore Ts'o]

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