Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: dsterba@suse.cz, Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v4 5/7] btrfs: qgroup: Introduce per-root swapped blocks infrastructure
Date: Wed, 23 Jan 2019 07:07:50 +0800
Message-ID: <507b96ec-86b1-6c23-2d16-b6b67b2f5920@gmx.com> (raw)
In-Reply-To: <20190122165554.GO2900@twin.jikos.cz>

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

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-15  8:15 [PATCH v4 0/7] btrfs: qgroup: Delay subtree scan to reduce overhead Qu Wenruo
2019-01-15  8:15 ` [PATCH v4 1/7] btrfs: qgroup: Move reserved data account from btrfs_delayed_ref_head to btrfs_qgroup_extent_record Qu Wenruo
2019-01-15  8:15 ` [PATCH v4 2/7] btrfs: qgroup: Don't trigger backref walk at delayed ref insert time Qu Wenruo
2019-01-15  8:16 ` [PATCH v4 3/7] btrfs: relocation: Delay reloc tree deletion after merge_reloc_roots() Qu Wenruo
2019-01-22 16:32   ` David Sterba
2019-01-23  6:01     ` Qu Wenruo
2019-01-15  8:16 ` [PATCH v4 4/7] btrfs: qgroup: Refactor btrfs_qgroup_trace_subtree_swap() Qu Wenruo
2019-01-22 16:38   ` David Sterba
2019-01-15  8:16 ` [PATCH v4 5/7] btrfs: qgroup: Introduce per-root swapped blocks infrastructure Qu Wenruo
2019-01-22 16:55   ` David Sterba
2019-01-22 23:07     ` Qu Wenruo [this message]
2019-01-24 18:44       ` David Sterba
2019-01-15  8:16 ` [PATCH v4 6/7] btrfs: qgroup: Use delayed subtree rescan for balance Qu Wenruo
2019-01-22 17:05   ` David Sterba
2019-01-15  8:16 ` [PATCH v4 7/7] btrfs: qgroup: Cleanup old subtree swap code Qu Wenruo
2019-01-15 17:26 ` [PATCH v4 0/7] btrfs: qgroup: Delay subtree scan to reduce overhead Josef Bacik
2019-01-16  0:31   ` Qu Wenruo
2019-01-16  1:07     ` Qu Wenruo
2019-01-16  1:15       ` Josef Bacik
2019-01-16  1:29         ` Qu Wenruo
2019-01-16  1:34           ` Josef Bacik
2019-01-16  1:36             ` Qu Wenruo
2019-01-22 16:28 ` David Sterba
2019-01-23  5:43   ` Qu Wenruo

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=507b96ec-86b1-6c23-2d16-b6b67b2f5920@gmx.com \
    --to=quwenruo.btrfs@gmx.com \
    --cc=dsterba@suse.cz \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=wqu@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox