From: Tejun Heo <tj@kernel.org>
To: dsterba@suse.com, clm@fb.com, josef@toxicpanda.com,
axboe@kernel.dk, jack@suse.cz
Cc: linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, kernel-team@fb.com
Subject: [PATCH 6/8] Btrfs: only associate the locked page with one async_cow struct
Date: Thu, 13 Jun 2019 17:33:48 -0700 [thread overview]
Message-ID: <20190614003350.1178444-7-tj@kernel.org> (raw)
In-Reply-To: <20190614003350.1178444-1-tj@kernel.org>
From: Chris Mason <clm@fb.com>
The btrfs writepages function collects a large range of pages flagged
for delayed allocation, and then sends them down through the COW code
for processing. When compression is on, we allocate one async_cow
structure for every 512K, and then run those pages through the
compression code for IO submission.
writepages starts all of this off with a single page, locked by
the original call to extent_write_cache_pages(), and it's important to
keep track of this page because it has already been through
clear_page_dirty_for_io().
The btrfs async_cow struct has a pointer to the locked_page, and when
we're redirtying the page because compression had to fallback to
uncompressed IO, we use page->index to decide if a given async_cow
struct really owns that page.
But, this is racey. If a given delalloc range is broken up into two
async_cows (cow_A and cow_B), we can end up with something like this:
compress_file_range(cowA)
submit_compress_extents(cowA)
submit compressed bios(cowA)
put_page(locked_page)
compress_file_range(cowB)
...
The end result is that cowA is completed and cleaned up before cowB even
starts processing. This means we can free locked_page() and reuse it
elsewhere. If we get really lucky, it'll have the same page->index in
its new home as it did before.
While we're processing cowB, we might decide we need to fall back to
uncompressed IO, and so compress_file_range() will call
__set_page_dirty_nobufers() on cowB->locked_page.
Without cgroups in use, this creates as a phantom dirty page, which
isn't great but isn't the end of the world. With cgroups in use, we
might crash in the accounting code because page->mapping->i_wb isn't
set.
[ 8308.523110] BUG: unable to handle kernel NULL pointer dereference at 00000000000000d0
[ 8308.531084] IP: percpu_counter_add_batch+0x11/0x70
[ 8308.538371] PGD 66534e067 P4D 66534e067 PUD 66534f067 PMD 0
[ 8308.541750] Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 8308.551948] CPU: 16 PID: 2172 Comm: rm Not tainted
[ 8308.566883] RIP: 0010:percpu_counter_add_batch+0x11/0x70
[ 8308.567891] RSP: 0018:ffffc9000a97bbe0 EFLAGS: 00010286
[ 8308.568986] RAX: 0000000000000005 RBX: 0000000000000090 RCX: 0000000000026115
[ 8308.570734] RDX: 0000000000000030 RSI: ffffffffffffffff RDI: 0000000000000090
[ 8308.572543] RBP: 0000000000000000 R08: fffffffffffffff5 R09: 0000000000000000
[ 8308.573856] R10: 00000000000260c0 R11: ffff881037fc26c0 R12: ffffffffffffffff
[ 8308.580099] R13: ffff880fe4111548 R14: ffffc9000a97bc90 R15: 0000000000000001
[ 8308.582520] FS: 00007f5503ced480(0000) GS:ffff880ff7200000(0000) knlGS:0000000000000000
[ 8308.585440] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 8308.587951] CR2: 00000000000000d0 CR3: 00000001e0459005 CR4: 0000000000360ee0
[ 8308.590707] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 8308.592865] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 8308.594469] Call Trace:
[ 8308.595149] account_page_cleaned+0x15b/0x1f0
[ 8308.596340] __cancel_dirty_page+0x146/0x200
[ 8308.599395] truncate_cleanup_page+0x92/0xb0
[ 8308.600480] truncate_inode_pages_range+0x202/0x7d0
[ 8308.617392] btrfs_evict_inode+0x92/0x5a0
[ 8308.619108] evict+0xc1/0x190
[ 8308.620023] do_unlinkat+0x176/0x280
[ 8308.621202] do_syscall_64+0x63/0x1a0
[ 8308.623451] entry_SYSCALL_64_after_hwframe+0x42/0xb7
The fix here is to make asyc_cow->locked_page NULL everywhere but the
one async_cow struct that's allowed to do things to the locked page.
Signed-off-by: Chris Mason <clm@fb.com>
Fixes: 771ed689d2cd ("Btrfs: Optimize compressed writeback and reads")
---
fs/btrfs/extent_io.c | 2 +-
fs/btrfs/inode.c | 25 +++++++++++++++++++++----
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 13fca7bfc1f2..9f223d7d78c0 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1838,7 +1838,7 @@ static int __process_pages_contig(struct address_space *mapping,
if (page_ops & PAGE_SET_PRIVATE2)
SetPagePrivate2(pages[i]);
- if (pages[i] == locked_page) {
+ if (locked_page && pages[i] == locked_page) {
put_page(pages[i]);
pages_locked++;
continue;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 91b161fb1521..df5527cc07b9 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -666,10 +666,12 @@ static noinline void compress_file_range(struct async_chunk *async_chunk,
* to our extent and set things up for the async work queue to run
* cow_file_range to do the normal delalloc dance.
*/
- if (page_offset(async_chunk->locked_page) >= start &&
- page_offset(async_chunk->locked_page) <= end)
+ if (async_chunk->locked_page &&
+ (page_offset(async_chunk->locked_page) >= start &&
+ page_offset(async_chunk->locked_page)) <= end) {
__set_page_dirty_nobuffers(async_chunk->locked_page);
/* unlocked later on in the async handlers */
+ }
if (redirty)
extent_range_redirty_for_io(inode, start, end);
@@ -759,7 +761,7 @@ static noinline void submit_compressed_extents(struct async_chunk *async_chunk)
async_extent->start +
async_extent->ram_size - 1,
WB_SYNC_ALL);
- else if (ret)
+ else if (ret && async_chunk->locked_page)
unlock_page(async_chunk->locked_page);
kfree(async_extent);
cond_resched();
@@ -1236,10 +1238,25 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
async_chunk[i].inode = inode;
async_chunk[i].start = start;
async_chunk[i].end = cur_end;
- async_chunk[i].locked_page = locked_page;
async_chunk[i].write_flags = write_flags;
INIT_LIST_HEAD(&async_chunk[i].extents);
+ /*
+ * The locked_page comes all the way from writepage and its
+ * the original page we were actually given. As we spread
+ * this large delalloc region across multiple async_cow
+ * structs, only the first struct needs a pointer to locked_page
+ *
+ * This way we don't need racey decisions about who is supposed
+ * to unlock it.
+ */
+ if (locked_page) {
+ async_chunk[i].locked_page = locked_page;
+ locked_page = NULL;
+ } else {
+ async_chunk[i].locked_page = NULL;
+ }
+
btrfs_init_work(&async_chunk[i].work,
btrfs_delalloc_helper,
async_cow_start, async_cow_submit,
--
2.17.1
next prev parent reply other threads:[~2019-06-14 0:34 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-14 0:33 [PATCHSET btrfs/for-next] btrfs: fix cgroup writeback support Tejun Heo
2019-06-14 0:33 ` [PATCH 1/8] blkcg, writeback: Add wbc->no_wbc_acct Tejun Heo
2019-06-14 13:41 ` Josef Bacik
2019-06-14 0:33 ` [PATCH 2/8] blkcg, writeback: Implement wbc_blkcg_css() Tejun Heo
2019-06-14 13:41 ` Josef Bacik
2019-06-14 0:33 ` [PATCH 3/8] blkcg: implement REQ_CGROUP_PUNT Tejun Heo
2019-06-14 13:43 ` Josef Bacik
2019-06-14 0:33 ` [PATCH 4/8] Btrfs: stop using btrfs_schedule_bio() Tejun Heo
2019-06-14 13:45 ` Josef Bacik
2019-06-14 0:33 ` [PATCH 5/8] Btrfs: delete the entire async bio submission framework Tejun Heo
2019-06-14 13:48 ` Josef Bacik
2019-06-14 0:33 ` Tejun Heo [this message]
2019-06-14 13:48 ` [PATCH 6/8] Btrfs: only associate the locked page with one async_cow struct Josef Bacik
2019-06-14 0:33 ` [PATCH 7/8] Btrfs: use REQ_CGROUP_PUNT for worker thread submitted bios Tejun Heo
2019-06-14 13:50 ` Josef Bacik
2019-06-14 15:15 ` David Sterba
2019-06-14 15:34 ` Tejun Heo
2019-06-14 0:33 ` [PATCH 8/8] Btrfs: extent_write_locked_range() should attach inode->i_wb Tejun Heo
2019-06-14 14:12 ` Josef Bacik
2019-06-14 15:32 ` [PATCH v2 " Tejun Heo
2019-06-14 17:46 ` [PATCHSET btrfs/for-next] btrfs: fix cgroup writeback support Tejun Heo
Reply instructions:
You may reply publicly 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=20190614003350.1178444-7-tj@kernel.org \
--to=tj@kernel.org \
--cc=axboe@kernel.dk \
--cc=clm@fb.com \
--cc=dsterba@suse.com \
--cc=jack@suse.cz \
--cc=josef@toxicpanda.com \
--cc=kernel-team@fb.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-kernel@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).