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

[David Sterba <dsterba@suse.cz>]

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.