[PATCH] Btrfs: add missing extents release on file extent cluster relocation error

[PATCH] Btrfs: add missing extents release on file extent cluster relocation error

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we error out when finding a page at relocate_file_extent_cluster(), we
need to release the outstanding extents counter on the relocation inode,
set by the previous call to btrfs_delalloc_reserve_metadata(), otherwise
the inode's block reserve size can never decrease to zero and metadata
space is leaked. Therefore add a call to btrfs_delalloc_release_extents()
in case we can't find the target page.

Fixes: 8b62f87bad9cf0 ("Btrfs: rework outstanding_extents")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/relocation.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 00504657b602..88dbc0127793 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3277,6 +3277,8 @@ static int relocate_file_extent_cluster(struct inode *inode,
 			if (!page) {
 				btrfs_delalloc_release_metadata(BTRFS_I(inode),
 							PAGE_SIZE, true);
+				btrfs_delalloc_release_extents(BTRFS_I(inode),
+							       PAGE_SIZE, true);
 				ret = -ENOMEM;
 				goto out;
 			}
-- 
2.11.0

Re: [PATCH] Btrfs: add missing extents release on file extent cluster relocation error

[Johannes Thumshirn]

On 09/10/2019 18:43, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we error out when finding a page at relocate_file_extent_cluster(), we
> need to release the outstanding extents counter on the relocation inode,
> set by the previous call to btrfs_delalloc_reserve_metadata(), otherwise
> the inode's block reserve size can never decrease to zero and metadata
> space is leaked. Therefore add a call to btrfs_delalloc_release_extents()
> in case we can't find the target page.
> 
> Fixes: 8b62f87bad9cf0 ("Btrfs: rework outstanding_extents")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/relocation.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 00504657b602..88dbc0127793 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -3277,6 +3277,8 @@ static int relocate_file_extent_cluster(struct inode *inode,
>  			if (!page) {
>  				btrfs_delalloc_release_metadata(BTRFS_I(inode),
>  							PAGE_SIZE, true);
> +				btrfs_delalloc_release_extents(BTRFS_I(inode),
> +							       PAGE_SIZE, true);

Hmm how about adding a wrapper to combine these two calls similar to
what btrfs_delalloc_release_space() is doing for
btrfs_delalloc_release_metadata() and btrfs_free_reserved_data_space()?

I count at least 3 other occurences of this pattern with a simple
git grep -C 4 btrfs_delalloc_release_metadata fs/btrfs/ | \
   grep btrfs_delalloc_release_extents

One of them being in the same function.

Otherwise
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 247165, AG München)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] Btrfs: add missing extents release on file extent cluster relocation error

[David Sterba]

On Thu, Oct 10, 2019 at 09:13:43AM +0200, Johannes Thumshirn wrote:
> On 09/10/2019 18:43, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > If we error out when finding a page at relocate_file_extent_cluster(), we
> > need to release the outstanding extents counter on the relocation inode,
> > set by the previous call to btrfs_delalloc_reserve_metadata(), otherwise
> > the inode's block reserve size can never decrease to zero and metadata
> > space is leaked. Therefore add a call to btrfs_delalloc_release_extents()
> > in case we can't find the target page.
> > 
> > Fixes: 8b62f87bad9cf0 ("Btrfs: rework outstanding_extents")
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >  fs/btrfs/relocation.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > index 00504657b602..88dbc0127793 100644
> > --- a/fs/btrfs/relocation.c
> > +++ b/fs/btrfs/relocation.c
> > @@ -3277,6 +3277,8 @@ static int relocate_file_extent_cluster(struct inode *inode,
> >  			if (!page) {
> >  				btrfs_delalloc_release_metadata(BTRFS_I(inode),
> >  							PAGE_SIZE, true);
> > +				btrfs_delalloc_release_extents(BTRFS_I(inode),
> > +							       PAGE_SIZE, true);
> 
> Hmm how about adding a wrapper to combine these two calls similar to
> what btrfs_delalloc_release_space() is doing for
> btrfs_delalloc_release_metadata() and btrfs_free_reserved_data_space()?
> 
> I count at least 3 other occurences of this pattern with a simple
> git grep -C 4 btrfs_delalloc_release_metadata fs/btrfs/ | \
>    grep btrfs_delalloc_release_extents
> 
> One of them being in the same function.

I'm not sure another wrapper would be a significant improvement. The two
functions are called separatelly in many places so it has to be decided
case by case anyway.

Reading relocate_file_extent_cluster, there are places that call only
one of the functions (due to partial initialization) and that need both,
so that it's explicitly visible can be matched agains the context. Eg.
hilighting the function in the editor is a visual clue that I'm using
often so the extra wrapper would slightly obscure that.

Re: [PATCH] Btrfs: add missing extents release on file extent cluster relocation error

[David Sterba]

On Wed, Oct 09, 2019 at 05:43:45PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we error out when finding a page at relocate_file_extent_cluster(), we
> need to release the outstanding extents counter on the relocation inode,
> set by the previous call to btrfs_delalloc_reserve_metadata(), otherwise
> the inode's block reserve size can never decrease to zero and metadata
> space is leaked. Therefore add a call to btrfs_delalloc_release_extents()
> in case we can't find the target page.
> 
> Fixes: 8b62f87bad9cf0 ("Btrfs: rework outstanding_extents")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.