Btrfs: fix metadata space leak on fixup worker failure to set range as delalloc
diff mbox series

Message ID 20191009164359.29642-1-fdmanana@kernel.org
State New
Headers show
Series
  • Btrfs: fix metadata space leak on fixup worker failure to set range as delalloc
Related show

Commit Message

Filipe Manana Oct. 9, 2019, 4:43 p.m. UTC
From: Filipe Manana <fdmanana@suse.com>

In the fixup worker, if we fail to mark the range as delalloc in the io
tree, we must release the previously reserved metadata, as well as update
the outstanding extents counter for the inode, otherwise we leak metadata
space.

In pratice we can't return an error from btrfs_set_extent_delalloc(),
which is just a wrapper around __set_extent_bit(), as for most errors
__set_extent_bit() does a BUG_ON() (or panics which hits a BUG_ON() as
well) and returning an -EEXIST error doesn't happen in this case since
the exclusive bits parameter always has a value of 0 through this code
path. Nevertheless, just fix the error handling in the fixup worker,
in case one day __set_extent_bit() can return an error to this code
path.

Fixes: f3038ee3a3f101 ("btrfs: Handle btrfs_set_extent_delalloc failure in fixup worker")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Nikolay Borisov Oct. 11, 2019, 7 a.m. UTC | #1
On 9.10.19 г. 19:43 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> In the fixup worker, if we fail to mark the range as delalloc in the io
> tree, we must release the previously reserved metadata, as well as update
> the outstanding extents counter for the inode, otherwise we leak metadata
> space.
> 
> In pratice we can't return an error from btrfs_set_extent_delalloc(),
> which is just a wrapper around __set_extent_bit(), as for most errors
> __set_extent_bit() does a BUG_ON() (or panics which hits a BUG_ON() as
> well) and returning an -EEXIST error doesn't happen in this case since
> the exclusive bits parameter always has a value of 0 through this code
> path. Nevertheless, just fix the error handling in the fixup worker,
> in case one day __set_extent_bit() can return an error to this code
> path.
> 
> Fixes: f3038ee3a3f101 ("btrfs: Handle btrfs_set_extent_delalloc failure in fixup worker")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

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

> ---
>  fs/btrfs/inode.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 0f2754eaa05b..f23b14ec743a 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2201,12 +2201,16 @@ static void btrfs_writepage_fixup_worker(struct btrfs_work *work)
>  		mapping_set_error(page->mapping, ret);
>  		end_extent_writepage(page, ret, page_start, page_end);
>  		ClearPageChecked(page);
> -		goto out;
> +		goto out_reserved;
>  	}
>  
>  	ClearPageChecked(page);
>  	set_page_dirty(page);
> -	btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, false);
> +out_reserved:
> +	btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, ret != 0);
> +	if (ret)
> +		btrfs_delalloc_release_space(inode, data_reserved, page_start,
> +					     PAGE_SIZE, true);
>  out:
>  	unlock_extent_cached(&BTRFS_I(inode)->io_tree, page_start, page_end,
>  			     &cached_state);
>
David Sterba Oct. 11, 2019, 6:38 p.m. UTC | #2
On Wed, Oct 09, 2019 at 05:43:59PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> In the fixup worker, if we fail to mark the range as delalloc in the io
> tree, we must release the previously reserved metadata, as well as update
> the outstanding extents counter for the inode, otherwise we leak metadata
> space.
> 
> In pratice we can't return an error from btrfs_set_extent_delalloc(),
> which is just a wrapper around __set_extent_bit(), as for most errors
> __set_extent_bit() does a BUG_ON() (or panics which hits a BUG_ON() as
> well) and returning an -EEXIST error doesn't happen in this case since
> the exclusive bits parameter always has a value of 0 through this code
> path. Nevertheless, just fix the error handling in the fixup worker,
> in case one day __set_extent_bit() can return an error to this code
> path.
> 
> Fixes: f3038ee3a3f101 ("btrfs: Handle btrfs_set_extent_delalloc failure in fixup worker")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.

> @@ -2201,12 +2201,16 @@ static void btrfs_writepage_fixup_worker(struct btrfs_work *work)
>  		mapping_set_error(page->mapping, ret);
>  		end_extent_writepage(page, ret, page_start, page_end);
>  		ClearPageChecked(page);
> -		goto out;
> +		goto out_reserved;
>  	}
>  
>  	ClearPageChecked(page);
>  	set_page_dirty(page);
> -	btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, false);
> +out_reserved:
> +	btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, ret != 0);

This is a shortcut to avoid extra variable to track the status of the
3rd parameter (qgroup_free) but as the goto and label are only a few
lines apart, I guess it's ok.

> +	if (ret)
> +		btrfs_delalloc_release_space(inode, data_reserved, page_start,
> +					     PAGE_SIZE, true);

Patch
diff mbox series

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0f2754eaa05b..f23b14ec743a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2201,12 +2201,16 @@  static void btrfs_writepage_fixup_worker(struct btrfs_work *work)
 		mapping_set_error(page->mapping, ret);
 		end_extent_writepage(page, ret, page_start, page_end);
 		ClearPageChecked(page);
-		goto out;
+		goto out_reserved;
 	}
 
 	ClearPageChecked(page);
 	set_page_dirty(page);
-	btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, false);
+out_reserved:
+	btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, ret != 0);
+	if (ret)
+		btrfs_delalloc_release_space(inode, data_reserved, page_start,
+					     PAGE_SIZE, true);
 out:
 	unlock_extent_cached(&BTRFS_I(inode)->io_tree, page_start, page_end,
 			     &cached_state);