Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Btrfs: add missing extents release on file extent cluster relocation error
@ 2019-10-09 16:43 fdmanana
  2019-10-10  7:13 ` Johannes Thumshirn
  2019-10-11 17:50 ` David Sterba
  0 siblings, 2 replies; 4+ messages in thread
From: fdmanana @ 2019-10-09 16:43 UTC (permalink / raw)
  To: linux-btrfs

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Btrfs: add missing extents release on file extent cluster relocation error
  2019-10-09 16:43 [PATCH] Btrfs: add missing extents release on file extent cluster relocation error fdmanana
@ 2019-10-10  7:13 ` Johannes Thumshirn
  2019-10-10 16:38   ` David Sterba
  2019-10-11 17:50 ` David Sterba
  1 sibling, 1 reply; 4+ messages in thread
From: Johannes Thumshirn @ 2019-10-10  7:13 UTC (permalink / raw)
  To: fdmanana, linux-btrfs

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Btrfs: add missing extents release on file extent cluster relocation error
  2019-10-10  7:13 ` Johannes Thumshirn
@ 2019-10-10 16:38   ` David Sterba
  0 siblings, 0 replies; 4+ messages in thread
From: David Sterba @ 2019-10-10 16:38 UTC (permalink / raw)
  To: Johannes Thumshirn; +Cc: fdmanana, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Btrfs: add missing extents release on file extent cluster relocation error
  2019-10-09 16:43 [PATCH] Btrfs: add missing extents release on file extent cluster relocation error fdmanana
  2019-10-10  7:13 ` Johannes Thumshirn
@ 2019-10-11 17:50 ` David Sterba
  1 sibling, 0 replies; 4+ messages in thread
From: David Sterba @ 2019-10-11 17:50 UTC (permalink / raw)
  To: fdmanana; +Cc: linux-btrfs

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09 16:43 [PATCH] Btrfs: add missing extents release on file extent cluster relocation error fdmanana
2019-10-10  7:13 ` Johannes Thumshirn
2019-10-10 16:38   ` David Sterba
2019-10-11 17:50 ` David Sterba

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox