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 <redacted>

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