A user reported FITRIM ioctl failing for him on ext4 on some devices
without apparent reason. After some debugging we've found out that
these devices (being LVM volumes) report rather large discard
granularity of 42MB and the filesystem had 1k blocksize and thus group
size of 8MB. Because ext4 FITRIM implementation puts discard
granularity into minlen, ext4_trim_fs() declared the trim request as
invalid. However just silently doing nothing seems to be a more
appropriate reaction to such combination of parameters since user did
not specify anything wrong.
CC: Lukas Czerner <[email protected]>
Fixes: 5c2ed62fd447 ("ext4: Adjust minlen with discard_granularity in the FITRIM ioctl")
Signed-off-by: Jan Kara <[email protected]>
---
fs/ext4/ioctl.c | 2 --
fs/ext4/mballoc.c | 8 ++++++++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 606dee9e08a3..220a4c8178b5 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1117,8 +1117,6 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
sizeof(range)))
return -EFAULT;
- range.minlen = max((unsigned int)range.minlen,
- q->limits.discard_granularity);
ret = ext4_trim_fs(sb, &range);
if (ret < 0)
return ret;
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 72bfac2d6dce..7174add7b153 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6405,6 +6405,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
*/
int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
{
+ struct request_queue *q = bdev_get_queue(sb->s_bdev);
struct ext4_group_info *grp;
ext4_group_t group, first_group, last_group;
ext4_grpblk_t cnt = 0, first_cluster, last_cluster;
@@ -6423,6 +6424,13 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
start >= max_blks ||
range->len < sb->s_blocksize)
return -EINVAL;
+ /* No point to try to trim less than discard granularity */
+ if (range->minlen < q->limits.discard_granularity) {
+ minlen = EXT4_NUM_B2C(EXT4_SB(sb),
+ q->limits.discard_granularity >> sb->s_blocksize_bits);
+ if (minlen > EXT4_CLUSTERS_PER_GROUP(sb))
+ goto out;
+ }
if (end >= max_blks)
end = max_blks - 1;
if (end <= first_data_blk)
--
2.26.2
On Fri 12-11-21 16:22:02, Jan Kara wrote:
> A user reported FITRIM ioctl failing for him on ext4 on some devices
> without apparent reason. After some debugging we've found out that
> these devices (being LVM volumes) report rather large discard
> granularity of 42MB and the filesystem had 1k blocksize and thus group
> size of 8MB. Because ext4 FITRIM implementation puts discard
> granularity into minlen, ext4_trim_fs() declared the trim request as
> invalid. However just silently doing nothing seems to be a more
> appropriate reaction to such combination of parameters since user did
> not specify anything wrong.
>
> CC: Lukas Czerner <[email protected]>
> Fixes: 5c2ed62fd447 ("ext4: Adjust minlen with discard_granularity in the FITRIM ioctl")
> Signed-off-by: Jan Kara <[email protected]>
> ---
> fs/ext4/ioctl.c | 2 --
> fs/ext4/mballoc.c | 8 ++++++++
> 2 files changed, 8 insertions(+), 2 deletions(-)
I wanted to add one more comment: Alternatively we could return EOPNOTSUPP
in this case to indicate trim is never going to work on this fs. But just
doing nothing since we cannot submit useful discard request seems
appropriate to me.
Honza
>
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 606dee9e08a3..220a4c8178b5 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1117,8 +1117,6 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> sizeof(range)))
> return -EFAULT;
>
> - range.minlen = max((unsigned int)range.minlen,
> - q->limits.discard_granularity);
> ret = ext4_trim_fs(sb, &range);
> if (ret < 0)
> return ret;
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 72bfac2d6dce..7174add7b153 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6405,6 +6405,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
> */
> int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
> {
> + struct request_queue *q = bdev_get_queue(sb->s_bdev);
> struct ext4_group_info *grp;
> ext4_group_t group, first_group, last_group;
> ext4_grpblk_t cnt = 0, first_cluster, last_cluster;
> @@ -6423,6 +6424,13 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
> start >= max_blks ||
> range->len < sb->s_blocksize)
> return -EINVAL;
> + /* No point to try to trim less than discard granularity */
> + if (range->minlen < q->limits.discard_granularity) {
> + minlen = EXT4_NUM_B2C(EXT4_SB(sb),
> + q->limits.discard_granularity >> sb->s_blocksize_bits);
> + if (minlen > EXT4_CLUSTERS_PER_GROUP(sb))
> + goto out;
> + }
> if (end >= max_blks)
> end = max_blks - 1;
> if (end <= first_data_blk)
> --
> 2.26.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Fri, Nov 12, 2021 at 04:22:02PM +0100, Jan Kara wrote:
> A user reported FITRIM ioctl failing for him on ext4 on some devices
> without apparent reason. After some debugging we've found out that
> these devices (being LVM volumes) report rather large discard
> granularity of 42MB and the filesystem had 1k blocksize and thus group
> size of 8MB. Because ext4 FITRIM implementation puts discard
> granularity into minlen, ext4_trim_fs() declared the trim request as
> invalid. However just silently doing nothing seems to be a more
> appropriate reaction to such combination of parameters since user did
> not specify anything wrong.
Hi Jan,
I agree that it's better to silently do nothing rather than returning
-ENOTSUPP in this case and the patch looks mostly fine.
However currently we return the adjusted minlen back to the user and it
is also stated in the fstrim man page. I think it's worth keeping that
behavior.
When I think about it, it would probably be worth updating fstrim to
notify the user that the minlen changed, I can send a patch for that.
Thanks!
-Lukas
>
> CC: Lukas Czerner <[email protected]>
> Fixes: 5c2ed62fd447 ("ext4: Adjust minlen with discard_granularity in the FITRIM ioctl")
> Signed-off-by: Jan Kara <[email protected]>
> ---
> fs/ext4/ioctl.c | 2 --
> fs/ext4/mballoc.c | 8 ++++++++
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 606dee9e08a3..220a4c8178b5 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1117,8 +1117,6 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> sizeof(range)))
> return -EFAULT;
>
> - range.minlen = max((unsigned int)range.minlen,
> - q->limits.discard_granularity);
> ret = ext4_trim_fs(sb, &range);
> if (ret < 0)
> return ret;
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 72bfac2d6dce..7174add7b153 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -6405,6 +6405,7 @@ ext4_trim_all_free(struct super_block *sb, ext4_group_t group,
> */
> int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
> {
> + struct request_queue *q = bdev_get_queue(sb->s_bdev);
> struct ext4_group_info *grp;
> ext4_group_t group, first_group, last_group;
> ext4_grpblk_t cnt = 0, first_cluster, last_cluster;
> @@ -6423,6 +6424,13 @@ int ext4_trim_fs(struct super_block *sb, struct fstrim_range *range)
> start >= max_blks ||
> range->len < sb->s_blocksize)
> return -EINVAL;
> + /* No point to try to trim less than discard granularity */
> + if (range->minlen < q->limits.discard_granularity) {
> + minlen = EXT4_NUM_B2C(EXT4_SB(sb),
> + q->limits.discard_granularity >> sb->s_blocksize_bits);
> + if (minlen > EXT4_CLUSTERS_PER_GROUP(sb))
> + goto out;
> + }
> if (end >= max_blks)
> end = max_blks - 1;
> if (end <= first_data_blk)
> --
> 2.26.2
>
On Fri, 12 Nov 2021 16:22:02 +0100, Jan Kara wrote:
> A user reported FITRIM ioctl failing for him on ext4 on some devices
> without apparent reason. After some debugging we've found out that
> these devices (being LVM volumes) report rather large discard
> granularity of 42MB and the filesystem had 1k blocksize and thus group
> size of 8MB. Because ext4 FITRIM implementation puts discard
> granularity into minlen, ext4_trim_fs() declared the trim request as
> invalid. However just silently doing nothing seems to be a more
> appropriate reaction to such combination of parameters since user did
> not specify anything wrong.
>
> [...]
Applied, thanks!
[1/1] ext4: Avoid trim error on fs with small groups
commit: a4934e25c01ed056dc4af8bef086616e3b083a14
Best regards,
--
Theodore Ts'o <[email protected]>