Btrfs: add missing extents release on file extent cluster relocation error
diff mbox series

Message ID 20191009164345.23713-1-fdmanana@kernel.org
State New
Headers show
Series
  • Btrfs: add missing extents release on file extent cluster relocation error
Related show

Commit Message

Filipe Manana Oct. 9, 2019, 4:43 p.m. UTC
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(+)

Comments

Johannes Thumshirn Oct. 10, 2019, 7:13 a.m. UTC | #1
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>
David Sterba Oct. 10, 2019, 4:38 p.m. UTC | #2
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.
David Sterba Oct. 11, 2019, 5:50 p.m. UTC | #3
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.

Patch
diff mbox series

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;
 			}