Re: [PATCH 5/5] btrfs: qgroup: warn about inconsistent qgroups when relation update fails

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

在 2024/6/21 06:58, Boris Burkov 写道:
> On Wed, Jun 19, 2024 at 07:09:20PM +0200, David Sterba wrote:
>> Calling btrfs_handle_fs_error() after btrfs_run_qgroups() fails to
>> update the qgroup status is probably not necessary, this would turn the
>> filesystem to read-only. For the same reason aborting the transaction is
>> also not a good option.
>>
>> The state is left inconsistent and can be fixed by rescan, printing a
>> warning should be sufficient. Return code reflects the status of
>> adding/deleting the relation and if the transaction was ended properly.
>
> A few thoughts/questions about this:
>
> 1. Why do we even need to btrfs_run_qgroups() here? Won't we btrfs_run_qgroups
> in the transaction that actually commits the qgroup relation items? I'm
> guessing some qgroup lookup sees the not-committed items in a way users
> care about? Are we expecting such high-scale `qgroup assign` that we
> can't just commit this txn and make it simpler?

I agree with this.

I'm originally thinking some weird situation that one have called
btrfs_run_qgroups(), then we do relation update without
btrfs_run_qgroups(), in this case it may cause problems.

But since btrfs_run_qgroups() is really only called in committing
transaction and this is the only other location, I do not think it's
going to cause much problem.

So overall we should just remove the btrfs_run_qgroups() call and no
need to handle the error (even no need to commit tranasaction).

Thanks,
Qu

>
> 2. Is this really failing in cases where adding the relation items
> succeeds and then it all gets committed successfully? i.e., do you have
> a reproducer for this case?
>
> 3. If this is a realistic scenario, I'm worried about the squotas case,
> which doesn't have any rescan capability to fallback on. However, I
> don't see why my (1) above doesn't just save us anyway. If we commit the
> relation item, then we also commit the status/info items. And the txn
> commit run_qgroups is not allowed to fail.
>
> 4. Let's say that 1. is not strictly essential, and the txn commit is
> going to fix us. In that case, is it really accurate to say we are
> inconsistent any more than just during a transaction before we run
> qgroups? I suppose compared to the case where this succeeded, we are. I
> just have a weird feeling we are stretching the meaning of inconsistent.
> Though not in an egregious way or anything, as we are reporting a
> non-fatal error?
>
> Sorry for the slightly rambling review..
>
> Thanks,
> Boris
>
>>
>> Signed-off-by: David Sterba <dsterba@suse.com>
>> ---
>>   fs/btrfs/ioctl.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 31c4aea8b93a..f893a6b711c6 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3877,8 +3877,9 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>>   	err = btrfs_run_qgroups(trans);
>>   	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>>   	if (err < 0)
>> -		btrfs_handle_fs_error(fs_info, err,
>> -				      "failed to update qgroup status and info");
>> +		btrfs_warn(fs_info,
>> +			   "qgroup status update failed after %s relation, marked as inconsistent",
>> +			   sa->assign ? "adding" : "deleting");
>>   	err = btrfs_end_transaction(trans);
>>   	if (err && !ret)
>>   		ret = err;
>> --
>> 2.45.0
>>
>