Re: [PATCH v3 2/3] btrfs: qgroup: try to flush qgroup space when we get -EDQUOT

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2103 bytes --]



On 2020/7/10 上午12:32, David Sterba wrote:
> On Wed, Jul 08, 2020 at 02:24:46PM +0800, Qu Wenruo wrote:
>> -int btrfs_qgroup_reserve_data(struct btrfs_inode *inode,
>> +static int try_flush_qgroup(struct btrfs_root *root)
>> +{
>> +	struct btrfs_trans_handle *trans;
>> +	int ret;
>> +
>> +	/*
>> +	 * We don't want to run flush again and again, so if there is a running
>> +	 * one, we won't try to start a new flush, but exit directly.
>> +	 */
>> +	ret = mutex_trylock(&root->qgroup_flushing_mutex);
>> +	if (!ret) {
>> +		mutex_lock(&root->qgroup_flushing_mutex);
>> +		mutex_unlock(&root->qgroup_flushing_mutex);
> 
> This is abuse of mutex, for status tracking "is somebody flushing" and
> for waiting until it's over.
> 
> We have many root::status bits (the BTRFS_ROOT_* namespace) so that
> qgroups are flushing should another one. The bit atomically set when it
> starts and cleared when it ends.

In fact, I want to avoid plain wait_queue usage if possible.

Unlike mutex, wait_queue doesn't have good enough debug mechanism like
lockdep.

I see no reason re-implementing the existing mutex code by ourselves
could bring any benefit here.

It may looks like an abuse of mutex, but I could wrap it into something
like wait_or_lock_mutex(), which may slightly improve the readability.

Or am I missing anything?

> 
> All waiting tasks should queue in a normal wait_queue_head.
> 
>> +		return 0;
>> +	}
>> +
>> +	ret = btrfs_start_delalloc_snapshot(root);
>> +	if (ret < 0)
>> +		goto unlock;
>> +	btrfs_wait_ordered_extents(root, U64_MAX, 0, (u64)-1);
>> +
>> +	trans = btrfs_join_transaction(root);
>> +	if (IS_ERR(trans)) {
>> +		ret = PTR_ERR(trans);
>> +		goto unlock;
>> +	}
>> +
>> +	ret = btrfs_commit_transaction(trans);
>> +unlock:
>> +	mutex_unlock(&root->qgroup_flushing_mutex);
> 
> And also the whole wait/join/commit combo is in one huge mutex, that's
> really an anti-pattern.
> 
But that mutex is per-root, and is the slow path.

Converting to wait_queue won't change the pattern either.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]