[PATCH 0/3] 3 misc patches

[PATCH 0/3] 3 misc patches

[Nikolay Borisov]

Here are 3 misc patches. First one is an optimsation to prevent issuing discards
for reserved but not written extents. The other 2 simply get rid of
__btrfs_free_reserved_extent by moving its relevant parts to the two wrappers.

Nikolay Borisov (3):
  btrfs: Don't discard unwritten extents
  btrfs: Open code __btrfs_free_reserved_extent in
    btrfs_free_reserved_extent
  btrfs: Rename __btrfs_free_reserved_extent to
    btrfs_pin_reserved_extent

 fs/btrfs/ctree.h       |  4 ++--
 fs/btrfs/extent-tree.c | 44 ++++++++++++++++++------------------------
 fs/btrfs/inode.c       |  7 ++++++-
 fs/btrfs/tree-log.c    | 12 +++++-------
 4 files changed, 32 insertions(+), 35 deletions(-)

--
2.17.1

[PATCH 1/3] btrfs: Don't discard unwritten extents

[Nikolay Borisov]

All callers of btrfs_free_reserved_extent (respectively
__btrfs_free_reserved_extent with in set to 0) pass in extents which
have only been reserved but not yet written to. Namely,

* In cow_file_range that function is called only if create_io_em fails or
btrfs_add_ordered_extent fail, both of which happen _before_ any IO is
submitted to the newly reserved range.

* In submit_compressed_extents the code flow is similar - out_free_reserve
can be called only before btrfs_submit_compressed_write which is where
any writes to the range could occur.

* btrfs_new_extent_direct also calls btrfs_free_reserved_extent only
if extent_map fails, before any IO is issued.

* __btrfs_prealloc_file_range also calls btrfs_free_reserved_extent
in case insertion of the metadata fails.

* btrfs_alloc_tree_block again can only be called in case in-memory
operations fail, before any IO is submitted.

* btrfs_finish_ordered_io - this is the only caller where discarding
the extent could have a material effect, since it can be called for
an extent which was partially written.

With this change the submission of discards is optimised since discards
are now not being created for extents which are known to not have been
touched on disk.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 2 --
 fs/btrfs/inode.c       | 7 ++++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index fe0f33dd344d..613c7bbf5cbd 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4169,8 +4169,6 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 		if (ret)
 			goto out;
 	} else {
-		if (btrfs_test_opt(fs_info, DISCARD))
-			ret = btrfs_discard_extent(fs_info, start, len, NULL);
 		btrfs_add_free_space(cache, start, len);
 		btrfs_free_reserved_bytes(cache, len, delalloc);
 		trace_btrfs_reserved_extent_free(fs_info, start, len);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0ac0f5b33003..5d80fe030e79 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3250,10 +3250,15 @@ static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent)
 		if ((ret || !logical_len) &&
 		    clear_reserved_extent &&
 		    !test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags) &&
-		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags))
+		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags)) {
 			btrfs_free_reserved_extent(fs_info,
 						   ordered_extent->start,
 						   ordered_extent->disk_len, 1);
+			if (ret && btrfs_test_opt(fs_info, DISCARD))
+				btrfs_discard_extent(fs_info,
+				ordered_extent->start, ordered_extent->disk_len,
+				NULL);
+		}
 	}
 
 
-- 
2.17.1

[PATCH 2/3] btrfs: Open code __btrfs_free_reserved_extent in btrfs_free_reserved_extent

[Nikolay Borisov]

__btrfs_free_reserved_extent performs 2 entirely different operations
depending on whether its 'pin' argument is true or false. This patch
lifts the 2nd case (pin is false) into it's sole caller
btrfs_free_reserved_extent. No semantics changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 613c7bbf5cbd..dae6f8d07852 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4164,17 +4164,7 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 		return -ENOSPC;
 	}
 
-	if (pin) {
-		ret = pin_down_extent(cache, start, len, 1);
-		if (ret)
-			goto out;
-	} else {
-		btrfs_add_free_space(cache, start, len);
-		btrfs_free_reserved_bytes(cache, len, delalloc);
-		trace_btrfs_reserved_extent_free(fs_info, start, len);
-	}
-
-out:
+	ret = pin_down_extent(cache, start, len, 1);
 	btrfs_put_block_group(cache);
 	return ret;
 }
