[PATCH] btrfs: return 0 for success in btrfs_alloc_data_chunk_ondemand

[PATCH] btrfs: return 0 for success in btrfs_alloc_data_chunk_ondemand

[Josef Bacik]

From: Josef Bacik <jbacik@fb.com>

My delayed refs rsv patches uncovered a problem in
btrfs_alloc_data_chunk_ondemand where we don't clear ret before
returning, so we could have whatever left over value we had from trying
to do a chunk allocation or whatever that may have failed.  Since we
know we've succeeded at this point just unconditionally return 0.  This
fixed the xfstests failures I was seeing with my delayed refs rsv
patches.

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/extent-tree.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index dbae25d882de..33c9efbfc9a7 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4387,8 +4387,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes)
 	trace_btrfs_space_reservation(fs_info, "space_info",
 				      data_sinfo->flags, bytes, 1);
 	spin_unlock(&data_sinfo->lock);
-
-	return ret;
+	return 0;
 }
 
 int btrfs_check_data_free_space(struct inode *inode,
-- 
2.7.5

Re: [PATCH] btrfs: return 0 for success in btrfs_alloc_data_chunk_ondemand

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1387 bytes --]



On 2018年01月11日 00:55, Josef Bacik wrote:
> From: Josef Bacik <jbacik@fb.com>
> 
> My delayed refs rsv patches uncovered a problem in
> btrfs_alloc_data_chunk_ondemand where we don't clear ret before
> returning, so we could have whatever left over value we had from trying
> to do a chunk allocation or whatever that may have failed.  Since we
> know we've succeeded at this point just unconditionally return 0.  This
> fixed the xfstests failures I was seeing with my delayed refs rsv
> patches.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

The branch to leave ret = 1 would be the "ret = do_chunk_alloc()" call,
which could return 1, and goes to final return.
So the fix is good.

Thanks,
Qu

> ---
>  fs/btrfs/extent-tree.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index dbae25d882de..33c9efbfc9a7 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4387,8 +4387,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes)
>  	trace_btrfs_space_reservation(fs_info, "space_info",
>  				      data_sinfo->flags, bytes, 1);
>  	spin_unlock(&data_sinfo->lock);
> -
> -	return ret;
> +	return 0;
>  }
>  
>  int btrfs_check_data_free_space(struct inode *inode,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH] btrfs: return 0 for success in btrfs_alloc_data_chunk_ondemand

[Nikolay Borisov]

On 10.01.2018 18:55, Josef Bacik wrote:
> From: Josef Bacik <jbacik@fb.com>
> 
> My delayed refs rsv patches uncovered a problem in
> btrfs_alloc_data_chunk_ondemand where we don't clear ret before
> returning, so we could have whatever left over value we had from trying
> to do a chunk allocation or whatever that may have failed.  Since we
> know we've succeeded at this point just unconditionally return 0.  This
> fixed the xfstests failures I was seeing with my delayed refs rsv
> patches.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/extent-tree.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index dbae25d882de..33c9efbfc9a7 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4387,8 +4387,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes)
>  	trace_btrfs_space_reservation(fs_info, "space_info",
>  				      data_sinfo->flags, bytes, 1);
>  	spin_unlock(&data_sinfo->lock);
> -
> -	return ret;
> +	return 0;
>  }
>  

While this patch indeed seems to fix some failure I was seeing with
non-empty delalloc resv I fail to see why returning 1 could cause them.

In btrfs_check_data_free_space we only check if ret is < 0 and after
that we call btrfs_qgroup_reserve_data so this can't cause an issue.

Then we have usage in btrfs_zero_range which is a new code and is not
really used-per se in the tests that I saw failing.

So the last remaining possible culprit is the usage in btrfs_fallocate
(and indeed adding printk's shows that everytime a test fail we indeed
returned 1 from alloc_data_chunk. However, looking at the code in
fallocate ret is always overwritten before being checked. So how exactly
is returning a positive value influencing the freeing of per-inode block
rsvs?

>  int btrfs_check_data_free_space(struct inode *inode,
> 

Re: [PATCH] btrfs: return 0 for success in btrfs_alloc_data_chunk_ondemand

[David Sterba]

On Wed, Jan 10, 2018 at 11:55:34AM -0500, Josef Bacik wrote:
> From: Josef Bacik <jbacik@fb.com>
> 
> My delayed refs rsv patches uncovered a problem in
> btrfs_alloc_data_chunk_ondemand where we don't clear ret before
> returning, so we could have whatever left over value we had from trying
> to do a chunk allocation or whatever that may have failed.  Since we
> know we've succeeded at this point just unconditionally return 0.  This
> fixed the xfstests failures I was seeing with my delayed refs rsv
> patches.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  fs/btrfs/extent-tree.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index dbae25d882de..33c9efbfc9a7 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4387,8 +4387,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes)
>  	trace_btrfs_space_reservation(fs_info, "space_info",
>  				      data_sinfo->flags, bytes, 1);
>  	spin_unlock(&data_sinfo->lock);
> -
> -	return ret;
> +	return 0;

I don't think this is the right way to fix it. The return code of
do_chunk_alloc depends on the force parameter, and in case it's
CHUNK_ALLOC_NO_FORCE the caller should handle all the possibilities, ie.
negative/0/positive. Other callers do that, so I'd rather see it fixed
right after do_chunk_alloc and not forcing 0.