On Wed, Jan 23, 2019 at 07:07:50AM +0800, Qu Wenruo wrote:
>
>
> On 2019/1/23 上午12:55, David Sterba wrote:
> > On Tue, Jan 15, 2019 at 04:16:02PM +0800, Qu Wenruo wrote:
> <snip>
> >> +
> >> + block = kmalloc(sizeof(*block), GFP_NOFS);
> >> + if (!block) {
> >> + ret = -ENOMEM;
> >
> > This goes to 'out' that marks quota as inconsistent, is this
> > intentional?
>
> Yes
>
> > Ie. if this function does not succed for whatever reason,
> > the quotas are indeed inconsistent.
>
> If we hit this ENOMEM case, although it's possible later CoW doesn't
> touch this subtree, it's also possible later CoW touches this and make
> qgroup numbers inconsistent.
>
> So we should mark qgroup inconsistent for the possibility of
> inconsistent qgroup numbers.
I see, thanks.
> [snip]
> >
> >> + while (*p) {
> >> + struct btrfs_qgroup_swapped_block *entry;
> >> +
> >> + parent = *p;
> >> + entry = rb_entry(parent, struct btrfs_qgroup_swapped_block,
> >> + node);
> >> +
> >> + if (entry->subv_bytenr < block->subv_bytenr)
> >> + p = &(*p)->rb_left;
> >> + else if (entry->subv_bytenr > block->subv_bytenr)
> >> + p = &(*p)->rb_right;
> >> + else {
> >
> > I've pointed out missing { } around if/else if/else statemtents in patch
> > 1, would be good if you fix all patches.
>
> Sorry, it's sometimes hard to apply the same comment to older branches.
> Normally I'll pay more attention for newly written code, but it's not
> that easy to spot older code.
>
> >
> >> + if (entry->subv_generation != block->subv_generation ||
> >> + entry->reloc_bytenr != block->reloc_bytenr ||
> >> + entry->reloc_generation !=
> >> + block->reloc_generation) {
> >> + WARN_ON_ONCE(1);
> >
> > This would need some explanation why you want to see the warning.
>
> The case here is, I don't want to populate the whole dmesg with tons of
> possible backtrace from this call sites, but I still hope user to report
> such error.
>
> So I choose _ONCE() to inform user to report the error, as I don't
> really see it's possible.
>
> > _ONCE
> > is for the whole lifetime of the module but I think you'd be interested
> > on a per-filesystem basis. If this is meant for developers it should go
> > under DEBUG, but otherwise I don't see what would this warning bring to
> > the users.
>
> OK, I'll put it under DEBUG and change from ONCE to regular WARN_ON().
Works for me.
> [snip]
> >>
> >> +/*
> >> + * Special performance hack for balance.
> >
> > Is it really a hack? Hack sounds cool, but isn't it just an optimization?
>
> It's optimization indeed.
> (But hack sounds really cool).
>
>
> >>
> >> +struct btrfs_qgroup_swapped_block {
> >> + struct rb_node node;
> >> +
> >> + bool trace_leaf;
> >> + int level;
> >
> > Please reorder that so int goes before bool, othewise this adds
> > unnecessary padding.
>
> Can we make compiler to do this work?
No, compiler must keep the order of definition and may add padding to
satify the alignment constraints. It can warn though, -Wpadded but it's
too noisy so you'd have to diff the warnings to see if your code adds
new paddings. For any new structure or new member I use the tool
'pahole' that prints the detailed layout.
