Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: Josef Bacik <josef@toxicpanda.com>
To: David Sterba <dsterba@suse.cz>
Cc: Josef Bacik <josef@toxicpanda.com>,
	linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 0/8][V2] Enospc cleanups and fixeS
Date: Thu, 13 Dec 2018 13:28:52 -0500
Message-ID: <20181213182850.qyyaegqzc5qijodx@MacBook-Pro-91.local> (raw)
In-Reply-To: <20181213181741.GJ23615@twin.jikos.cz>

On Thu, Dec 13, 2018 at 07:17:41PM +0100, David Sterba wrote:
> On Thu, Dec 13, 2018 at 09:45:55AM -0500, Josef Bacik wrote:
> > On Thu, Dec 13, 2018 at 03:11:11PM +0100, David Sterba wrote:
> > > On Mon, Dec 03, 2018 at 10:24:51AM -0500, Josef Bacik wrote:
> > > > v1->v2:
> > > > - addressed comments from reviewers.
> > > > - fixed a bug in patch 6 that was introduced because of changes to upstream.
> > > > 
> > > > -- Original message --
> > > > 
> > > > The delayed refs rsv patches exposed a bunch of issues in our enospc
> > > > infrastructure that needed to be addressed.  These aren't really one coherent
> > > > group, but they are all around flushing and reservations.
> > > > may_commit_transaction() needed to be updated a little bit, and we needed to add
> > > > a new state to force chunk allocation if things got dicey.  Also because we can
> > > > end up needed to reserve a whole bunch of extra space for outstanding delayed
> > > > refs we needed to add the ability to only ENOSPC tickets that were too big to
> > > > satisfy, instead of failing all of the tickets.  There's also a fix in here for
> > > > one of the corner cases where we didn't quite have enough space reserved for the
> > > > delayed refs we were generating during evict().  Thanks,
> > > 
> > > One testbox reports an assertion failure on current for-next,
> > > generic/224. I'm reporting it under this patchset as it's my best guess.
> > > Same host running misc-next (with the delayed rsv patchset) was fine and
> > > the run with for-next (including this patchset) fails. The assertion is
> > > 
> > >  5225 static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
> > >  5226                                     struct btrfs_space_info *space_info,
> > >  5227                                     u64 orig_bytes,
> > >  5228                                     enum btrfs_reserve_flush_enum flush,
> > >  5229                                     bool system_chunk)
> > >  5230 {
> > >  5231         struct reserve_ticket ticket;
> > >  5232         u64 used;
> > >  5233         u64 reclaim_bytes = 0;
> > >  5234         int ret = 0;
> > >  5235
> > >  5236         ASSERT(orig_bytes);
> > >  ^^^^
> > > 
> > 
> > Looking at your for-next branch on your github (I assume this is what you are
> > testing)
> > 
> > https://github.com/kdave/btrfs-devel/blob/for-next-20181212/fs/btrfs/extent-tree.c
> > 
> > at line 5860 there's supposed to be a 
> > 
> > if (num_bytes == 0)
> > 	return 0
> > 
> > that's what I changed in v2 of this patchset, as I hit this bug as well.  It
> 
> What does 'this' refer to? The patchset in this mail thread? If yes,
> then something's wrong, because in the patch
> 
> https://patchwork.kernel.org/patch/10709827/
> 
> there's a clear ASSERT(orig_bytes) in the context of one hunk:
> 
> @@ -5210,6 +5217,7 @@  static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
>  {
>  	struct reserve_ticket ticket;
>  	u64 used;
> +	u64 reclaim_bytes = 0;
>  	int ret = 0;
>  
>  	ASSERT(orig_bytes);
> ---
> 
> > looks like you still have v1 of this patchset applied.  Thanks,
> 
> I looked up the patch series on patchwork too to double check that I haven't
> missed it in my mailboxes but no.
> 
> The assert was introduced by "Btrfs: introduce ticketed enospc infrastructure"
> which is quite old. The v2 of that patch is
> 
> https://lore.kernel.org/linux-btrfs/1463506255-15918-1-git-send-email-jbacik@fb.com/
> 
> and also has the assert and not if (orig_bytes). Confused.

I mean I hit this ASSERT() in testing, and this patch series has the fixed patch

https://patchwork.kernel.org/patch/10709829/

this is what fixes the problem that is causing the ASSERT(), it appears your
next branch doesn't have this updated patch, but the previous one which trips
that ASSERT.  Thanks,

Josef

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-03 15:24 Josef Bacik
2018-12-03 15:24 ` [PATCH 1/8] btrfs: check if free bgs for commit Josef Bacik
2018-12-03 15:24 ` [PATCH 2/8] btrfs: dump block_rsv whe dumping space info Josef Bacik
2018-12-03 15:24 ` [PATCH 3/8] btrfs: don't use global rsv for chunk allocation Josef Bacik
2018-12-11  9:59   ` Nikolay Borisov
2018-12-03 15:24 ` [PATCH 4/8] btrfs: add ALLOC_CHUNK_FORCE to the flushing code Josef Bacik
2018-12-11 10:08   ` Nikolay Borisov
2018-12-11 16:47     ` David Sterba
2018-12-11 16:51       ` Nikolay Borisov
2018-12-11 19:04         ` David Sterba
2018-12-03 15:24 ` [PATCH 5/8] btrfs: don't enospc all tickets on flush failure Josef Bacik
2018-12-11 14:32   ` Nikolay Borisov
2018-12-03 15:24 ` [PATCH 6/8] btrfs: loop in inode_rsv_refill Josef Bacik
2018-12-12 16:01   ` Nikolay Borisov
2019-02-06 18:20     ` David Sterba
2019-01-30 16:41   ` David Sterba
2018-12-03 15:24 ` [PATCH 7/8] btrfs: be more explicit about allowed flush states Josef Bacik
2018-12-11 18:28   ` David Sterba
2018-12-12  8:40   ` Nikolay Borisov
2018-12-03 15:24 ` [PATCH 8/8] btrfs: reserve extra space during evict() Josef Bacik
2018-12-14  8:20   ` Nikolay Borisov
2018-12-13 14:11 ` [PATCH 0/8][V2] Enospc cleanups and fixeS David Sterba
2018-12-13 14:36   ` Nikolay Borisov
2018-12-13 14:45   ` Josef Bacik
2018-12-13 18:17     ` David Sterba
2018-12-13 18:28       ` Josef Bacik [this message]
2018-12-13 18:41         ` David Sterba
2019-02-08 16:08 ` David Sterba

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=20181213182850.qyyaegqzc5qijodx@MacBook-Pro-91.local \
    --to=josef@toxicpanda.com \
    --cc=dsterba@suse.cz \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    /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