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

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

[Chao Yu]

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."

So this patch tries to report adjusted fstrim_range.minlen back to
userspace via FITRIM interface, if the value is smaller than device's
minimum discard granularity.

Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/ext4/mballoc.c | 3 +++
 1 file changed, 3 insertions(+)

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;
 	}
 	if (end >= max_blks - 1) {
 		end = max_blks - 1;
-- 
2.25.1

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

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,