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

[Boris Burkov <boris@bur.io>]

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?

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
>