[3/5] btrfs: use add_old_bytes when updating global reserve
diff mbox series

Message ID 20190816152019.1962-4-josef@toxicpanda.com
State New
Headers show
Series
  • Fix global reserve size and can overcommit
Related show

Commit Message

Josef Bacik Aug. 16, 2019, 3:20 p.m. UTC
We have some annoying xfstests tests that will create a very small fs,
fill it up, delete it, and repeat to make sure everything works right.
This trips btrfs up sometimes because we may commit a transaction to
free space, but most of the free metadata space was being reserved by
the global reserve.  So we commit and update the global reserve, but the
space is simply added to bytes_may_use directly, instead of trying to
add it to existing tickets.  This results in ENOSPC when we really did
have space.  Fix this by returning the space via
btrfs_space_info_add_old_bytes.  The global reserve _can_ be using
overcommitted space, but the add_old_bytes checks this and won't add the
reservation if we're still overcommitted, so we are safe in this regard.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/block-rsv.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Nikolay Borisov Aug. 20, 2019, 3:33 p.m. UTC | #1
On 16.08.19 г. 18:20 ч., Josef Bacik wrote:
> We have some annoying xfstests tests that will create a very small fs,
> fill it up, delete it, and repeat to make sure everything works right.
> This trips btrfs up sometimes because we may commit a transaction to
> free space, but most of the free metadata space was being reserved by
> the global reserve.  So we commit and update the global reserve, but the
> space is simply added to bytes_may_use directly, instead of trying to
> add it to existing tickets.  This results in ENOSPC when we really did
> have space.  Fix this by returning the space via
> btrfs_space_info_add_old_bytes.  The global reserve _can_ be using
> overcommitted space, but the add_old_bytes checks this and won't add the
> reservation if we're still overcommitted, so we are safe in this regard.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com> but see below for one
suggestion.

> ---
>  fs/btrfs/block-rsv.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
> index 18a0af20ee5a..394b8fff3a4b 100644
> --- a/fs/btrfs/block-rsv.c
> +++ b/fs/btrfs/block-rsv.c
> @@ -258,6 +258,7 @@ void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
>  	struct btrfs_block_rsv *block_rsv = &fs_info->global_block_rsv;
>  	struct btrfs_space_info *sinfo = block_rsv->space_info;
>  	u64 num_bytes;
> +	u64 to_free = 0;
>  	unsigned min_items;
>  
>  	/*
> @@ -300,9 +301,7 @@ void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
>  		btrfs_space_info_update_bytes_may_use(fs_info, sinfo,
>  						      num_bytes);
>  	} else if (block_rsv->reserved > block_rsv->size) {
> -		num_bytes = block_rsv->reserved - block_rsv->size;
> -		btrfs_space_info_update_bytes_may_use(fs_info, sinfo,
> -						      -num_bytes);
> +		to_free = block_rsv->reserved - block_rsv->size;

nit: Since you hold  sinfo->lock here you could just call the try to
wakeup function, you already have the bytes_may_use update call, that
will also make the to_free variable private to this 'else if' branch.
The only reason to suggest is because further down the sinfo->lock is
released only to be acquired milliseconds later if to_free != 0

>  		block_rsv->reserved = block_rsv->size;
>  	}
>  
> @@ -313,6 +312,9 @@ void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
>  
>  	spin_unlock(&block_rsv->lock);
>  	spin_unlock(&sinfo->lock);
> +
> +	if (to_free)
> +		btrfs_space_info_add_old_bytes(fs_info, sinfo, to_free);
>  }
>  
>  void btrfs_init_global_block_rsv(struct btrfs_fs_info *fs_info)
>

Patch
diff mbox series

diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
index 18a0af20ee5a..394b8fff3a4b 100644
--- a/fs/btrfs/block-rsv.c
+++ b/fs/btrfs/block-rsv.c
@@ -258,6 +258,7 @@  void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
 	struct btrfs_block_rsv *block_rsv = &fs_info->global_block_rsv;
 	struct btrfs_space_info *sinfo = block_rsv->space_info;
 	u64 num_bytes;
+	u64 to_free = 0;
 	unsigned min_items;
 
 	/*
@@ -300,9 +301,7 @@  void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
 		btrfs_space_info_update_bytes_may_use(fs_info, sinfo,
 						      num_bytes);
 	} else if (block_rsv->reserved > block_rsv->size) {
-		num_bytes = block_rsv->reserved - block_rsv->size;
-		btrfs_space_info_update_bytes_may_use(fs_info, sinfo,
-						      -num_bytes);
+		to_free = block_rsv->reserved - block_rsv->size;
 		block_rsv->reserved = block_rsv->size;
 	}
 
@@ -313,6 +312,9 @@  void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
 
 	spin_unlock(&block_rsv->lock);
 	spin_unlock(&sinfo->lock);
+
+	if (to_free)
+		btrfs_space_info_add_old_bytes(fs_info, sinfo, to_free);
 }
 
 void btrfs_init_global_block_rsv(struct btrfs_fs_info *fs_info)