@@ -4182,7 +4172,21 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 			       u64 start, u64 len, int delalloc)
 {
-	return __btrfs_free_reserved_extent(fs_info, start, len, 0, delalloc);
+	struct btrfs_block_group *cache;
+
+	cache = btrfs_lookup_block_group(fs_info, start);
+	if (!cache) {
+		btrfs_err(fs_info, "Unable to find block group for %llu",
+			  start);
+		return -ENOSPC;
+	}
+
+	btrfs_add_free_space(cache, start, len);
+	btrfs_free_reserved_bytes(cache, len, delalloc);
+	trace_btrfs_reserved_extent_free(fs_info, start, len);
+
+	btrfs_put_block_group(cache);
+	return 0;
 }
 
 int btrfs_free_and_pin_reserved_extent(struct btrfs_fs_info *fs_info,
-- 
2.17.1

[PATCH 3/3] btrfs: Rename __btrfs_free_reserved_extent to btrfs_pin_reserved_extent

[Nikolay Borisov]

__btrfs_free_reserved_extent now performs the actions of
btrfs_free_and_pin_reserved_extent. But this name is a bit of a misnomer
, since the extent is not really freed but just pinned. Reflect this
in the new name. No semantics changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h       |  4 ++--
 fs/btrfs/extent-tree.c | 30 +++++++++++-------------------
 fs/btrfs/tree-log.c    | 12 +++++-------
 3 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 5b2196e2778b..fea637806ac4 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2445,8 +2445,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans, struct btrfs_ref *ref);
 
 int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 			       u64 start, u64 len, int delalloc);
-int btrfs_free_and_pin_reserved_extent(struct btrfs_fs_info *fs_info,
-				       u64 start, u64 len);
+int btrfs_pin_reserved_extent(struct btrfs_fs_info *fs_info, u64 start,
+			      u64 len);
 void btrfs_prepare_extent_commit(struct btrfs_fs_info *fs_info);
 int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
 int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index dae6f8d07852..f451dfcca303 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4150,12 +4150,10 @@ int btrfs_reserve_extent(struct btrfs_root *root, u64 ram_bytes,
 	return ret;
 }
 
-static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
-					u64 start, u64 len,
-					int pin, int delalloc)
+int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
+			       u64 start, u64 len, int delalloc)
 {
 	struct btrfs_block_group *cache;
-	int ret = 0;
 
 	cache = btrfs_lookup_block_group(fs_info, start);
 	if (!cache) {
@@ -4164,15 +4162,18 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 		return -ENOSPC;
 	}
 
-	ret = pin_down_extent(cache, start, len, 1);
+	btrfs_add_free_space(cache, start, len);
+	btrfs_free_reserved_bytes(cache, len, delalloc);
+	trace_btrfs_reserved_extent_free(fs_info, start, len);
+
 	btrfs_put_block_group(cache);
-	return ret;
+	return 0;
 }
 
