* [PATCH v3 1/3] btrfs: Add WARN_ON for qgroup reserved underflow @ 2016-11-08 2:20 Qu Wenruo 2016-11-08 2:20 ` [PATCH v3 2/3] btrfs: Add trace point for qgroup reserved space Qu Wenruo 2016-11-08 2:20 ` [PATCH v3 3/3] btrfs: qgroup: Re-arrange tracepoint timing to co-operate with reserved space tracepoint Qu Wenruo 0 siblings, 2 replies; 5+ messages in thread From: Qu Wenruo @ 2016-11-08 2:20 UTC (permalink / raw) To: linux-btrfs, dsterba Goldwyn Rodrigues has exposed and fixed a bug which underflows btrfs qgroup reserved space, and leads to non-writable fs. This reminds us that we don't have enough underflow check for qgroup reserved space. For underflow case, we should not really underflow the numbers but warn and keeps qgroup still work. So add more check on qgroup reserved space and add WARN_ON() and btrfs_warn() for any underflow case. Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> Reviewed-by: David Sterba <dsterba@suse.com> --- v2: Add btrfs_warn() output for fsid, qgroupid, original reserved space and num_bytes to reduce, for end-user to locate the subvolume causing the problem. Suggested by David. v3: None --- fs/btrfs/qgroup.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 11f4fff..fc0c64e 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -1031,6 +1031,15 @@ static void qgroup_dirty(struct btrfs_fs_info *fs_info, list_add(&qgroup->dirty, &fs_info->dirty_qgroups); } +static void report_reserved_underflow(struct btrfs_fs_info *fs_info, + struct btrfs_qgroup *qgroup, + u64 num_bytes) +{ + btrfs_warn(fs_info, + "qgroup %llu reserved space underflow, have: %llu, to free: %llu", + qgroup->qgroupid, qgroup->reserved, num_bytes); + qgroup->reserved = 0; +} /* * The easy accounting, if we are adding/removing the only ref for an extent * then this qgroup and all of the parent qgroups get their reference and @@ -1058,8 +1067,13 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, WARN_ON(sign < 0 && qgroup->excl < num_bytes); qgroup->excl += sign * num_bytes; qgroup->excl_cmpr += sign * num_bytes; - if (sign > 0) - qgroup->reserved -= num_bytes; + if (sign > 0) { + if (WARN_ON(qgroup->reserved < num_bytes)) + report_reserved_underflow(fs_info, qgroup, + num_bytes); + else + qgroup->reserved -= num_bytes; + } qgroup_dirty(fs_info, qgroup); @@ -1079,8 +1093,13 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, qgroup->rfer_cmpr += sign * num_bytes; WARN_ON(sign < 0 && qgroup->excl < num_bytes); qgroup->excl += sign * num_bytes; - if (sign > 0) - qgroup->reserved -= num_bytes; + if (sign > 0) { + if (WARN_ON(qgroup->reserved < num_bytes)) + report_reserved_underflow(fs_info, qgroup, + num_bytes); + else + qgroup->reserved -= num_bytes; + } qgroup->excl_cmpr += sign * num_bytes; qgroup_dirty(fs_info, qgroup); @@ -2204,7 +2223,10 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info, qg = u64_to_ptr(unode->aux); - qg->reserved -= num_bytes; + if (WARN_ON(qgroup->reserved < num_bytes)) + report_reserved_underflow(fs_info, qgroup, num_bytes); + else + qgroup->reserved -= num_bytes; list_for_each_entry(glist, &qg->groups, next_group) { ret = ulist_add(fs_info->qgroup_ulist, -- 2.10.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 2/3] btrfs: Add trace point for qgroup reserved space 2016-11-08 2:20 [PATCH v3 1/3] btrfs: Add WARN_ON for qgroup reserved underflow Qu Wenruo @ 2016-11-08 2:20 ` Qu Wenruo 2016-11-08 11:27 ` Sanidhya Solanki 2016-11-08 2:20 ` [PATCH v3 3/3] btrfs: qgroup: Re-arrange tracepoint timing to co-operate with reserved space tracepoint Qu Wenruo 1 sibling, 1 reply; 5+ messages in thread From: Qu Wenruo @ 2016-11-08 2:20 UTC (permalink / raw) To: linux-btrfs, dsterba Introduce the following trace points: qgroup_update_reserve qgroup_meta_reserve These trace points are handy to trace qgroup reserve space related problems. Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- Although I hope these trace points will not be used again. v2: None v3: Separate from trace point timing modification patch. --- fs/btrfs/qgroup.c | 15 +++++++++++++++ include/trace/events/btrfs.h | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index fc0c64e..c00f962 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -1068,6 +1068,8 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, qgroup->excl += sign * num_bytes; qgroup->excl_cmpr += sign * num_bytes; if (sign > 0) { + trace_qgroup_update_reserve(fs_info, qgroup->qgroupid, + qgroup->reserved, (s64)-num_bytes); if (WARN_ON(qgroup->reserved < num_bytes)) report_reserved_underflow(fs_info, qgroup, num_bytes); @@ -1094,6 +1096,9 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, WARN_ON(sign < 0 && qgroup->excl < num_bytes); qgroup->excl += sign * num_bytes; if (sign > 0) { + trace_qgroup_update_reserve(fs_info, qgroup->qgroupid, + qgroup->reserved, + (s64)-num_bytes); if (WARN_ON(qgroup->reserved < num_bytes)) report_reserved_underflow(fs_info, qgroup, num_bytes); @@ -2178,6 +2183,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes) qg = u64_to_ptr(unode->aux); + trace_qgroup_update_reserve(fs_info, qg->qgroupid, + qg->reserved, num_bytes); qg->reserved += num_bytes; } @@ -2223,6 +2230,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info, qg = u64_to_ptr(unode->aux); + trace_qgroup_update_reserve(fs_info, qg->qgroupid, + qg->reserved, (s64)-num_bytes); if (WARN_ON(qgroup->reserved < num_bytes)) report_reserved_underflow(fs_info, qgroup, num_bytes); else @@ -2706,6 +2715,8 @@ int btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes) return 0; BUG_ON(num_bytes != round_down(num_bytes, root->nodesize)); + trace_qgroup_meta_reserve(root->fs_info, root->objectid, + (s64)num_bytes); ret = qgroup_reserve(root, num_bytes); if (ret < 0) return ret; @@ -2724,6 +2735,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root) reserved = atomic_xchg(&root->qgroup_meta_rsv, 0); if (reserved == 0) return; + trace_qgroup_meta_reserve(root->fs_info, root->objectid, + (s64)-reserved); qgroup_free(root, reserved); } @@ -2736,6 +2749,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes) BUG_ON(num_bytes != round_down(num_bytes, root->nodesize)); WARN_ON(atomic_read(&root->qgroup_meta_rsv) < num_bytes); atomic_sub(num_bytes, &root->qgroup_meta_rsv); + trace_qgroup_meta_reserve(root->fs_info, root->objectid, + (s64)-num_bytes); qgroup_free(root, num_bytes); } diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h index e030d6f..fb3cb6c 100644 --- a/include/trace/events/btrfs.h +++ b/include/trace/events/btrfs.h @@ -1468,6 +1468,49 @@ TRACE_EVENT(qgroup_update_counters, __entry->cur_new_count) ); +TRACE_EVENT(qgroup_update_reserve, + + TP_PROTO(struct btrfs_fs_info *fs_info, u64 qgid, u64 cur_reserved, + s64 diff), + + TP_ARGS(fs_info, qgid, cur_reserved, diff), + + TP_STRUCT__entry_btrfs( + __field( u64, qgid ) + __field( u64, cur_reserved ) + __field( s64, diff ) + ), + + TP_fast_assign_btrfs(fs_info, + __entry->qgid = qgid; + __entry->cur_reserved = cur_reserved; + __entry->diff = diff; + ), + + TP_printk_btrfs("qgid = %llu, cur_reserved = %llu, diff = %lld", + __entry->qgid, __entry->cur_reserved, __entry->diff) +); + +TRACE_EVENT(qgroup_meta_reserve, + + TP_PROTO(struct btrfs_fs_info *fs_info, u64 refroot, s64 diff), + + TP_ARGS(fs_info, refroot, diff), + + TP_STRUCT__entry_btrfs( + __field( u64, refroot ) + __field( u64, diff ) + ), + + TP_fast_assign_btrfs(fs_info, + __entry->refroot = refroot; + __entry->diff = diff; + ), + + TP_printk_btrfs("refroot = %llu, diff = %lld", + __entry->refroot, __entry->diff) +); + #endif /* _TRACE_BTRFS_H */ /* This part must be outside protection */ -- 2.10.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 2/3] btrfs: Add trace point for qgroup reserved space 2016-11-08 2:20 ` [PATCH v3 2/3] btrfs: Add trace point for qgroup reserved space Qu Wenruo @ 2016-11-08 11:27 ` Sanidhya Solanki 2016-11-25 13:04 ` David Sterba 0 siblings, 1 reply; 5+ messages in thread From: Sanidhya Solanki @ 2016-11-08 11:27 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs, dsterba On Tue, 8 Nov 2016 10:20:43 +0800 Qu Wenruo <quwenruo@cn.fujitsu.com> wrote: > Introduce the following trace points: > qgroup_update_reserve > qgroup_meta_reserve > > These trace points are handy to trace qgroup reserve space related > problems. > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > --- > Although I hope these trace points will not be used again. > > v2: > None > v3: > Separate from trace point timing modification patch. > --- > fs/btrfs/qgroup.c | 15 +++++++++++++++ > include/trace/events/btrfs.h | 43 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 58 insertions(+) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index fc0c64e..c00f962 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -1068,6 +1068,8 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, > qgroup->excl += sign * num_bytes; > qgroup->excl_cmpr += sign * num_bytes; > if (sign > 0) { > + trace_qgroup_update_reserve(fs_info, qgroup->qgroupid, > + qgroup->reserved, (s64)-num_bytes); > if (WARN_ON(qgroup->reserved < num_bytes)) > report_reserved_underflow(fs_info, qgroup, > num_bytes); > @@ -1094,6 +1096,9 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, > WARN_ON(sign < 0 && qgroup->excl < num_bytes); > qgroup->excl += sign * num_bytes; > if (sign > 0) { > + trace_qgroup_update_reserve(fs_info, qgroup->qgroupid, > + qgroup->reserved, > + (s64)-num_bytes); > if (WARN_ON(qgroup->reserved < num_bytes)) > report_reserved_underflow(fs_info, qgroup, > num_bytes); > @@ -2178,6 +2183,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes) > > qg = u64_to_ptr(unode->aux); > > + trace_qgroup_update_reserve(fs_info, qg->qgroupid, > + qg->reserved, num_bytes); > qg->reserved += num_bytes; > } > > @@ -2223,6 +2230,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info, > > qg = u64_to_ptr(unode->aux); > > + trace_qgroup_update_reserve(fs_info, qg->qgroupid, > + qg->reserved, (s64)-num_bytes); > if (WARN_ON(qgroup->reserved < num_bytes)) > report_reserved_underflow(fs_info, qgroup, num_bytes); > else > @@ -2706,6 +2715,8 @@ int btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes) > return 0; > > BUG_ON(num_bytes != round_down(num_bytes, root->nodesize)); > + trace_qgroup_meta_reserve(root->fs_info, root->objectid, > + (s64)num_bytes); > ret = qgroup_reserve(root, num_bytes); > if (ret < 0) > return ret; > @@ -2724,6 +2735,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root) > reserved = atomic_xchg(&root->qgroup_meta_rsv, 0); > if (reserved == 0) > return; > + trace_qgroup_meta_reserve(root->fs_info, root->objectid, > + (s64)-reserved); > qgroup_free(root, reserved); > } > > @@ -2736,6 +2749,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes) > BUG_ON(num_bytes != round_down(num_bytes, root->nodesize)); > WARN_ON(atomic_read(&root->qgroup_meta_rsv) < num_bytes); > atomic_sub(num_bytes, &root->qgroup_meta_rsv); > + trace_qgroup_meta_reserve(root->fs_info, root->objectid, > + (s64)-num_bytes); > qgroup_free(root, num_bytes); > } Why is there a hyphen in some places after "(s64)" and not in others? Is that supposed to be type casting or something else? Sanidhya ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 2/3] btrfs: Add trace point for qgroup reserved space 2016-11-08 11:27 ` Sanidhya Solanki @ 2016-11-25 13:04 ` David Sterba 0 siblings, 0 replies; 5+ messages in thread From: David Sterba @ 2016-11-25 13:04 UTC (permalink / raw) To: Sanidhya Solanki; +Cc: Qu Wenruo, linux-btrfs, dsterba On Tue, Nov 08, 2016 at 06:27:12AM -0500, Sanidhya Solanki wrote: > On Tue, 8 Nov 2016 10:20:43 +0800 > Qu Wenruo <quwenruo@cn.fujitsu.com> wrote: > > > Introduce the following trace points: > > qgroup_update_reserve > > qgroup_meta_reserve > > > > These trace points are handy to trace qgroup reserve space related > > problems. > > > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > > --- > > Although I hope these trace points will not be used again. > > > > v2: > > None > > v3: > > Separate from trace point timing modification patch. > > --- > > fs/btrfs/qgroup.c | 15 +++++++++++++++ > > include/trace/events/btrfs.h | 43 +++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 58 insertions(+) > > > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > > index fc0c64e..c00f962 100644 > > --- a/fs/btrfs/qgroup.c > > +++ b/fs/btrfs/qgroup.c > > @@ -1068,6 +1068,8 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, > > qgroup->excl += sign * num_bytes; > > qgroup->excl_cmpr += sign * num_bytes; > > if (sign > 0) { > > + trace_qgroup_update_reserve(fs_info, qgroup->qgroupid, > > + qgroup->reserved, (s64)-num_bytes); > > if (WARN_ON(qgroup->reserved < num_bytes)) > > report_reserved_underflow(fs_info, qgroup, > > num_bytes); > > @@ -1094,6 +1096,9 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, > > WARN_ON(sign < 0 && qgroup->excl < num_bytes); > > qgroup->excl += sign * num_bytes; > > if (sign > 0) { > > + trace_qgroup_update_reserve(fs_info, qgroup->qgroupid, > > + qgroup->reserved, > > + (s64)-num_bytes); > > if (WARN_ON(qgroup->reserved < num_bytes)) > > report_reserved_underflow(fs_info, qgroup, > > num_bytes); > > @@ -2178,6 +2183,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes) > > > > qg = u64_to_ptr(unode->aux); > > > > + trace_qgroup_update_reserve(fs_info, qg->qgroupid, > > + qg->reserved, num_bytes); > > qg->reserved += num_bytes; > > } > > > > @@ -2223,6 +2230,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info, > > > > qg = u64_to_ptr(unode->aux); > > > > + trace_qgroup_update_reserve(fs_info, qg->qgroupid, > > + qg->reserved, (s64)-num_bytes); > > if (WARN_ON(qgroup->reserved < num_bytes)) > > report_reserved_underflow(fs_info, qgroup, num_bytes); > > else > > @@ -2706,6 +2715,8 @@ int btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes) > > return 0; > > > > BUG_ON(num_bytes != round_down(num_bytes, root->nodesize)); > > + trace_qgroup_meta_reserve(root->fs_info, root->objectid, > > + (s64)num_bytes); > > ret = qgroup_reserve(root, num_bytes); > > if (ret < 0) > > return ret; > > @@ -2724,6 +2735,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root) > > reserved = atomic_xchg(&root->qgroup_meta_rsv, 0); > > if (reserved == 0) > > return; > > + trace_qgroup_meta_reserve(root->fs_info, root->objectid, > > + (s64)-reserved); > > qgroup_free(root, reserved); > > } > > > > @@ -2736,6 +2749,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes) > > BUG_ON(num_bytes != round_down(num_bytes, root->nodesize)); > > WARN_ON(atomic_read(&root->qgroup_meta_rsv) < num_bytes); > > atomic_sub(num_bytes, &root->qgroup_meta_rsv); > > + trace_qgroup_meta_reserve(root->fs_info, root->objectid, > > + (s64)-num_bytes); > > qgroup_free(root, num_bytes); > > } > > Why is there a hyphen in some places after "(s64)" and not in others? Is that supposed to be > type casting or something else? Right, this looks strange and not clear from the context, besides that the typecast should precede negation: -(s64)num_bytes There's also a type mismatch in the 2nd tracepoint, where argument is s64, the tracepoint entry is u64 and printf format is %lld. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 3/3] btrfs: qgroup: Re-arrange tracepoint timing to co-operate with reserved space tracepoint 2016-11-08 2:20 [PATCH v3 1/3] btrfs: Add WARN_ON for qgroup reserved underflow Qu Wenruo 2016-11-08 2:20 ` [PATCH v3 2/3] btrfs: Add trace point for qgroup reserved space Qu Wenruo @ 2016-11-08 2:20 ` Qu Wenruo 1 sibling, 0 replies; 5+ messages in thread From: Qu Wenruo @ 2016-11-08 2:20 UTC (permalink / raw) To: linux-btrfs, dsterba Newly introduced qgroup reserved space trace points are normally nested into several common qgroup operations. While some other trace points are not well placed to co-operate with them, causing confusing output. This patch re-arrange trace_btrfs_qgroup_release_data() and trace_btrfs_qgroup_free_delayed_ref() trace points so they are triggered before reserved space ones. Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- v3: Separated from new trace points patch. --- fs/btrfs/qgroup.c | 6 +++--- fs/btrfs/qgroup.h | 6 +----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index c00f962..aad34314 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -2660,12 +2660,12 @@ static int __btrfs_qgroup_release_data(struct inode *inode, u64 start, u64 len, if (ret < 0) goto out; - if (free) { - qgroup_free(BTRFS_I(inode)->root, changeset.bytes_changed); + if (free) trace_op = QGROUP_FREE; - } trace_btrfs_qgroup_release_data(inode, start, len, changeset.bytes_changed, trace_op); + if (free) + qgroup_free(BTRFS_I(inode)->root, changeset.bytes_changed); out: ulist_free(changeset.range_changed); return ret; diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h index 1bc64c8..93b222b 100644 --- a/fs/btrfs/qgroup.h +++ b/fs/btrfs/qgroup.h @@ -107,15 +107,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, struct btrfs_qgroup_inherit *inherit); void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info, u64 ref_root, u64 num_bytes); -/* - * TODO: Add proper trace point for it, as btrfs_qgroup_free() is - * called by everywhere, can't provide good trace for delayed ref case. - */ static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info, u64 ref_root, u64 num_bytes) { - btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes); trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes); + btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes); } void assert_qgroups_uptodate(struct btrfs_trans_handle *trans); -- 2.10.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-25 13:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-08 2:20 [PATCH v3 1/3] btrfs: Add WARN_ON for qgroup reserved underflow Qu Wenruo 2016-11-08 2:20 ` [PATCH v3 2/3] btrfs: Add trace point for qgroup reserved space Qu Wenruo 2016-11-08 11:27 ` Sanidhya Solanki 2016-11-25 13:04 ` David Sterba 2016-11-08 2:20 ` [PATCH v3 3/3] btrfs: qgroup: Re-arrange tracepoint timing to co-operate with reserved space tracepoint Qu Wenruo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).