| Message ID | 20161128014009.12179-1-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Nov 28, 2016 at 09:40:07AM +0800, Qu Wenruo wrote: > 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> One of the warnings is visible during xfstests (btrfs_qgroup_free_refroot), is there a fix? Either a patch in mailinglist or work in progress. If not, I'm a bit reluctant to add it to 4.10 as we'd get that reported from users for sure. -- 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 11/30/2016 12:10 AM, David Sterba wrote: > On Mon, Nov 28, 2016 at 09:40:07AM +0800, Qu Wenruo wrote: >> 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> > > One of the warnings is visible during xfstests > (btrfs_qgroup_free_refroot), is there a fix? Either a patch in > mailinglist or work in progress. If not, I'm a bit reluctant to add it > to 4.10 as we'd get that reported from users for sure. Fix WIP, ETA would be in this week. At least, this warning is working and helped us to find bugs. 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 Wed, Nov 30, 2016 at 08:24:32AM +0800, Qu Wenruo wrote: > > > At 11/30/2016 12:10 AM, David Sterba wrote: > > On Mon, Nov 28, 2016 at 09:40:07AM +0800, Qu Wenruo wrote: > >> 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> > > > > One of the warnings is visible during xfstests > > (btrfs_qgroup_free_refroot), is there a fix? Either a patch in > > mailinglist or work in progress. If not, I'm a bit reluctant to add it > > to 4.10 as we'd get that reported from users for sure. > > Fix WIP, ETA would be in this week. Good, thanks. > At least, this warning is working and helped us to find bugs. No doubt about that, I'll keep the patch in 4.10 queue but will not add it to the first pull that I'm about to send today. -- 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 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,