-int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
-			       u64 start, u64 len, int delalloc)
+int btrfs_pin_reserved_extent(struct btrfs_fs_info *fs_info, u64 start, u64 len)
 {
 	struct btrfs_block_group *cache;
+	int ret = 0;
 
 	cache = btrfs_lookup_block_group(fs_info, start);
 	if (!cache) {
@@ -4181,18 +4182,9 @@ int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
 		return -ENOSPC;
 	}
 
-	btrfs_add_free_space(cache, start, len);
-	btrfs_free_reserved_bytes(cache, len, delalloc);
-	trace_btrfs_reserved_extent_free(fs_info, start, len);
-
+	ret = pin_down_extent(cache, start, len, 1);
 	btrfs_put_block_group(cache);
-	return 0;
-}
-
-int btrfs_free_and_pin_reserved_extent(struct btrfs_fs_info *fs_info,
-				       u64 start, u64 len)
-{
-	return __btrfs_free_reserved_extent(fs_info, start, len, 1, 0);
+	return ret;
 }
 
 static int alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 6f757361db53..7321f1da3dd7 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2731,9 +2731,8 @@ static noinline int walk_down_log_tree(struct btrfs_trans_handle *trans,
 
 				WARN_ON(root_owner !=
 					BTRFS_TREE_LOG_OBJECTID);
-				ret = btrfs_free_and_pin_reserved_extent(
-							fs_info, bytenr,
-							blocksize);
+				ret = btrfs_pin_reserved_extent(fs_info,
+							bytenr, blocksize);
 				if (ret) {
 					free_extent_buffer(next);
 					return ret;
@@ -2814,8 +2813,7 @@ static noinline int walk_up_log_tree(struct btrfs_trans_handle *trans,
 				}
 
 				WARN_ON(root_owner != BTRFS_TREE_LOG_OBJECTID);
-				ret = btrfs_free_and_pin_reserved_extent(
-						fs_info,
+				ret = btrfs_pin_reserved_extent(fs_info,
 						path->nodes[*level]->start,
 						path->nodes[*level]->len);
 				if (ret)
@@ -2897,8 +2895,8 @@ static int walk_log_tree(struct btrfs_trans_handle *trans,
 
 			WARN_ON(log->root_key.objectid !=
 				BTRFS_TREE_LOG_OBJECTID);
-			ret = btrfs_free_and_pin_reserved_extent(fs_info,
-							next->start, next->len);
+			ret = btrfs_pin_reserved_extent(fs_info, next->start,
+							next->len);
 			if (ret)
 				goto out;
 		}
-- 
2.17.1

Re: [PATCH 1/3] btrfs: Don't discard unwritten extents

[David Sterba]

On Thu, Nov 21, 2019 at 02:03:29PM +0200, Nikolay Borisov wrote:
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4169,8 +4169,6 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>  		if (ret)
>  			goto out;
>  	} else {
> -		if (btrfs_test_opt(fs_info, DISCARD))
> -			ret = btrfs_discard_extent(fs_info, start, len, NULL);
>  		btrfs_add_free_space(cache, start, len);
>  		btrfs_free_reserved_bytes(cache, len, delalloc);
>  		trace_btrfs_reserved_extent_free(fs_info, start, len);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 0ac0f5b33003..5d80fe030e79 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3250,10 +3250,15 @@ static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent)
>  		if ((ret || !logical_len) &&
>  		    clear_reserved_extent &&
>  		    !test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags) &&
> -		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags))
> +		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags)) {
>  			btrfs_free_reserved_extent(fs_info,
>  						   ordered_extent->start,
>  						   ordered_extent->disk_len, 1);
> +			if (ret && btrfs_test_opt(fs_info, DISCARD))
> +				btrfs_discard_extent(fs_info,
> +				ordered_extent->start, ordered_extent->disk_len,
> +				NULL);

This brings back vague memories of misplaced discard (in
finish_ordered_io), that was quite hard to catch. I can't find the fix
though. Filipe, is it the same issue?

Re: [PATCH 1/3] btrfs: Don't discard unwritten extents

[David Sterba]

On Wed, Nov 27, 2019 at 05:06:00PM +0100, David Sterba wrote:
> On Thu, Nov 21, 2019 at 02:03:29PM +0200, Nikolay Borisov wrote:
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -4169,8 +4169,6 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
> >  		if (ret)
> >  			goto out;
> >  	} else {
> > -		if (btrfs_test_opt(fs_info, DISCARD))
> > -			ret = btrfs_discard_extent(fs_info, start, len, NULL);
> >  		btrfs_add_free_space(cache, start, len);
> >  		btrfs_free_reserved_bytes(cache, len, delalloc);
> >  		trace_btrfs_reserved_extent_free(fs_info, start, len);
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 0ac0f5b33003..5d80fe030e79 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -3250,10 +3250,15 @@ static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent)
> >  		if ((ret || !logical_len) &&
> >  		    clear_reserved_extent &&
> >  		    !test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags) &&
> > -		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags))
> > +		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags)) {
> >  			btrfs_free_reserved_extent(fs_info,
> >  						   ordered_extent->start,
> >  						   ordered_extent->disk_len, 1);
> > +			if (ret && btrfs_test_opt(fs_info, DISCARD))
> > +				btrfs_discard_extent(fs_info,
> > +				ordered_extent->start, ordered_extent->disk_len,
> > +				NULL);
> 
> This brings back vague memories of misplaced discard (in
> finish_ordered_io), that was quite hard to catch. I can't find the fix
> though. Filipe, is it the same issue?

Closest to what I thought are dcc82f4783ad9 "Btrfs: fix log tree
corruption when fs mounted with -o discard" or 678886bdc6378 "Btrfs: fix
fs corruption on transaction abort if device supports discard".

Re: [PATCH 1/3] btrfs: Don't discard unwritten extents

[Nikolay Borisov]

On 27.11.19 г. 18:15 ч., David Sterba wrote:
> On Wed, Nov 27, 2019 at 05:06:00PM +0100, David Sterba wrote:
>> On Thu, Nov 21, 2019 at 02:03:29PM +0200, Nikolay Borisov wrote:
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -4169,8 +4169,6 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>>>  		if (ret)
>>>  			goto out;
>>>  	} else {
>>> -		if (btrfs_test_opt(fs_info, DISCARD))
>>> -			ret = btrfs_discard_extent(fs_info, start, len, NULL);
>>>  		btrfs_add_free_space(cache, start, len);
>>>  		btrfs_free_reserved_bytes(cache, len, delalloc);
>>>  		trace_btrfs_reserved_extent_free(fs_info, start, len);
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 0ac0f5b33003..5d80fe030e79 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -3250,10 +3250,15 @@ static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent)
>>>  		if ((ret || !logical_len) &&
>>>  		    clear_reserved_extent &&
>>>  		    !test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags) &&
>>> -		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags))
>>> +		    !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags)) {
>>>  			btrfs_free_reserved_extent(fs_info,
>>>  						   ordered_extent->start,
>>>  						   ordered_extent->disk_len, 1);
>>> +			if (ret && btrfs_test_opt(fs_info, DISCARD))
>>> +				btrfs_discard_extent(fs_info,
>>> +				ordered_extent->start, ordered_extent->disk_len,
>>> +				NULL);
>>
>> This brings back vague memories of misplaced discard (in
>> finish_ordered_io), that was quite hard to catch. I can't find the fix
>> though. Filipe, is it the same issue?
> 
> Closest to what I thought are dcc82f4783ad9 "Btrfs: fix log tree
> corruption when fs mounted with -o discard" or 678886bdc6378 "Btrfs: fix
> fs corruption on transaction abort if device supports discard".
> 

Pinned extents are discarded when btrfs_finish_extent_commit is called
from transaction commit. If you look closer you'd see
btrfs_Free_Reserved_extent here means we call into
__btrfs_Free_Reserved_extent with pin equal to false, so the original
code would have gone to the else {} clause where the discard was. By
predicating on (ret) I only handle the case where an ordered extent
(which only applies to data) is potentially partially written.

Re: [PATCH 1/3] btrfs: Don't discard unwritten extents

[Filipe Manana]

On Wed, Nov 27, 2019 at 4:08 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Thu, Nov 21, 2019 at 02:03:29PM +0200, Nikolay Borisov wrote:
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -4169,8 +4169,6 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
> >               if (ret)
> >                       goto out;
> >       } else {
> > -             if (btrfs_test_opt(fs_info, DISCARD))
> > -                     ret = btrfs_discard_extent(fs_info, start, len, NULL);
> >               btrfs_add_free_space(cache, start, len);
> >               btrfs_free_reserved_bytes(cache, len, delalloc);
> >               trace_btrfs_reserved_extent_free(fs_info, start, len);
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 0ac0f5b33003..5d80fe030e79 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -3250,10 +3250,15 @@ static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent)
> >               if ((ret || !logical_len) &&
> >                   clear_reserved_extent &&
> >                   !test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags) &&
> > -                 !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags))
> > +                 !test_bit(BTRFS_ORDERED_PREALLOC, &ordered_extent->flags)) {
> >                       btrfs_free_reserved_extent(fs_info,
> >                                                  ordered_extent->start,
> >                                                  ordered_extent->disk_len, 1);
> > +                     if (ret && btrfs_test_opt(fs_info, DISCARD))
> > +                             btrfs_discard_extent(fs_info,
> > +                             ordered_extent->start, ordered_extent->disk_len,
> > +                             NULL);
>
> This brings back vague memories of misplaced discard (in
> finish_ordered_io), that was quite hard to catch. I can't find the fix
> though. Filipe, is it the same issue?

It's different (as you identified already in another reply).
Even though this is rarely hit on a healthy system, it looks fine to me.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

thanks



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 2/3] btrfs: Open code __btrfs_free_reserved_extent in btrfs_free_reserved_extent

