* [PATCH RFC] btrfs: handle the added ordered extent when bio submission fails
@ 2021-07-20 6:56 Qu Wenruo
2021-07-20 11:26 ` Qu Wenruo
0 siblings, 1 reply; 2+ messages in thread
From: Qu Wenruo @ 2021-07-20 6:56 UTC (permalink / raw)
To: linux-btrfs
[BUG]
There is a strange bug that when running "-o compress" mount option for
subpage case, btrfs/160 has a random chance (~20%) to crash the system:
BTRFS critical (device dm-7): panic in __btrfs_add_ordered_extent:238: inconsistency in ordered tree at offset 0 (errno=-17 Object already exists)
------------[ cut here ]------------
kernel BUG at fs/btrfs/ordered-data.c:238!
Internal error: Oops - BUG: 0 [#1] SMP
pc : __btrfs_add_ordered_extent+0x550/0x670 [btrfs]
lr : __btrfs_add_ordered_extent+0x550/0x670 [btrfs]
Call trace:
__btrfs_add_ordered_extent+0x550/0x670 [btrfs]
btrfs_add_ordered_extent+0x2c/0x50 [btrfs]
run_delalloc_nocow+0x81c/0x8fc [btrfs]
btrfs_run_delalloc_range+0xa4/0x390 [btrfs]
writepage_delalloc+0xc0/0x1ac [btrfs]
__extent_writepage+0xf4/0x370 [btrfs]
extent_write_cache_pages+0x288/0x4f4 [btrfs]
extent_writepages+0x58/0xe0 [btrfs]
btrfs_writepages+0x1c/0x30 [btrfs]
do_writepages+0x60/0x110
__filemap_fdatawrite_range+0x108/0x170
filemap_fdatawrite_range+0x20/0x30
btrfs_fdatawrite_range+0x34/0x4dc [btrfs]
__btrfs_write_out_cache+0x34c/0x480 [btrfs]
btrfs_write_out_cache+0x144/0x220 [btrfs]
btrfs_start_dirty_block_groups+0x3ac/0x6b0 [btrfs]
btrfs_commit_transaction+0xd0/0xbb4 [btrfs]
btrfs_sync_fs+0x64/0x1cc [btrfs]
sync_fs_one_sb+0x3c/0x50
iterate_supers+0xcc/0x1d4
ksys_sync+0x6c/0xd0
__arm64_sys_sync+0x1c/0x30
invoke_syscall+0x50/0x120
el0_svc_common.constprop.0+0x4c/0xd4
do_el0_svc+0x30/0x9c
el0_svc+0x2c/0x54
el0_sync_handler+0x1a8/0x1b0
el0_sync+0x198/0x1c0
---[ end trace 336f67369ae6e0af ]---
However if we disable v1 space cache, the crash just disappera.
[CAUSE]
The offending inode is in fact a v1 space cache inode.
The crash happens in the following sequence:
- Writing back space cache for inode 1/257 (root 1, ino 257).
The space cache is a little big in this case, 983040 bytes (960K).
- Run delalloc range for the free space cache inode
Ordered extent [0, 960K) is inserted for inode 1/257.
- Call __extent_writepage_io() for each page
The first 3 pages submitted without problem.
But the 4th page get submitted but failed due to the injected error.
- Error out without handling the submitted ordered extent
Since only 4 pages submitted and finished, the ordered extent
[0, 960K) is still there.
- Retry the free space cache writeback with some update
Now inode 1/257 have its contents updated, and need to try writeback
again.
- Run delalloc range for the free space cache inode.
Ordered extent [0, 960K) is going to be inserted again.
But previously added ordered extent [0, 960K) is still there,
triggering the crash.
This problem only happens for free space cache inode, as other inode
won't try to re-write the same inode after error.
Although the free space cache size 960K seems to be a subpage specific
problem, but the root cause is still there.
[FIX]
Fix the problem by introducing a new helper,
btrfs_cleanup_hanging_ordered_extents(), to cleanup the added ordered
extent after bio submission error.
This function will call btrfs_mark_ordered_io_finished() on each
involved page, and finish the ordered extent and clear the page Private
bit.
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Reason for RFC:
Current version works and will no longer crash at btrfs/160.
(Although still need some polish, as the delalloc space for v1 cache
inode is not yet properly cleaned up)
But I'm not sure why we never hit such problem for v1 space cache write
back, and not sure if this is the correct way to go.
Should we just error out if we failed to write back v1 space cache?
Anyway, send out the RFC version to make sure if the fix is the correct
way to go.
---
fs/btrfs/ctree.h | 3 +++
fs/btrfs/extent_io.c | 15 +++++++++++++++
fs/btrfs/inode.c | 41 +++++++++++++++++++++++++++++++++++++++++
fs/btrfs/transaction.c | 11 +++++++++++
4 files changed, 70 insertions(+)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index e265a7eb0d5c..8c32f2119790 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3194,6 +3194,9 @@ int btrfs_writepage_cow_fixup(struct page *page, u64 start, u64 end);
void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
struct page *page, u64 start,
u64 end, int uptodate);
+void btrfs_cleanup_hanging_ordered_extents(struct btrfs_fs_info *fs_info,
+ struct btrfs_inode *inode,
+ u64 start);
extern const struct dentry_operations btrfs_dentry_operations;
extern const struct iomap_ops btrfs_dio_iomap_ops;
extern const struct iomap_dio_ops btrfs_dio_ops;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index fff1a4d8fe25..cfbed849b598 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4137,6 +4137,21 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
ret = ret < 0 ? ret : -EIO;
end_extent_writepage(page, ret, page_start, page_end);
}
+
+ /*
+ * We may have ordered extent already allocated, and since we error out
+ * the remaining pages will never be submitted thus the ordered extent
+ * will hang there forever.
+ *
+ * Such hanging OE would cause problem for things like free space cache
+ * where we could retry to write it back.
+ * So here if we hit error submitting the IO, we need to cleanup the
+ * hanging ordered extent.
+ */
+ if (ret < 0)
+ btrfs_cleanup_hanging_ordered_extents(fs_info, BTRFS_I(inode),
+ page_end + 1);
+
if (epd->extent_locked) {
/*
* If epd->extent_locked, it's from extent_write_locked_range(),
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e8af0021af78..05d392315f3b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3211,6 +3211,47 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
finish_ordered_fn, uptodate);
}
+/*
+ * Cleanup any added ordered ordered extent from @start.
+ *
+ * This fucntion should be called when ordered extent is submitted but by some
+ * how the bio submission hits some error and has to error out.
+ */
+void btrfs_cleanup_hanging_ordered_extents(struct btrfs_fs_info *fs_info,
+ struct btrfs_inode *inode,
+ u64 start)
+{
+ struct btrfs_ordered_extent *oe;
+ u64 cur;
+ u64 end;
+
+ ASSERT(IS_ALIGNED(start, PAGE_SIZE));
+ /* Grab the ordered extent covers the bytenr to calculate the end. */
+ oe = btrfs_lookup_first_ordered_extent(inode, start);
+ if (!oe)
+ return;
+ end = oe->file_offset + oe->num_bytes;
+ btrfs_put_ordered_extent(oe);
+
+ /* Finish the ordered extent of the remaining pages */
+ for (cur = start; cur < end; cur += PAGE_SIZE) {
+ struct page *page;
+ u32 len;
+
+ page = find_lock_page(inode->vfs_inode.i_mapping,
+ cur >> PAGE_SHIFT);
+ if (!page)
+ continue;
+
+ len = min(page_offset(page) + PAGE_SIZE, end + 1) -
+ page_offset(page);
+ btrfs_mark_ordered_io_finished(inode, page, cur, len,
+ finish_ordered_fn, false);
+ unlock_page(page);
+ put_page(page);
+ }
+}
+
/*
* check_data_csum - verify checksum of one sector of uncompressed data
* @inode: inode
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 14b9fdc8aaa9..05d786401374 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -313,6 +313,17 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
if (type == TRANS_ATTACH)
return -ENOENT;
+ /*
+ * TRANS_JOIN_NOLOCK is only utilized by free space cache endio.
+ * Normally it only happens during commit transaction, but it's
+ * possible that we error out and abort transaction.
+ *
+ * In that case, the endio for free space cache may get no running
+ * trans.
+ * Stay cool and just return error for the only caller.
+ */
+ if (type == TRANS_JOIN_NOLOCK)
+ return -ENOENT;
/*
* JOIN_NOLOCK only happens during the transaction commit, so
* it is impossible that ->running_transaction is NULL
--
2.32.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH RFC] btrfs: handle the added ordered extent when bio submission fails
2021-07-20 6:56 [PATCH RFC] btrfs: handle the added ordered extent when bio submission fails Qu Wenruo
@ 2021-07-20 11:26 ` Qu Wenruo
0 siblings, 0 replies; 2+ messages in thread
From: Qu Wenruo @ 2021-07-20 11:26 UTC (permalink / raw)
To: linux-btrfs
On 2021/7/20 下午2:56, Qu Wenruo wrote:
> [BUG]
> There is a strange bug that when running "-o compress" mount option for
> subpage case, btrfs/160 has a random chance (~20%) to crash the system:
>
> BTRFS critical (device dm-7): panic in __btrfs_add_ordered_extent:238: inconsistency in ordered tree at offset 0 (errno=-17 Object already exists)
> ------------[ cut here ]------------
> kernel BUG at fs/btrfs/ordered-data.c:238!
> Internal error: Oops - BUG: 0 [#1] SMP
> pc : __btrfs_add_ordered_extent+0x550/0x670 [btrfs]
> lr : __btrfs_add_ordered_extent+0x550/0x670 [btrfs]
> Call trace:
> __btrfs_add_ordered_extent+0x550/0x670 [btrfs]
> btrfs_add_ordered_extent+0x2c/0x50 [btrfs]
> run_delalloc_nocow+0x81c/0x8fc [btrfs]
> btrfs_run_delalloc_range+0xa4/0x390 [btrfs]
> writepage_delalloc+0xc0/0x1ac [btrfs]
> __extent_writepage+0xf4/0x370 [btrfs]
> extent_write_cache_pages+0x288/0x4f4 [btrfs]
> extent_writepages+0x58/0xe0 [btrfs]
> btrfs_writepages+0x1c/0x30 [btrfs]
> do_writepages+0x60/0x110
> __filemap_fdatawrite_range+0x108/0x170
> filemap_fdatawrite_range+0x20/0x30
> btrfs_fdatawrite_range+0x34/0x4dc [btrfs]
> __btrfs_write_out_cache+0x34c/0x480 [btrfs]
> btrfs_write_out_cache+0x144/0x220 [btrfs]
> btrfs_start_dirty_block_groups+0x3ac/0x6b0 [btrfs]
> btrfs_commit_transaction+0xd0/0xbb4 [btrfs]
> btrfs_sync_fs+0x64/0x1cc [btrfs]
> sync_fs_one_sb+0x3c/0x50
> iterate_supers+0xcc/0x1d4
> ksys_sync+0x6c/0xd0
> __arm64_sys_sync+0x1c/0x30
> invoke_syscall+0x50/0x120
> el0_svc_common.constprop.0+0x4c/0xd4
> do_el0_svc+0x30/0x9c
> el0_svc+0x2c/0x54
> el0_sync_handler+0x1a8/0x1b0
> el0_sync+0x198/0x1c0
> ---[ end trace 336f67369ae6e0af ]---
>
> However if we disable v1 space cache, the crash just disappera.
>
> [CAUSE]
> The offending inode is in fact a v1 space cache inode.
>
> The crash happens in the following sequence:
>
> - Writing back space cache for inode 1/257 (root 1, ino 257).
> The space cache is a little big in this case, 983040 bytes (960K).
>
> - Run delalloc range for the free space cache inode
> Ordered extent [0, 960K) is inserted for inode 1/257.
>
> - Call __extent_writepage_io() for each page
> The first 3 pages submitted without problem.
> But the 4th page get submitted but failed due to the injected error.
>
> - Error out without handling the submitted ordered extent
> Since only 4 pages submitted and finished, the ordered extent
> [0, 960K) is still there.
>
> - Retry the free space cache writeback with some update
> Now inode 1/257 have its contents updated, and need to try writeback
> again.
>
> - Run delalloc range for the free space cache inode.
> Ordered extent [0, 960K) is going to be inserted again.
> But previously added ordered extent [0, 960K) is still there,
> triggering the crash.
>
> This problem only happens for free space cache inode, as other inode
> won't try to re-write the same inode after error.
>
> Although the free space cache size 960K seems to be a subpage specific
> problem, but the root cause is still there.
>
> [FIX]
> Fix the problem by introducing a new helper,
> btrfs_cleanup_hanging_ordered_extents(), to cleanup the added ordered
> extent after bio submission error.
>
> This function will call btrfs_mark_ordered_io_finished() on each
> involved page, and finish the ordered extent and clear the page Private
> bit.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Reason for RFC:
>
> Current version works and will no longer crash at btrfs/160.
> (Although still need some polish, as the delalloc space for v1 cache
> inode is not yet properly cleaned up)
>
> But I'm not sure why we never hit such problem for v1 space cache write
> back, and not sure if this is the correct way to go.
OK, after more digging, it's a subpage specific problem.
The offending code in fact lies at the ending part of __extent_writepage():
done:
if (nr == 0) {
/* make sure the mapping tag for page dirty gets cleared */
set_page_writeback(page);
end_page_writeback(page);
}
if (PageError(page)) {
ret = ret < 0 ? ret : -EIO;
end_extent_writepage(page, ret, page_start, page_end);
}
If by somehow the page we're submitting is marked error, then we error out.
The caller will just stop submitting the remaining pages.
This looks correct but in fact we should just continue submitting, and
it's exactly what non-subpage case does.
For non-subpage case, one page only contains one sector, thus we either
add the page into previous bio, or submit previous bio and allocate a
new bio, and add the page into the newly allocated bio.
We never have the current page submitted while we're holding it.
This means, even for the dm-dust/error injection case, that PageError()
branch will never be true, and we always return 0 (if no other error
happens).
But for subpage case, we can have part of the page submitted, and the
endio will mark part of the page error.
In that case, dm-dust can easily make __extent_writepage() to return
error, and stop submitting the remaining part.
Thus to properly fix this problem, I'm afraid I have to remove the if
(PageError()) branch, ironically, to fix a bug...
Thanks,
Qu
>
> Should we just error out if we failed to write back v1 space cache?
>
> Anyway, send out the RFC version to make sure if the fix is the correct
> way to go.
> ---
> fs/btrfs/ctree.h | 3 +++
> fs/btrfs/extent_io.c | 15 +++++++++++++++
> fs/btrfs/inode.c | 41 +++++++++++++++++++++++++++++++++++++++++
> fs/btrfs/transaction.c | 11 +++++++++++
> 4 files changed, 70 insertions(+)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index e265a7eb0d5c..8c32f2119790 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3194,6 +3194,9 @@ int btrfs_writepage_cow_fixup(struct page *page, u64 start, u64 end);
> void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
> struct page *page, u64 start,
> u64 end, int uptodate);
> +void btrfs_cleanup_hanging_ordered_extents(struct btrfs_fs_info *fs_info,
> + struct btrfs_inode *inode,
> + u64 start);
> extern const struct dentry_operations btrfs_dentry_operations;
> extern const struct iomap_ops btrfs_dio_iomap_ops;
> extern const struct iomap_dio_ops btrfs_dio_ops;
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index fff1a4d8fe25..cfbed849b598 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4137,6 +4137,21 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
> ret = ret < 0 ? ret : -EIO;
> end_extent_writepage(page, ret, page_start, page_end);
> }
> +
> + /*
> + * We may have ordered extent already allocated, and since we error out
> + * the remaining pages will never be submitted thus the ordered extent
> + * will hang there forever.
> + *
> + * Such hanging OE would cause problem for things like free space cache
> + * where we could retry to write it back.
> + * So here if we hit error submitting the IO, we need to cleanup the
> + * hanging ordered extent.
> + */
> + if (ret < 0)
> + btrfs_cleanup_hanging_ordered_extents(fs_info, BTRFS_I(inode),
> + page_end + 1);
> +
> if (epd->extent_locked) {
> /*
> * If epd->extent_locked, it's from extent_write_locked_range(),
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e8af0021af78..05d392315f3b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3211,6 +3211,47 @@ void btrfs_writepage_endio_finish_ordered(struct btrfs_inode *inode,
> finish_ordered_fn, uptodate);
> }
>
> +/*
> + * Cleanup any added ordered ordered extent from @start.
> + *
> + * This fucntion should be called when ordered extent is submitted but by some
> + * how the bio submission hits some error and has to error out.
> + */
> +void btrfs_cleanup_hanging_ordered_extents(struct btrfs_fs_info *fs_info,
> + struct btrfs_inode *inode,
> + u64 start)
> +{
> + struct btrfs_ordered_extent *oe;
> + u64 cur;
> + u64 end;
> +
> + ASSERT(IS_ALIGNED(start, PAGE_SIZE));
> + /* Grab the ordered extent covers the bytenr to calculate the end. */
> + oe = btrfs_lookup_first_ordered_extent(inode, start);
> + if (!oe)
> + return;
> + end = oe->file_offset + oe->num_bytes;
> + btrfs_put_ordered_extent(oe);
> +
> + /* Finish the ordered extent of the remaining pages */
> + for (cur = start; cur < end; cur += PAGE_SIZE) {
> + struct page *page;
> + u32 len;
> +
> + page = find_lock_page(inode->vfs_inode.i_mapping,
> + cur >> PAGE_SHIFT);
> + if (!page)
> + continue;
> +
> + len = min(page_offset(page) + PAGE_SIZE, end + 1) -
> + page_offset(page);
> + btrfs_mark_ordered_io_finished(inode, page, cur, len,
> + finish_ordered_fn, false);
> + unlock_page(page);
> + put_page(page);
> + }
> +}
> +
> /*
> * check_data_csum - verify checksum of one sector of uncompressed data
> * @inode: inode
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 14b9fdc8aaa9..05d786401374 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -313,6 +313,17 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
> if (type == TRANS_ATTACH)
> return -ENOENT;
>
> + /*
> + * TRANS_JOIN_NOLOCK is only utilized by free space cache endio.
> + * Normally it only happens during commit transaction, but it's
> + * possible that we error out and abort transaction.
> + *
> + * In that case, the endio for free space cache may get no running
> + * trans.
> + * Stay cool and just return error for the only caller.
> + */
> + if (type == TRANS_JOIN_NOLOCK)
> + return -ENOENT;
> /*
> * JOIN_NOLOCK only happens during the transaction commit, so
> * it is impossible that ->running_transaction is NULL
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-07-20 11:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-20 6:56 [PATCH RFC] btrfs: handle the added ordered extent when bio submission fails Qu Wenruo
2021-07-20 11:26 ` Qu Wenruo
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).