Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: 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: Thu, 24 Jan 2019 19:44:58 +0100
Message-ID: <20190124184457.GZ2900@twin.jikos.cz> (raw)
In-Reply-To: <507b96ec-86b1-6c23-2d16-b6b67b2f5920@gmx.com>

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.

  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
2019-01-24 18:44       ` David Sterba [this message]
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=20190124184457.GZ2900@twin.jikos.cz \
    --to=dsterba@suse.cz \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=quwenruo.btrfs@gmx.com \
    --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