On 30.01.2018 16:34, David Sterba wrote: > On Fri, Jan 12, 2018 at 04:21:05PM +0200, Nikolay Borisov wrote: >> @@ -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. >> >> num_bytes = ALIGN(num_bytes, fs_info->sectorsize); -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html