[PATCH 0/4] Misc cleanups

[PATCH 0/4] Misc cleanups

[Nikolay Borisov]

Here's a collection of various cleanups I've done while working on other things. 
All of these should be pretty low-risk, have soaked for a while on my test 
branch and survived multiple xfstest runs. 

They strive to mostly untangle the code and make it more readable. 

Nikolay Borisov (4):
  btrfs: Use while loop instead of labels in __endio_write_update_ordered
  btrfs: Fix lock release order
  btrfs: Consolidate error checking for btrfs_alloc_chunk
  btrfs: Rewrite retry logic in do_chunk_alloc

 fs/btrfs/extent-tree.c | 79 +++++++++++++++++++++++---------------------------
 fs/btrfs/inode.c       | 52 ++++++++++++++++-----------------
 2 files changed, 61 insertions(+), 70 deletions(-)

-- 
2.7.4

[PATCH 1/4] btrfs: Use while loop instead of labels in __endio_write_update_ordered

[Nikolay Borisov]

Currently __endio_write_update_ordered uses labels to implement
what is essentially a simple while loop. This makes the code more
cumbersome to follow than it actually has to be. No functional
changes. No xfstest regressions were found during testing.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 52 +++++++++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ddaed8369874..16f688a4b92d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8159,7 +8159,6 @@ static void __endio_write_update_ordered(struct inode *inode,
 	u64 ordered_offset = offset;
 	u64 ordered_bytes = bytes;
 	u64 last_offset;
-	int ret;
 
 	if (btrfs_is_free_space_inode(BTRFS_I(inode))) {
 		wq = fs_info->endio_freespace_worker;
@@ -8169,32 +8168,31 @@ static void __endio_write_update_ordered(struct inode *inode,
 		func = btrfs_endio_write_helper;
 	}
 
-again:
-	last_offset = ordered_offset;
-	ret = btrfs_dec_test_first_ordered_pending(inode, &ordered,
-						   &ordered_offset,
-						   ordered_bytes,
-						   uptodate);
-	if (!ret)
-		goto out_test;
-
-	btrfs_init_work(&ordered->work, func, finish_ordered_fn, NULL, NULL);
-	btrfs_queue_work(wq, &ordered->work);
-out_test:
-	/*
-	 * If btrfs_dec_test_ordered_pending does not find any ordered extent
-	 * in the range, we can exit.
-	 */
-	if (ordered_offset == last_offset)
-		return;
-	/*
-	 * our bio might span multiple ordered extents.  If we haven't
-	 * completed the accounting for the whole dio, go back and try again
-	 */
-	if (ordered_offset < offset + bytes) {
-		ordered_bytes = offset + bytes - ordered_offset;
-		ordered = NULL;
-		goto again;
+	while (ordered_offset < offset + bytes) {
+		last_offset = ordered_offset;
+		if (btrfs_dec_test_first_ordered_pending(inode, &ordered,
+							   &ordered_offset,
+							   ordered_bytes,
+							   uptodate)) {
+			btrfs_init_work(&ordered->work, func,
+					finish_ordered_fn,
+					NULL, NULL);
+			btrfs_queue_work(wq, &ordered->work);
+		}
+		/*
+		 * If btrfs_dec_test_ordered_pending does not find any ordered
+		 * extent in the range, we can exit.
+		 */
+		if (ordered_offset == last_offset)
+			return;
+		/*
+		 * Our bio might span multiple ordered extents. In this case
+		 * we keep goin until we have accounted the whole dio.
+		 */
+		if (ordered_offset < offset + bytes) {
+			ordered_bytes = offset + bytes - ordered_offset;
+			ordered = NULL;
+		}
 	}
 }
 
-- 
2.7.4

[PATCH 2/4] btrfs: Fix lock release order

[Nikolay Borisov]

Locks should generally be released in the oppposite order they are
acquired. Generally lock acquisiton ordering is used to ensure
deadlocks don't happen. However, as becomes more complicated it's
best to also maintain proper unlock order so as to avoid possible dead
locks. This was found by code inspection and doesn't necessarily lead
to a deadlock scenario.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3ae6394f9265..5e0c987e8fa8 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2609,8 +2609,8 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
 	delayed_refs->num_heads--;
 	rb_erase(&head->href_node, &delayed_refs->href_root);
 	RB_CLEAR_NODE(&head->href_node);
-	spin_unlock(&delayed_refs->lock);
 	spin_unlock(&head->lock);
+	spin_unlock(&delayed_refs->lock);
 	atomic_dec(&delayed_refs->num_entries);
 
 	trace_run_delayed_ref_head(fs_info, head, 0);
-- 
2.7.4

[PATCH 3/4] btrfs: Consolidate error checking for btrfs_alloc_chunk

[Nikolay Borisov]

The second if is really a subcase of ret being less than 0. So
introduce a generic if (ret < 0) check, and inside have another if
which explicitly handles the -ENOSPC and any other errors. No
functional changes.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 5e0c987e8fa8..facc5248d500 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4681,12 +4681,14 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
 	trans->allocating_chunk = false;
 
 	spin_lock(&space_info->lock);
-	if (ret < 0 && ret != -ENOSPC)
-		goto out;
-	if (ret)
-		space_info->full = 1;
-	else
+	if (ret < 0) {
+		if (ret == -ENOSPC)
+			space_info->full = 1;
+		else
+			goto out;
+	} else {
 		ret = 1;
+	}
 
 	space_info->force_alloc = CHUNK_ALLOC_NO_FORCE;
 out:
-- 
2.7.4

[PATCH 4/4] btrfs: Rewrite retry logic in do_chunk_alloc

[Nikolay Borisov]

do_chunk_alloc implements logic to detect whether there is currently
pending chunk allocation  (by means of space_info->chunk_alloc being
set) and if so it loops around to the 'again' label. Additionally,
based on the state of the space_info (e.g. whether it's full or not)
and the return value of should_alloc_chunk() it decides whether this
is a "hard" error (ENOSPC) or we can just return 0.

This patch refactors all of this:

1. Put order to the scattered ifs handling the various cases in an
easy-to-read if {} else if{} branches. This makes clear the various
cases we are interested in handling.

2. Call should_alloc_chunk only once and use the result in the
if/else if constructs. All of this is done under space_info->lock, so
even before multiple calls of should_alloc_chunk were unnecessary.

3. Rewrite the "do {} while()" loop currently implemented via label
into an explicit loop construct.

4. Move the mutex locking outside of the while loop and remove the
comment. The comment in fact was misleading since in the old code if the
mutex is acquired and we don't need to loop again (wait_for_alloc) is
0 then we don't recheck if we need to allocate (the comment was stating
the opposite).

5. Switch local vars to bool type where pertinent.

All in all this shouldn't introduce any functional changes.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index facc5248d500..9a05e5b5089f 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4601,7 +4601,8 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
 			  struct btrfs_fs_info *fs_info, u64 flags, int force)
 {
 	struct btrfs_space_info *space_info;
-	int wait_for_alloc = 0;
+	bool wait_for_alloc = false;
+	bool should_alloc = false;
 	int ret = 0;
 
 	/* Don't re-enter if we're already allocating a chunk */
@@ -4611,45 +4612,35 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
 	space_info = __find_space_info(fs_info, flags);
 	ASSERT(space_info);
 
-again:
-	spin_lock(&space_info->lock);
-	if (force < space_info->force_alloc)
-		force = space_info->force_alloc;
-	if (space_info->full) {
-		if (should_alloc_chunk(fs_info, space_info, force))
-			ret = -ENOSPC;
-		else
-			ret = 0;
-		spin_unlock(&space_info->lock);
-		return ret;
-	}
-
-	if (!should_alloc_chunk(fs_info, space_info, force)) {
+	do {
+		spin_lock(&space_info->lock);
+		if (force < space_info->force_alloc)
+			force = space_info->force_alloc;
+		should_alloc = should_alloc_chunk(fs_info, space_info, force);
+		if (space_info->full) {
+			/* No more free physical space */
+			if (should_alloc)
+				ret = -ENOSPC;
+			else
+				ret = 0;
+			spin_unlock(&space_info->lock);
+			return ret;
+		} else if (!should_alloc) {
+			spin_unlock(&space_info->lock);
+			return 0;
+		} else if (space_info->chunk_alloc) {
+			/* Someone is already allocating, we need to loop */
+			wait_for_alloc = true;
+		} else {
+			/* Proceed with allocation */
+			space_info->chunk_alloc = 1;
+			wait_for_alloc = false;
+		}
 		spin_unlock(&space_info->lock);
-		return 0;
-	} else if (space_info->chunk_alloc) {
-		wait_for_alloc = 1;
-	} else {
-		space_info->chunk_alloc = 1;
-	}
-
-	spin_unlock(&space_info->lock);
-
-	mutex_lock(&fs_info->chunk_mutex);
-
-	/*
-	 * The chunk_mutex is held throughout the entirety of a chunk
-	 * allocation, so once we've acquired the chunk_mutex we know that the
-	 * other guy is done and we need to recheck and see if we should
-	 * allocate.
-	 */
-	if (wait_for_alloc) {
-		mutex_unlock(&fs_info->chunk_mutex);
-		wait_for_alloc = 0;
 		cond_resched();
-		goto again;
-	}
+	} while (wait_for_alloc);
 
+	mutex_lock(&fs_info->chunk_mutex);
 	trans->allocating_chunk = true;
 
 	/*
-- 
2.7.4

Re: [PATCH 1/4] btrfs: Use while loop instead of labels in __endio_write_update_ordered

[David Sterba]

On Wed, Apr 11, 2018 at 11:21:17AM +0300, Nikolay Borisov wrote:
> Currently __endio_write_update_ordered uses labels to implement
> what is essentially a simple while loop. This makes the code more
> cumbersome to follow than it actually has to be. No functional
> changes. No xfstest regressions were found during testing.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 2/4] btrfs: Fix lock release order

[David Sterba]

On Wed, Apr 11, 2018 at 11:21:18AM +0300, Nikolay Borisov wrote:
> Locks should generally be released in the oppposite order they are
> acquired. Generally lock acquisiton ordering is used to ensure
> deadlocks don't happen. However, as becomes more complicated it's
> best to also maintain proper unlock order so as to avoid possible dead
> locks. This was found by code inspection and doesn't necessarily lead
> to a deadlock scenario.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 3/4] btrfs: Consolidate error checking for btrfs_alloc_chunk

[David Sterba]

On Wed, Apr 11, 2018 at 11:21:19AM +0300, Nikolay Borisov wrote:
> The second if is really a subcase of ret being less than 0. So
> introduce a generic if (ret < 0) check, and inside have another if
> which explicitly handles the -ENOSPC and any other errors. No
> functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 4/4] btrfs: Rewrite retry logic in do_chunk_alloc

[David Sterba]

On Wed, Apr 11, 2018 at 11:21:20AM +0300, Nikolay Borisov wrote:
> do_chunk_alloc implements logic to detect whether there is currently
> pending chunk allocation  (by means of space_info->chunk_alloc being
> set) and if so it loops around to the 'again' label. Additionally,
> based on the state of the space_info (e.g. whether it's full or not)
> and the return value of should_alloc_chunk() it decides whether this
> is a "hard" error (ENOSPC) or we can just return 0.
> 
> This patch refactors all of this:
> 
> 1. Put order to the scattered ifs handling the various cases in an
> easy-to-read if {} else if{} branches. This makes clear the various
> cases we are interested in handling.
> 
> 2. Call should_alloc_chunk only once and use the result in the
> if/else if constructs. All of this is done under space_info->lock, so
> even before multiple calls of should_alloc_chunk were unnecessary.
> 
> 3. Rewrite the "do {} while()" loop currently implemented via label
> into an explicit loop construct.
> 
> 4. Move the mutex locking outside of the while loop and remove the
> comment. The comment in fact was misleading since in the old code if the
> mutex is acquired and we don't need to loop again (wait_for_alloc) is
> 0 then we don't recheck if we need to allocate (the comment was stating
> the opposite).

The comment seems correct to me. It is referring to the actual
allocation that happens in paralell (a few lines below calling
btrfs_alloc_chunk). So the first part attempts to determine if we should
wait, and if yes, then take the mutex and block until the other
allocation finishes.

This is completely mising in the new code so the loop is only blocking
on the space info spinlock, but not going to sleep. Busy looping.

Re: [PATCH 4/4] btrfs: Rewrite retry logic in do_chunk_alloc

[Nikolay Borisov]

On 16.04.2018 21:53, David Sterba wrote:
> On Wed, Apr 11, 2018 at 11:21:20AM +0300, Nikolay Borisov wrote:
>> do_chunk_alloc implements logic to detect whether there is currently
>> pending chunk allocation  (by means of space_info->chunk_alloc being
>> set) and if so it loops around to the 'again' label. Additionally,
>> based on the state of the space_info (e.g. whether it's full or not)
>> and the return value of should_alloc_chunk() it decides whether this
>> is a "hard" error (ENOSPC) or we can just return 0.
>>
>> This patch refactors all of this:
>>
>> 1. Put order to the scattered ifs handling the various cases in an
>> easy-to-read if {} else if{} branches. This makes clear the various
>> cases we are interested in handling.
>>
>> 2. Call should_alloc_chunk only once and use the result in the
>> if/else if constructs. All of this is done under space_info->lock, so
>> even before multiple calls of should_alloc_chunk were unnecessary.
>>
>> 3. Rewrite the "do {} while()" loop currently implemented via label
>> into an explicit loop construct.
>>
>> 4. Move the mutex locking outside of the while loop and remove the
>> comment. The comment in fact was misleading since in the old code if the
>> mutex is acquired and we don't need to loop again (wait_for_alloc) is
>> 0 then we don't recheck if we need to allocate (the comment was stating
>> the opposite).
> 
> The comment seems correct to me. It is referring to the actual
> allocation that happens in paralell (a few lines below calling
> btrfs_alloc_chunk). So the first part attempts to determine if we should
> wait, and if yes, then take the mutex and block until the other
> allocation finishes.
> 
> This is completely mising in the new code so the loop is only blocking
> on the space info spinlock, but not going to sleep. Busy looping.

With your explanation the code now makes a bit more sense and indeed
I've missed that. Will send an updated patch.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v2] btrfs: Rewrite retry logic in do_chunk_alloc

[Nikolay Borisov]

do_chunk_alloc implements logic to detect whether there is currently
pending chunk allocation  (by means of space_info->chunk_alloc being
set) and if so it loops around to the 'again' label. Additionally,
based on the state of the space_info (e.g. whether it's full or not)
and the return value of should_alloc_chunk() it decides whether this
is a "hard" error (ENOSPC) or we can just return 0.

This patch refactors all of this:

1. Put order to the scattered ifs handling the various cases in an
easy-to-read if {} else if{} branches. This makes clear the various
cases we are interested in handling.

2. Call should_alloc_chunk only once and use the result in the
if/else if constructs. All of this is done under space_info->lock, so
even before multiple calls of should_alloc_chunk were unnecessary.

3. Rewrite the "do {} while()" loop currently implemented via label
into an explicit loop construct.

4. Move the mutex locking for the case where the caller is the one doing the 
allocation. For the case where the caller needs to wait a concurrent allocation, 
introduce a pair of mutex_lock/mutex_unlock to act as a barrier and reword the 
comment. 

5. Switch local vars to bool type where pertinent.

All in all this shouldn't introduce any functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

v2: 
 * Introduce missing logic in the case where we have to loop. The correct 
 behavior when a concurrent allocation is executing is to acquire/release the 
 mutex and loop to check if it makes sense to continue with our allocation try. 

 fs/btrfs/extent-tree.c | 73 +++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 37 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 7bfc03494c39..bfb19bcdeee3 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4601,7 +4601,8 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
 			  struct btrfs_fs_info *fs_info, u64 flags, int force)
 {
 	struct btrfs_space_info *space_info;
-	int wait_for_alloc = 0;
+	bool wait_for_alloc = false;
+	bool should_alloc = false;
 	int ret = 0;
 
 	/* Don't re-enter if we're already allocating a chunk */
@@ -4611,45 +4612,43 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
 	space_info = __find_space_info(fs_info, flags);
 	ASSERT(space_info);
 
-again:
-	spin_lock(&space_info->lock);
-	if (force < space_info->force_alloc)
-		force = space_info->force_alloc;
-	if (space_info->full) {
-		if (should_alloc_chunk(fs_info, space_info, force))
-			ret = -ENOSPC;
-		else
-			ret = 0;
-		spin_unlock(&space_info->lock);
-		return ret;
-	}
-
-	if (!should_alloc_chunk(fs_info, space_info, force)) {
-		spin_unlock(&space_info->lock);
-		return 0;
-	} else if (space_info->chunk_alloc) {
-		wait_for_alloc = 1;
-	} else {
-		space_info->chunk_alloc = 1;
-	}
-
-	spin_unlock(&space_info->lock);
-
-	mutex_lock(&fs_info->chunk_mutex);
+	do {
+		spin_lock(&space_info->lock);
+		if (force < space_info->force_alloc)
+			force = space_info->force_alloc;
+		should_alloc = should_alloc_chunk(fs_info, space_info, force);
+		if (space_info->full) {
+			/* No more free physical space */
+			if (should_alloc)
+				ret = -ENOSPC;
+			else
+				ret = 0;
+			spin_unlock(&space_info->lock);
+			return ret;
+		} else if (!should_alloc) {
+			spin_unlock(&space_info->lock);
+			return 0;
+		} else if (space_info->chunk_alloc) {
+			/* Someone is already allocating, so we need to block
+			 * while this someone is finished and then loop, to
+			 * recheck if we should continue with our allocation
+			 * try
+			 */
+			wait_for_alloc = true;
+			spin_unlock(&space_info->lock);
+			mutex_lock(&fs_info->chunk_mutex);
+			mutex_unlock(&fs_info->chunk_mutex);
+		} else {
+			/* Proceed with allocation */
+			space_info->chunk_alloc = 1;
+			wait_for_alloc = false;
+			spin_unlock(&space_info->lock);
+		}
 
-	/*
-	 * The chunk_mutex is held throughout the entirety of a chunk
-	 * allocation, so once we've acquired the chunk_mutex we know that the
-	 * other guy is done and we need to recheck and see if we should
-	 * allocate.
-	 */
-	if (wait_for_alloc) {
-		mutex_unlock(&fs_info->chunk_mutex);
-		wait_for_alloc = 0;
 		cond_resched();
-		goto again;
-	}
+	} while (wait_for_alloc);
 
+	mutex_lock(&fs_info->chunk_mutex);
 	trans->allocating_chunk = true;
 
 	/*
-- 
2.7.4

Re: [PATCH v2] btrfs: Rewrite retry logic in do_chunk_alloc

[Nikolay Borisov]

On 18.04.2018 10:27, Nikolay Borisov wrote:
> do_chunk_alloc implements logic to detect whether there is currently
> pending chunk allocation  (by means of space_info->chunk_alloc being
> set) and if so it loops around to the 'again' label. Additionally,
> based on the state of the space_info (e.g. whether it's full or not)
> and the return value of should_alloc_chunk() it decides whether this
> is a "hard" error (ENOSPC) or we can just return 0.
> 
> This patch refactors all of this:
> 
> 1. Put order to the scattered ifs handling the various cases in an
> easy-to-read if {} else if{} branches. This makes clear the various
> cases we are interested in handling.
> 
> 2. Call should_alloc_chunk only once and use the result in the
> if/else if constructs. All of this is done under space_info->lock, so
> even before multiple calls of should_alloc_chunk were unnecessary.
> 
> 3. Rewrite the "do {} while()" loop currently implemented via label
> into an explicit loop construct.
> 
> 4. Move the mutex locking for the case where the caller is the one doing the 
> allocation. For the case where the caller needs to wait a concurrent allocation, 
> introduce a pair of mutex_lock/mutex_unlock to act as a barrier and reword the 
> comment. 
> 
> 5. Switch local vars to bool type where pertinent.
> 
> All in all this shouldn't introduce any functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> v2: 
>  * Introduce missing logic in the case where we have to loop. The correct 
>  behavior when a concurrent allocation is executing is to acquire/release the 
>  mutex and loop to check if it makes sense to continue with our allocation try. 
> 
>  fs/btrfs/extent-tree.c | 73 +++++++++++++++++++++++++-------------------------
>  1 file changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 7bfc03494c39..bfb19bcdeee3 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4601,7 +4601,8 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
>  			  struct btrfs_fs_info *fs_info, u64 flags, int force)
>  {
>  	struct btrfs_space_info *space_info;
> -	int wait_for_alloc = 0;
> +	bool wait_for_alloc = false;
> +	bool should_alloc = false;
>  	int ret = 0;
>  
>  	/* Don't re-enter if we're already allocating a chunk */
> @@ -4611,45 +4612,43 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
>  	space_info = __find_space_info(fs_info, flags);
>  	ASSERT(space_info);
>  
> -again:
> -	spin_lock(&space_info->lock);
> -	if (force < space_info->force_alloc)
> -		force = space_info->force_alloc;
> -	if (space_info->full) {
> -		if (should_alloc_chunk(fs_info, space_info, force))
> -			ret = -ENOSPC;
> -		else
> -			ret = 0;
> -		spin_unlock(&space_info->lock);
> -		return ret;
> -	}
> -
> -	if (!should_alloc_chunk(fs_info, space_info, force)) {
> -		spin_unlock(&space_info->lock);
> -		return 0;
> -	} else if (space_info->chunk_alloc) {
> -		wait_for_alloc = 1;
> -	} else {
> -		space_info->chunk_alloc = 1;
> -	}
> -
> -	spin_unlock(&space_info->lock);
> -
> -	mutex_lock(&fs_info->chunk_mutex);
> +	do {
> +		spin_lock(&space_info->lock);
> +		if (force < space_info->force_alloc)
> +			force = space_info->force_alloc;
> +		should_alloc = should_alloc_chunk(fs_info, space_info, force);
> +		if (space_info->full) {
> +			/* No more free physical space */
> +			if (should_alloc)
> +				ret = -ENOSPC;
> +			else
> +				ret = 0;
> +			spin_unlock(&space_info->lock);
> +			return ret;
> +		} else if (!should_alloc) {
> +			spin_unlock(&space_info->lock);
> +			return 0;
> +		} else if (space_info->chunk_alloc) {
> +			/* Someone is already allocating, so we need to block
> +			 * while this someone is finished and then loop, to
> +			 * recheck if we should continue with our allocation
> +			 * try
> +			 */
> +			wait_for_alloc = true;
> +			spin_unlock(&space_info->lock);
> +			mutex_lock(&fs_info->chunk_mutex);
> +			mutex_unlock(&fs_info->chunk_mutex);
> +		} else {
> +			/* Proceed with allocation */
> +			space_info->chunk_alloc = 1;
> +			wait_for_alloc = false;
> +			spin_unlock(&space_info->lock);
> +		}
>  
> -	/*
> -	 * The chunk_mutex is held throughout the entirety of a chunk
> -	 * allocation, so once we've acquired the chunk_mutex we know that the
> -	 * other guy is done and we need to recheck and see if we should
> -	 * allocate.
> -	 */
> -	if (wait_for_alloc) {
> -		mutex_unlock(&fs_info->chunk_mutex);
> -		wait_for_alloc = 0;
>  		cond_resched();

I can't help but wonder do we really need this cond_resched given that
if we have to loop we are going to do the blocking sleep in case of a
concurrent allocation. In my testing on a vm with a single core not
having the resched did cause softlockups but now I'm wondering why,
since we should have in any case blocked on acquiring the mutex... hm

> -		goto again;
> -	}
> +	} while (wait_for_alloc);
>  
> +	mutex_lock(&fs_info->chunk_mutex);
>  	trans->allocating_chunk = true;
>  
>  	/*
> 

Re: [PATCH v2] btrfs: Rewrite retry logic in do_chunk_alloc

[Nikolay Borisov]

On 18.04.2018 10:27, Nikolay Borisov wrote:
> do_chunk_alloc implements logic to detect whether there is currently
> pending chunk allocation  (by means of space_info->chunk_alloc being
> set) and if so it loops around to the 'again' label. Additionally,
> based on the state of the space_info (e.g. whether it's full or not)
> and the return value of should_alloc_chunk() it decides whether this
> is a "hard" error (ENOSPC) or we can just return 0.
> 
> This patch refactors all of this:
> 
> 1. Put order to the scattered ifs handling the various cases in an
> easy-to-read if {} else if{} branches. This makes clear the various
> cases we are interested in handling.
> 
> 2. Call should_alloc_chunk only once and use the result in the
> if/else if constructs. All of this is done under space_info->lock, so
> even before multiple calls of should_alloc_chunk were unnecessary.
> 
> 3. Rewrite the "do {} while()" loop currently implemented via label
> into an explicit loop construct.
> 
> 4. Move the mutex locking for the case where the caller is the one doing the 
> allocation. For the case where the caller needs to wait a concurrent allocation, 
> introduce a pair of mutex_lock/mutex_unlock to act as a barrier and reword the 
> comment. 
> 
> 5. Switch local vars to bool type where pertinent.
> 
> All in all this shouldn't introduce any functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> v2: 
>  * Introduce missing logic in the case where we have to loop. The correct 
>  behavior when a concurrent allocation is executing is to acquire/release the 
>  mutex and loop to check if it makes sense to continue with our allocation try. 
> 

Ping

>  fs/btrfs/extent-tree.c | 73 +++++++++++++++++++++++++-------------------------
>  1 file changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 7bfc03494c39..bfb19bcdeee3 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4601,7 +4601,8 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
>  			  struct btrfs_fs_info *fs_info, u64 flags, int force)
>  {
>  	struct btrfs_space_info *space_info;
> -	int wait_for_alloc = 0;
> +	bool wait_for_alloc = false;
> +	bool should_alloc = false;
>  	int ret = 0;
>  
>  	/* Don't re-enter if we're already allocating a chunk */
> @@ -4611,45 +4612,43 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
>  	space_info = __find_space_info(fs_info, flags);
>  	ASSERT(space_info);
>  
> -again:
> -	spin_lock(&space_info->lock);
> -	if (force < space_info->force_alloc)
> -		force = space_info->force_alloc;
> -	if (space_info->full) {
> -		if (should_alloc_chunk(fs_info, space_info, force))
> -			ret = -ENOSPC;
> -		else
> -			ret = 0;
> -		spin_unlock(&space_info->lock);
> -		return ret;
> -	}
> -
> -	if (!should_alloc_chunk(fs_info, space_info, force)) {
> -		spin_unlock(&space_info->lock);
> -		return 0;
> -	} else if (space_info->chunk_alloc) {
> -		wait_for_alloc = 1;
> -	} else {
> -		space_info->chunk_alloc = 1;
> -	}
> -
> -	spin_unlock(&space_info->lock);
> -
> -	mutex_lock(&fs_info->chunk_mutex);
> +	do {
> +		spin_lock(&space_info->lock);
> +		if (force < space_info->force_alloc)
> +			force = space_info->force_alloc;
> +		should_alloc = should_alloc_chunk(fs_info, space_info, force);
> +		if (space_info->full) {
> +			/* No more free physical space */
> +			if (should_alloc)
> +				ret = -ENOSPC;
> +			else
> +				ret = 0;
> +			spin_unlock(&space_info->lock);
> +			return ret;
> +		} else if (!should_alloc) {
> +			spin_unlock(&space_info->lock);
> +			return 0;
> +		} else if (space_info->chunk_alloc) {
> +			/* Someone is already allocating, so we need to block
> +			 * while this someone is finished and then loop, to
> +			 * recheck if we should continue with our allocation
> +			 * try
> +			 */
> +			wait_for_alloc = true;
> +			spin_unlock(&space_info->lock);
> +			mutex_lock(&fs_info->chunk_mutex);
> +			mutex_unlock(&fs_info->chunk_mutex);
> +		} else {
> +			/* Proceed with allocation */
> +			space_info->chunk_alloc = 1;
> +			wait_for_alloc = false;
> +			spin_unlock(&space_info->lock);
> +		}
>  
> -	/*
> -	 * The chunk_mutex is held throughout the entirety of a chunk
> -	 * allocation, so once we've acquired the chunk_mutex we know that the
> -	 * other guy is done and we need to recheck and see if we should
> -	 * allocate.
> -	 */
> -	if (wait_for_alloc) {
> -		mutex_unlock(&fs_info->chunk_mutex);
> -		wait_for_alloc = 0;
>  		cond_resched();
> -		goto again;
> -	}
> +	} while (wait_for_alloc);
>  
> +	mutex_lock(&fs_info->chunk_mutex);
>  	trans->allocating_chunk = true;
>  
>  	/*
> 

Re: [PATCH v2] btrfs: Rewrite retry logic in do_chunk_alloc

[David Sterba]

On Wed, Apr 18, 2018 at 10:27:57AM +0300, Nikolay Borisov wrote:
> do_chunk_alloc implements logic to detect whether there is currently
> pending chunk allocation  (by means of space_info->chunk_alloc being
> set) and if so it loops around to the 'again' label. Additionally,
> based on the state of the space_info (e.g. whether it's full or not)
> and the return value of should_alloc_chunk() it decides whether this
> is a "hard" error (ENOSPC) or we can just return 0.
> 
> This patch refactors all of this:
> 
> 1. Put order to the scattered ifs handling the various cases in an
> easy-to-read if {} else if{} branches. This makes clear the various
> cases we are interested in handling.
> 
> 2. Call should_alloc_chunk only once and use the result in the
> if/else if constructs. All of this is done under space_info->lock, so
> even before multiple calls of should_alloc_chunk were unnecessary.
> 
> 3. Rewrite the "do {} while()" loop currently implemented via label
> into an explicit loop construct.
> 
> 4. Move the mutex locking for the case where the caller is the one doing the 
> allocation. For the case where the caller needs to wait a concurrent allocation, 
> introduce a pair of mutex_lock/mutex_unlock to act as a barrier and reword the 
> comment. 
> 
> 5. Switch local vars to bool type where pertinent.
> 
> All in all this shouldn't introduce any functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>