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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2745 bytes --]



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.

[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().

[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?

Thanks,
Qu

[snip]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]