On Tue, Jan 30, 2018 at 04:47:54PM +0200, Nikolay Borisov wrote:
> 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.
Well, I almost overlooked the mutex on first top-down reading the code.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
