Re: [PATCH] btrfs: make btrfs_dirty_inode() to always reserve metadata space

From: Josef Bacik <josef@toxicpanda.com>
To: Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] btrfs: make btrfs_dirty_inode() to always reserve metadata space
Date: Thu, 18 Feb 2021 11:14:16 -0500	[thread overview]
Message-ID: <6bc8bef5-43a0-f6c6-9b43-2f62a3e4e051@toxicpanda.com> (raw)
In-Reply-To: <20210108053659.87728-1-wqu@suse.com>

On 1/8/21 12:36 AM, Qu Wenruo wrote:
> There are several qgroup flush related bugs fixed recently, all of them
> are caused by the fact that we can trigger qgroup metadata space
> reservation holding a transaction handle.
> 
> Thankfully the only situation to trigger above reservation is
> btrfs_dirty_inode().
> 
> Currently btrfs_dirty_inode() will try join transactio first, then
> update the inode.
> If btrfs_update_inode() fails with -ENOSPC, then it retry to start
> transaction to reserve metadata space.
> 
> This not only forces us to reserve metadata space with a transaction
> handle hold, but can't handle other errors like -EDQUOT.
> 
> This patch will make btrfs_dirty_inode() to call
> btrfs_start_transaction() directly without first try joining then
> starting, so that in try_flush_qgroup() we won't hold a trans handle.
> 
> This will slow down btrfs_dirty_inode() but my fstests doesn't show too
> much different for most test cases, thus it may be worthy to skip such
> performance "optimization".
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

I'm not interested in slowing down the !qgroups case just for qgroups.  We want 
to short circuit the start here because it has the potential to be _very_ 
expensive, when we may very well have space already allocated for the inode.

The best solution I can think of for this is to add a bool to indicate that we 
don't want to attempt to make reservations.  The only problem here is if the 
inode doesn't have space allocated for it, if it doesn't we need to fall back 
anyway.  The speed up comes from inodes that already have the delayed inode 
setup.  So simply tell it to error out if we're not already set up, and then we 
can fail back to btrfs_start_transaction().  That'll keep us in line with our 
performance for !qgroups and solve your qgroup related deadlock problems.  Thanks,

Josef