On 2018年01月26日 22:13, Nikolay Borisov wrote:
> Running generic/019 with qgroups on the scratch device enabled is
> almost guaranteed to trigger the BUG_ON in btrfs_free_tree_block. It's
> supposed to trigger only on -ENOMEM, in reality, however, it's possible
> to get -EIO from btrfs_qgroup_trace_extent_post. This function just
> finds the roots of the extent being tracked and sets the qrecord->old_roots
> list. If this operation fails nothing critical happens except the
> quota accounting can be considered wrong. In such case just set the
> INCONSISTENT flag for the quota and be done with it.
>
> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
> ---
>
> Qu,
>
> This fixes the crash for me, however I'm not entirely sure it's really the
> best fix since it's leaking the usage of INCONSISTENT out of qgroup.c. Ideally
> I'd like to avoid this. Since you have more experience with qgroups and also you
> introduced the extent tracking code in add_delayed_tree_ref how does
> look to you?
>
>
> fs/btrfs/delayed-ref.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index a1a40cf382e3..1e9aa18cc0d8 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -820,8 +820,13 @@ int btrfs_add_delayed_tree_ref(struct btrfs_fs_info *fs_info,
> num_bytes, parent, ref_root, level, action);
> spin_unlock(&delayed_refs->lock);
>
> - if (qrecord_inserted)
> - return btrfs_qgroup_trace_extent_post(fs_info, record);
> + if (qrecord_inserted) {
> + int ret = btrfs_qgroup_trace_extent_post(fs_info, record);
Since generic/019 is using make_fail_request, it can cause some tree
reads fail.
And since btrfs_qgroup_trace_extent_post() cause btrfs_find_all_roots()
to fill @old_roots for new extents, it will definitely read tree blocks,
and when its request failed, it returns -EIO as expected.
So yes, the qgroup code break the old assumption that
btrfs_add_delayed_tree|data_ref() could only return -ENOMEM.
But compared to delayed-ref, qgroup is not a critical payload (if
delayed-ref is screwed up, extent tree and possible data extent will be
corrupted, while qgroup failure only affects qgroup), so it's OK to keep
the old assumption.
> + if (ret != -ENOMEM)
Here we don't really need to handle ENOMEM specially.
Just check ( ret < 0 ) should be enough.
> + fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
And extra btrfs_warn() would help.
> + else
> + return -ENOMEM;
Since btrfs is not a critical payload compared to delayed ref, returning
0 should not be a problem as we have already marked qgroup inconsistent.
(With comment explaining why it's OK to return 0)
Thanks,
Qu
> + }
> return 0;
>
> free_head_ref:
>
Attachment:
signature.asc
Description: OpenPGP digital signature