[David Sterba]

On Thu, Nov 21, 2019 at 02:03:30PM +0200, Nikolay Borisov wrote:
> __btrfs_free_reserved_extent performs 2 entirely different operations
> depending on whether its 'pin' argument is true or false. This patch
> lifts the 2nd case (pin is false) into it's sole caller
> btrfs_free_reserved_extent. No semantics changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 613c7bbf5cbd..dae6f8d07852 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4164,17 +4164,7 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>  		return -ENOSPC;
>  	}
>  
> -	if (pin) {
> -		ret = pin_down_extent(cache, start, len, 1);
> -		if (ret)
> -			goto out;
> -	} else {
> -		btrfs_add_free_space(cache, start, len);
> -		btrfs_free_reserved_bytes(cache, len, delalloc);
> -		trace_btrfs_reserved_extent_free(fs_info, start, len);
> -	}
> -
> -out:
> +	ret = pin_down_extent(cache, start, len, 1);
>  	btrfs_put_block_group(cache);
>  	return ret;
>  }
> @@ -4182,7 +4172,21 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>  int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>  			       u64 start, u64 len, int delalloc)
>  {
> -	return __btrfs_free_reserved_extent(fs_info, start, len, 0, delalloc);
> +	struct btrfs_block_group *cache;
> +
> +	cache = btrfs_lookup_block_group(fs_info, start);
> +	if (!cache) {
> +		btrfs_err(fs_info, "Unable to find block group for %llu",
> +			  start);

I think the error message should be either removed or at least hidden
under enospc_debug. This has little value to a normal user and also the
function can be indirectly called many times, spamming logs.

Re: [PATCH 3/3] btrfs: Rename __btrfs_free_reserved_extent to btrfs_pin_reserved_extent

[David Sterba]

On Thu, Nov 21, 2019 at 02:03:31PM +0200, Nikolay Borisov wrote:
> -	ret = pin_down_extent(cache, start, len, 1);
> +	btrfs_add_free_space(cache, start, len);
> +	btrfs_free_reserved_bytes(cache, len, delalloc);
> +	trace_btrfs_reserved_extent_free(fs_info, start, len);

Unrelated, this patch is only moving existing code, but
btrfs_add_free_space and btrfs_free_reserved_bytes don't have their
errors handled. In many other places too.

Re: [PATCH 2/3] btrfs: Open code __btrfs_free_reserved_extent in btrfs_free_reserved_extent

[Nikolay Borisov]

On 27.11.19 г. 20:55 ч., David Sterba wrote:
> On Thu, Nov 21, 2019 at 02:03:30PM +0200, Nikolay Borisov wrote:
>> __btrfs_free_reserved_extent performs 2 entirely different operations
>> depending on whether its 'pin' argument is true or false. This patch
>> lifts the 2nd case (pin is false) into it's sole caller
>> btrfs_free_reserved_extent. No semantics changes.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/extent-tree.c | 28 ++++++++++++++++------------
>>  1 file changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 613c7bbf5cbd..dae6f8d07852 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -4164,17 +4164,7 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>>  		return -ENOSPC;
>>  	}
>>  
>> -	if (pin) {
>> -		ret = pin_down_extent(cache, start, len, 1);
>> -		if (ret)
>> -			goto out;
>> -	} else {
>> -		btrfs_add_free_space(cache, start, len);
>> -		btrfs_free_reserved_bytes(cache, len, delalloc);
>> -		trace_btrfs_reserved_extent_free(fs_info, start, len);
>> -	}
>> -
>> -out:
>> +	ret = pin_down_extent(cache, start, len, 1);
>>  	btrfs_put_block_group(cache);
>>  	return ret;
>>  }
>> @@ -4182,7 +4172,21 @@ static int __btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>>  int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
>>  			       u64 start, u64 len, int delalloc)
>>  {
>> -	return __btrfs_free_reserved_extent(fs_info, start, len, 0, delalloc);
>> +	struct btrfs_block_group *cache;
>> +
>> +	cache = btrfs_lookup_block_group(fs_info, start);
>> +	if (!cache) {
>> +		btrfs_err(fs_info, "Unable to find block group for %llu",
>> +			  start);
> 
> I think the error message should be either removed or at least hidden
> under enospc_debug. This has little value to a normal user and also the
> function can be indirectly called many times, spamming logs.

True but in general this should never happen because if we are freeing
an extent then it must have been reserved from a particular block group.
So if this triggers then we know something is awfully amiss.

> 

Re: [PATCH 0/3] 3 misc patches

[David Sterba]

On Thu, Nov 21, 2019 at 02:03:28PM +0200, Nikolay Borisov wrote:
> Here are 3 misc patches. First one is an optimsation to prevent issuing discards
> for reserved but not written extents. The other 2 simply get rid of
> __btrfs_free_reserved_extent by moving its relevant parts to the two wrappers.
> 
> Nikolay Borisov (3):
>   btrfs: Don't discard unwritten extents
>   btrfs: Open code __btrfs_free_reserved_extent in
>     btrfs_free_reserved_extent
>   btrfs: Rename __btrfs_free_reserved_extent to
>     btrfs_pin_reserved_extent

Added to misc-next, thanks.