[2/5] btrfs: always reserve our entire size for the global reserve
diff mbox series

Message ID 20190816152019.1962-3-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
While messing with the overcommit logic I noticed that sometimes we'd
ENOSPC out when really we should have run out of space much earlier.  It
turns out it's because we'll only reserve up to the free amount left in
the space info for the global reserve, but that doesn't make sense with
overcommit because we could be well above our actual size.  This results
in the global reserve not carving out it's entire reservation, and thus
not putting enough pressure on the rest of the infrastructure to do the
right thing and ENOSPC out at a convenient time.  Fix this by always
taking our full reservation amount for the global reserve.

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

Comments

Nikolay Borisov Aug. 20, 2019, 2:23 p.m. UTC | #1
On 16.08.19 г. 18:20 ч., Josef Bacik wrote:
> While messing with the overcommit logic I noticed that sometimes we'd
> ENOSPC out when really we should have run out of space much earlier.  It
> turns out it's because we'll only reserve up to the free amount left in
> the space info for the global reserve, but that doesn't make sense with
> overcommit because we could be well above our actual size.  This results
> in the global reserve not carving out it's entire reservation, and thus
> not putting enough pressure on the rest of the infrastructure to do the
> right thing and ENOSPC out at a convenient time.  Fix this by always
> taking our full reservation amount for the global reserve.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>


In effect you could possibly overcommit by increasing bytes_may_use you
potentially cause callers of reserve_metadata_bytes to get ENOSPC, am I
right? In any case the code itself looks ok:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/block-rsv.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
> index 657675eef443..18a0af20ee5a 100644
> --- a/fs/btrfs/block-rsv.c
> +++ b/fs/btrfs/block-rsv.c
> @@ -295,15 +295,10 @@ void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
>  	block_rsv->size = min_t(u64, num_bytes, SZ_512M);
>  
>  	if (block_rsv->reserved < block_rsv->size) {
> -		num_bytes = btrfs_space_info_used(sinfo, true);
> -		if (sinfo->total_bytes > num_bytes) {
> -			num_bytes = sinfo->total_bytes - num_bytes;
> -			num_bytes = min(num_bytes,
> -					block_rsv->size - block_rsv->reserved);
> -			block_rsv->reserved += num_bytes;
> -			btrfs_space_info_update_bytes_may_use(fs_info, sinfo,
> -							      num_bytes);
> -		}
> +		num_bytes = block_rsv->size - block_rsv->reserved;
> +		block_rsv->reserved += num_bytes;
> +		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,
>
Josef Bacik Aug. 20, 2019, 7:13 p.m. UTC | #2
On Tue, Aug 20, 2019 at 05:23:29PM +0300, Nikolay Borisov wrote:
> 
> 
> On 16.08.19 г. 18:20 ч., Josef Bacik wrote:
> > While messing with the overcommit logic I noticed that sometimes we'd
> > ENOSPC out when really we should have run out of space much earlier.  It
> > turns out it's because we'll only reserve up to the free amount left in
> > the space info for the global reserve, but that doesn't make sense with
> > overcommit because we could be well above our actual size.  This results
> > in the global reserve not carving out it's entire reservation, and thus
> > not putting enough pressure on the rest of the infrastructure to do the
> > right thing and ENOSPC out at a convenient time.  Fix this by always
> > taking our full reservation amount for the global reserve.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> 
> In effect you could possibly overcommit by increasing bytes_may_use you
> potentially cause callers of reserve_metadata_bytes to get ENOSPC, am I
> right? In any case the code itself looks ok:
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 

Right, again this is mostly for xfstests.  We need to be able to provide the
appropriate enospc pressure to make sure that we have enough space for the
global reserve.  In practice with real file systems we never end up pushing
ourselves into overcommit with the giant global reserve by itself.  Thanks,

Josef

Patch
diff mbox series

diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
index 657675eef443..18a0af20ee5a 100644
--- a/fs/btrfs/block-rsv.c
+++ b/fs/btrfs/block-rsv.c
@@ -295,15 +295,10 @@  void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
 	block_rsv->size = min_t(u64, num_bytes, SZ_512M);
 
 	if (block_rsv->reserved < block_rsv->size) {
-		num_bytes = btrfs_space_info_used(sinfo, true);
-		if (sinfo->total_bytes > num_bytes) {
-			num_bytes = sinfo->total_bytes - num_bytes;
-			num_bytes = min(num_bytes,
-					block_rsv->size - block_rsv->reserved);
-			block_rsv->reserved += num_bytes;
-			btrfs_space_info_update_bytes_may_use(fs_info, sinfo,
-							      num_bytes);
-		}
+		num_bytes = block_rsv->size - block_rsv->reserved;
+		block_rsv->reserved += num_bytes;
+		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,