ext4: fix to report fstrim.minlen back to userspace

PATCH Superseded Chao Yu 2 participants 3 messages Mar 8, 2023 – Mar 12, 2023 ext4 file system

lore

Share & tools

lore

Chao Yu

author

2023-03-08 01:18 UTC #0 lore raw

PATCH 1/1 ext4: fix to report fstrim.minlen back to userspace

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 <[email protected]>

---
 fs/ext4/mballoc.c | 3 +++
 1 file changed, 3 insertions(+)

fs/ext4/mballoc.c +3 -0

index 5b2ae37a8b80..bd3ef29cf8a6 100644
@@ -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;

Signature · 1 line

2.25.1

Theodore Ts'o

maintainer

2023-03-11 03:19 UTC #1 lore raw

Quoted 5 lines · 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."

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.

fs/ext4/mballoc.c int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range) · line 6497

Quoted 12 lines

fs/ext4/mballoc.c +3 -0

index 5b2ae37a8b80..bd3ef29cf8a6 100644
@@ -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

Chao Yu

author

2023-03-12 10:16 UTC #2 lore raw

Quoted 2 lines · Theodore Ts'o

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.

Quoted 7 lines

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));

Quoted 8 lines

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,