| Message ID | 20160930011537.4857-1-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 09/29/2016 08:15 PM, Qu Wenruo wrote: > While the reason why qgroup reserved space may underflow is still under > investigation, such WARN_ON will help us to expose the bug more easily, > and for end-user we can detect and avoid underflow. > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> Looks good. Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > --- > fs/btrfs/qgroup.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index 8db2e29..8532587 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -1061,8 +1061,12 @@ 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)) > + qgroup->reserved = 0; > + else > + qgroup->reserved -= num_bytes; > + } > > qgroup_dirty(fs_info, qgroup); > > @@ -1082,8 +1086,12 @@ 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)) > + qgroup->reserved = 0; > + else > + qgroup->reserved -= num_bytes; > + } > qgroup->excl_cmpr += sign * num_bytes; > qgroup_dirty(fs_info, qgroup); > > @@ -2201,7 +2209,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)) > + qgroup->reserved = 0; > + else > + qgroup->reserved -= num_bytes; > > list_for_each_entry(glist, &qg->groups, next_group) { > ret = ulist_add(fs_info->qgroup_ulist, >
On Fri, Sep 30, 2016 at 09:15:36AM +0800, Qu Wenruo wrote: > While the reason why qgroup reserved space may underflow is still under > investigation, such WARN_ON will help us to expose the bug more easily, > and for end-user we can detect and avoid underflow. > > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > --- > fs/btrfs/qgroup.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index 8db2e29..8532587 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -1061,8 +1061,12 @@ 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)) That's only partially helpful, you should also print the numbers, ref_root and/or qgroup id. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 10/19/2016 10:31 PM, David Sterba wrote: > On Fri, Sep 30, 2016 at 09:15:36AM +0800, Qu Wenruo wrote: >> While the reason why qgroup reserved space may underflow is still under >> investigation, such WARN_ON will help us to expose the bug more easily, >> and for end-user we can detect and avoid underflow. >> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >> --- >> fs/btrfs/qgroup.c | 21 ++++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >> index 8db2e29..8532587 100644 >> --- a/fs/btrfs/qgroup.c >> +++ b/fs/btrfs/qgroup.c >> @@ -1061,8 +1061,12 @@ 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)) > > That's only partially helpful, you should also print the numbers, > ref_root and/or qgroup id. > I'm OK to add more output. But normally it's not really help though. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 20, 2016 at 09:05:34AM +0800, Qu Wenruo wrote: > > > At 10/19/2016 10:31 PM, David Sterba wrote: > > On Fri, Sep 30, 2016 at 09:15:36AM +0800, Qu Wenruo wrote: > >> While the reason why qgroup reserved space may underflow is still under > >> investigation, such WARN_ON will help us to expose the bug more easily, > >> and for end-user we can detect and avoid underflow. > >> > >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > >> --- > >> fs/btrfs/qgroup.c | 21 ++++++++++++++++----- > >> 1 file changed, 16 insertions(+), 5 deletions(-) > >> > >> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > >> index 8db2e29..8532587 100644 > >> --- a/fs/btrfs/qgroup.c > >> +++ b/fs/btrfs/qgroup.c > >> @@ -1061,8 +1061,12 @@ 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)) > > > > That's only partially helpful, you should also print the numbers, > > ref_root and/or qgroup id. > > > > I'm OK to add more output. > But normally it's not really help though. So do you need the warning at all? The next patch adds tracepoints, I assume you turn them on and look for the issues you're debugging. The warning would otherwise appear for any user. Without the message it's IME more scary, it's not obvious what happened without looking into the code. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 10/20/2016 05:20 PM, David Sterba wrote: > On Thu, Oct 20, 2016 at 09:05:34AM +0800, Qu Wenruo wrote: >> >> >> At 10/19/2016 10:31 PM, David Sterba wrote: >>> On Fri, Sep 30, 2016 at 09:15:36AM +0800, Qu Wenruo wrote: >>>> While the reason why qgroup reserved space may underflow is still under >>>> investigation, such WARN_ON will help us to expose the bug more easily, >>>> and for end-user we can detect and avoid underflow. >>>> >>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >>>> --- >>>> fs/btrfs/qgroup.c | 21 ++++++++++++++++----- >>>> 1 file changed, 16 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >>>> index 8db2e29..8532587 100644 >>>> --- a/fs/btrfs/qgroup.c >>>> +++ b/fs/btrfs/qgroup.c >>>> @@ -1061,8 +1061,12 @@ 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)) >>> >>> That's only partially helpful, you should also print the numbers, >>> ref_root and/or qgroup id. >>> >> >> I'm OK to add more output. >> But normally it's not really help though. > > So do you need the warning at all? The next patch adds tracepoints, I > assume you turn them on and look for the issues you're debugging. The > warning would otherwise appear for any user. Without the message it's > IME more scary, it's not obvious what happened without looking into the > code. > The WARN_ON() is needed to let end-user know there is something wrong and encourage them to report the bug. And, other than trace events, normally I just check if there is any warning in kernel dmesg to do a quick check if the fix is OK or not. (trace events is never simpler than plain dmesg) So personally speaking, WARN_ON() is always needed and obvious enough for both developer and end-user. But that's just my personal preference. More data on fsid and qgroupid after scary kernel WARN_ON() won't hurt anyway. Especially fsid/qgroupid is useful for multi-btrfs/subvolume system. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 8db2e29..8532587 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -1061,8 +1061,12 @@ 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)) + qgroup->reserved = 0; + else + qgroup->reserved -= num_bytes; + } qgroup_dirty(fs_info, qgroup); @@ -1082,8 +1086,12 @@ 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)) + qgroup->reserved = 0; + else + qgroup->reserved -= num_bytes; + } qgroup->excl_cmpr += sign * num_bytes; qgroup_dirty(fs_info, qgroup); @@ -2201,7 +2209,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)) + qgroup->reserved = 0; + else + qgroup->reserved -= num_bytes; list_for_each_entry(glist, &qg->groups, next_group) { ret = ulist_add(fs_info->qgroup_ulist,
While the reason why qgroup reserved space may underflow is still under investigation, such WARN_ON will help us to expose the bug more easily, and for end-user we can detect and avoid underflow. Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- fs/btrfs/qgroup.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)