| Message ID | 1461638989-5048-1-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Apr 26, 2016 at 10:49:49AM +0800, Qu Wenruo wrote: > In the new add_extent_rec_nolookup() function, we add bytes_used to > update found bytes accounting. > > However there is a typo that we used tmpl->nr, which should be rec->nr. > This will make us to add 1 for data backref, instead the correct size. I'm not able to trace that back to the original code, the value template is just filled from the parameters and I don't see where bytes_used would get set from the rec->nr value. > --- a/cmds-check.c > +++ b/cmds-check.c > @@ -4550,7 +4550,7 @@ static int add_extent_rec_nolookup(struct cache_tree *extent_cache, > rec->cache.size = tmpl->nr; > ret = insert_cache_extent(extent_cache, &rec->cache); > BUG_ON(ret); > - bytes_used += tmpl->nr; > + bytes_used += rec->nr; ie. this would be just 'nr' before the refactoring. -- 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
David Sterba wrote on 2016/04/26 12:06 +0200: > On Tue, Apr 26, 2016 at 10:49:49AM +0800, Qu Wenruo wrote: >> In the new add_extent_rec_nolookup() function, we add bytes_used to >> update found bytes accounting. >> >> However there is a typo that we used tmpl->nr, which should be rec->nr. >> This will make us to add 1 for data backref, instead the correct size. > > I'm not able to trace that back to the original code, the value template > is just filled from the parameters and I don't see where bytes_used > would get set from the rec->nr value. Then it would be a very old bug. When iteration extent tree, when btrfsck found a data backref, it will fill tmpl->nr with 1 and max_size with real size. And if following that code, we increase bytes_used by 1, not max_size. This makes the final "Found XXX bytes" unaligned to sectorsize. While not much people will check such output, until we introduce the new low-memory mode, and compare the output, finding the difference. If checked manually (by adding all metadata and extent size from debug-tree output), one would find the result is always smaller than expected. > >> --- a/cmds-check.c >> +++ b/cmds-check.c >> @@ -4550,7 +4550,7 @@ static int add_extent_rec_nolookup(struct cache_tree *extent_cache, >> rec->cache.size = tmpl->nr; >> ret = insert_cache_extent(extent_cache, &rec->cache); >> BUG_ON(ret); >> - bytes_used += tmpl->nr; >> + bytes_used += rec->nr; > > ie. this would be just 'nr' before the refactoring. > > Yes, I also checked v4.2 code, it is indeed "nr", so it's a long standing bug. 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 Tue, Apr 26, 2016 at 10:49:49AM +0800, Qu Wenruo wrote: > In the new add_extent_rec_nolookup() function, we add bytes_used to > update found bytes accounting. > > However there is a typo that we used tmpl->nr, which should be rec->nr. > This will make us to add 1 for data backref, instead the correct size. > > Reported-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> Applied, thanks. -- 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/cmds-check.c b/cmds-check.c index d59968b..b207f8e 100644 --- a/cmds-check.c +++ b/cmds-check.c @@ -4550,7 +4550,7 @@ static int add_extent_rec_nolookup(struct cache_tree *extent_cache, rec->cache.size = tmpl->nr; ret = insert_cache_extent(extent_cache, &rec->cache); BUG_ON(ret); - bytes_used += tmpl->nr; + bytes_used += rec->nr; if (tmpl->metadata) rec->crossing_stripes = check_crossing_stripes(rec->start,
In the new add_extent_rec_nolookup() function, we add bytes_used to update found bytes accounting. However there is a typo that we used tmpl->nr, which should be rec->nr. This will make us to add 1 for data backref, instead the correct size. Reported-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- cmds-check.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)