[PATCH] btrfs: fix qgroup reserve overflow break the qgroup limit

[PATCH] btrfs: fix qgroup reserve overflow break the qgroup limit

[ethanlien]

We use extent_changeset->bytes_changed in qgroup_reserve_data() to record
how many bytes we set for EXTENT_QGROUP_RESERVED state. Currently the
bytes_changed is set as "unsigned int", and it will overflow if we try to
fallocate a range larger than 4GiB. The result is we reserve less bytes
and eventually break the qgroup limit.

The following example test script reproduces the problem:

  $ cat qgroup-overflow.sh
  #!/bin/bash

  DEV=/dev/sdj
  MNT=/mnt/sdj

  mkfs.btrfs -f $DEV
  mount $DEV $MNT

  # Set qgroup limit to 2GiB.
  btrfs quota enable $MNT
  btrfs qgroup limit 2G $MNT

  # Try to fallocate a 3GiB file. This should fail.
  echo
  echo "Try to fallocate a 3GiB file..."
  fallocate -l 3G $MNT/3G.file

  # Try to fallocate a 5GiB file.
  echo
  echo "Try to fallocate a 5GiB file..."
  fallocate -l 5G $MNT/5G.file

  # See we break the qgroup limit.
  echo
  sync
  btrfs qgroup show -r $MNT

  umount $MNT

When running the test:

  $ ./qgroup-overflow.sh
  (...)

  Try to fallocate a 3GiB file...
  fallocate: fallocate failed: Disk quota exceeded

  Try to fallocate a 5GiB file...

  qgroupid         rfer         excl     max_rfer
  --------         ----         ----     --------
  0/5           5.00GiB      5.00GiB      2.00GiB

Since we have no control of how bytes_changed is used, it's better to
set it to u64.

Signed-off-by: ethanlien <ethanlien@synology.com>
---
 fs/btrfs/extent_io.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 0399cf8e3c32..151e9da5da2d 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -118,7 +118,7 @@ struct btrfs_bio_ctrl {
  */
 struct extent_changeset {
 	/* How many bytes are set/cleared in this operation */
-	unsigned int bytes_changed;
+	u64 bytes_changed;
 
 	/* Changed ranges */
 	struct ulist range_changed;
-- 
2.17.1

Re: [PATCH] btrfs: fix qgroup reserve overflow break the qgroup limit

[Qu Wenruo]

On 2022/3/7 18:00, ethanlien wrote:
> We use extent_changeset->bytes_changed in qgroup_reserve_data() to record
> how many bytes we set for EXTENT_QGROUP_RESERVED state. Currently the
> bytes_changed is set as "unsigned int", and it will overflow if we try to
> fallocate a range larger than 4GiB. The result is we reserve less bytes
> and eventually break the qgroup limit.
>
> The following example test script reproduces the problem:
>
>    $ cat qgroup-overflow.sh
>    #!/bin/bash
>
>    DEV=/dev/sdj
>    MNT=/mnt/sdj
>
>    mkfs.btrfs -f $DEV
>    mount $DEV $MNT
>
>    # Set qgroup limit to 2GiB.
>    btrfs quota enable $MNT
>    btrfs qgroup limit 2G $MNT
>
>    # Try to fallocate a 3GiB file. This should fail.
>    echo
>    echo "Try to fallocate a 3GiB file..."
>    fallocate -l 3G $MNT/3G.file
>
>    # Try to fallocate a 5GiB file.
>    echo
>    echo "Try to fallocate a 5GiB file..."
>    fallocate -l 5G $MNT/5G.file
>
>    # See we break the qgroup limit.
>    echo
>    sync
>    btrfs qgroup show -r $MNT
>
>    umount $MNT
>
> When running the test:
>
>    $ ./qgroup-overflow.sh
>    (...)
>
>    Try to fallocate a 3GiB file...
>    fallocate: fallocate failed: Disk quota exceeded
>
>    Try to fallocate a 5GiB file...
>
>    qgroupid         rfer         excl     max_rfer
>    --------         ----         ----     --------
>    0/5           5.00GiB      5.00GiB      2.00GiB
>
> Since we have no control of how bytes_changed is used, it's better to
> set it to u64.
>
> Signed-off-by: ethanlien <ethanlien@synology.com>
> ---
>   fs/btrfs/extent_io.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 0399cf8e3c32..151e9da5da2d 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -118,7 +118,7 @@ struct btrfs_bio_ctrl {
>    */
>   struct extent_changeset {
>   	/* How many bytes are set/cleared in this operation */
> -	unsigned int bytes_changed;
> +	u64 bytes_changed;

Oh, falloc...

Unlike regular buffered/direct write, which we use one changeset for
each ordered extent, which can never be larger than 256M.

For fallocate, we use one changeset for the whole range, thus it no
longer respects the 256M per extent limit, and caused the problem.

Nice fix and reproducer.

Feel free to submit the reproducer as a test case in xfstests.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

>
>   	/* Changed ranges */
>   	struct ulist range_changed;

Re: [PATCH] btrfs: fix qgroup reserve overflow break the qgroup limit

[David Sterba]

On Mon, Mar 07, 2022 at 06:00:04PM +0800, ethanlien wrote:
> We use extent_changeset->bytes_changed in qgroup_reserve_data() to record
> how many bytes we set for EXTENT_QGROUP_RESERVED state. Currently the
> bytes_changed is set as "unsigned int", and it will overflow if we try to
> fallocate a range larger than 4GiB. The result is we reserve less bytes
> and eventually break the qgroup limit.
> 
> The following example test script reproduces the problem:
> 
>   $ cat qgroup-overflow.sh
>   #!/bin/bash
> 
>   DEV=/dev/sdj
>   MNT=/mnt/sdj
> 
>   mkfs.btrfs -f $DEV
>   mount $DEV $MNT
> 
>   # Set qgroup limit to 2GiB.
>   btrfs quota enable $MNT
>   btrfs qgroup limit 2G $MNT
> 
>   # Try to fallocate a 3GiB file. This should fail.
>   echo
>   echo "Try to fallocate a 3GiB file..."
>   fallocate -l 3G $MNT/3G.file
> 
>   # Try to fallocate a 5GiB file.
>   echo
>   echo "Try to fallocate a 5GiB file..."
>   fallocate -l 5G $MNT/5G.file
> 
>   # See we break the qgroup limit.
>   echo
>   sync
>   btrfs qgroup show -r $MNT
> 
>   umount $MNT
> 
> When running the test:
> 
>   $ ./qgroup-overflow.sh
>   (...)
> 
>   Try to fallocate a 3GiB file...
>   fallocate: fallocate failed: Disk quota exceeded
> 
>   Try to fallocate a 5GiB file...
> 
>   qgroupid         rfer         excl     max_rfer
>   --------         ----         ----     --------
>   0/5           5.00GiB      5.00GiB      2.00GiB
> 
> Since we have no control of how bytes_changed is used, it's better to
> set it to u64.
> 
> Signed-off-by: ethanlien <ethanlien@synology.com>

Added to misc-next, thanks.