Re: [PATCH v4 5/7] btrfs: qgroup: Introduce per-root swapped blocks infrastructure

[David Sterba <dsterba@suse.cz>]

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.