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 <nbori...@suse.com>
> ---
>
> 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: >
signature.asc
Description: OpenPGP digital signature
