On 30.01.2018 16:34, David Sterba wrote:

On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote:

quoted

@@ -6062,19 +6062,19 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes)
 	 * If we have a transaction open (can happen if we call truncate_block
 	 * from truncate), then we need FLUSH_LIMIT so we don't deadlock.
 	 */
+
 	if (btrfs_is_free_space_inode(inode)) {
 		flush = BTRFS_RESERVE_NO_FLUSH;
 		delalloc_lock = false;
-	} else if (current->journal_info) {
-		flush = BTRFS_RESERVE_FLUSH_LIMIT;
-	}
+	} else {
+		if (current->journal_info)
+			flush = BTRFS_RESERVE_FLUSH_LIMIT;
 
-	if (flush != BTRFS_RESERVE_NO_FLUSH &&
-	    btrfs_transaction_in_commit(fs_info))
-		schedule_timeout(1);
+		if (btrfs_transaction_in_commit(fs_info))
+			schedule_timeout(1);
 
-	if (delalloc_lock)
 		mutex_lock(&inode->delalloc_mutex);
+	}

Squeezing the condition branches makes the code more readable, I have
only one objection and it's the mutex_lock. It IMHO looks better when
it's a separate branch as it pairs with the unlock:

if (delalloc_lock)
	mutex_lock(...);

...

if (delalloc_lock)
	mutex_unlock(...);

In your version it's implied by the first if that checks
btrfs_is_free_space_inode and delalloc_lock is hidden there.

My line of thought when developing the patch was that delalloc is
another level of indirection and. What I wanted to achieve in the end is
to make it clear that delalloc_mutex really depends on whether we are
reserving for the freespace inode or not.

quoted

 
 	num_bytes = ALIGN(num_bytes, fs_info->sectorsize);