Skip to content

Commit e4447ca

Browse files
adam900710adam900710
committed
btrfs: always wait for ordered extents to avoid OE races
[BUG] Syzbot reported a bug that there can be conflicting OEs for the same range: BTRFS critical (device loop4): panic in insert_ordered_extent:264: overlapping ordered extents, existing oe file_offset 16384 num_bytes 430080 flags 0x1089, new oe file_offset 16384 num_bytes 430080 flags 0x80 (errno=-17 Object alrea[ 179.162726][ T6897] BTRFS critical (device loop4): panic in insert_ordered_extent:264: overlapping ordered extents, existing oe file_offset 16384 num_bytes 430080 flags 0x1089, new oe file_offset 16384 num_bytes 430080 flags 0x80 (errno=-17 Object already exists) ------------[ cut here ]------------ kernel BUG at fs/btrfs/ordered-data.c:264! Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/09/2026 RIP: 0010:btrfs_alloc_ordered_extent+0x943/0xad0 Call Trace: <TASK> cow_file_range+0x744/0x12a0 fallback_to_cow+0x5ea/0xa00 run_delalloc_nocow+0x110c/0x17a0 btrfs_run_delalloc_range+0xbe4/0x1c20 writepage_delalloc+0x104d/0x1ba0 btrfs_writepages+0x1667/0x28b0 do_writepages+0x338/0x560 filemap_fdatawrite_range+0x1f2/0x300 btrfs_fdatawrite_range+0x54/0xf0 btrfs_direct_write+0x6a0/0xc30 btrfs_do_write_iter+0x329/0x790 do_iter_readv_writev+0x624/0x8d0 vfs_writev+0x34c/0x990 __se_sys_pwritev2+0x17a/0x2a0 do_syscall_64+0x174/0x580 entry_SYSCALL_64_after_hwframe+0x77/0x7f </TASK> ---[ end trace 0000000000000000 ]--- [CAUSE] Since commit ff66fe6 ("btrfs: fix incorrect buffered IO fallback for append direct writes"), if the direct IO finished short, we will revert the isize back to the original one, so that append writes can be respected during the buffered fallback. Normally we rely on lock_and_cleanup_extent_if_need() function during buffered writeback to wait for any existing ordered extents. But that ordered extent waiting only happens if the start_pos is inside the isize. Since we have reverted the isize during failed direct IO, we will not wait for any ordered extents. This means we can have a race where the direct IO OE is still in the tree, finished but not yet removed, then we're inserting the OE for the buffered write, causing the above crash. [FIX] Make the OE wait to be unconditional, to handle the reverted isize situation. And since lock_and_cleanup_extent_if_need() now either lock the extents or return -EAGAIN, also remove the branches that handles no-extent-locked cases, and rename it to remove the "_if_need" suffix. The following micro benchmark shows the runtime difference for btrfs_buffered_write(), doing `xfs_io -f -c "pwrite 0 1m"` workload, all values are the average runtime in nano seconds. function runtime | before | after -----------------------------------+-------------+--------------- lock_and_cleanup_extent_if_need() | 58.2 | 183.0 btrfs_buffered_write() | 2115.6 | 2973.3 The overall runtime of btrfs_buffered_write() is still pretty tiny (still less than 3 micro seconds), I'd say the extra cost is still acceptable. An alternative to fix this problem is to wait ordered extents during iomap_end() where the isize revert is done. But that solution will break nowait requirement, as if a nowait direct IO finished short, we have to wait for the OEs unconditionally or the next append buffered IO can still hit the same problem. So here we have to move the wait cost to buffered write, but at least the code is slightly more streamline. Reported-by: syzbot+ba2afde329fc27e3f22e@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?extid=ba2afde329fc27e3f22e Fixes: ff66fe6 ("btrfs: fix incorrect buffered IO fallback for append direct writes") Reviewed-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: Qu Wenruo <wqu@suse.com>
1 parent b899628 commit e4447ca

1 file changed

Lines changed: 40 additions & 62 deletions

File tree

‎fs/btrfs/file.c‎

Lines changed: 40 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -875,70 +875,64 @@ static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_
875875

876876
/*
877877
* Locks the extent and properly waits for data=ordered extents to finish
878-
* before allowing the folios to be modified if need.
878+
* before allowing the folios to be modified.
879879
*
880880
* Return:
881-
* 1 - the extent is locked
882-
* 0 - the extent is not locked, and everything is OK
881+
* 0 - the extent is locked
883882
* -EAGAIN - need to prepare the folios again
884883
*/
885884
static noinline int
886-
lock_and_cleanup_extent_if_need(struct btrfs_inode *inode, struct folio *folio,
887-
loff_t pos, size_t write_bytes,
888-
u64 *lockstart, u64 *lockend, bool nowait,
889-
struct extent_state **cached_state)
885+
lock_and_cleanup_extent(struct btrfs_inode *inode, struct folio *folio,
886+
loff_t pos, size_t write_bytes,
887+
u64 *lockstart, u64 *lockend, bool nowait,
888+
struct extent_state **cached_state)
890889
{
891890
struct btrfs_fs_info *fs_info = inode->root->fs_info;
891+
struct btrfs_ordered_extent *ordered;
892892
u64 start_pos;
893893
u64 last_pos;
894-
int ret = 0;
895894

896895
start_pos = round_down(pos, fs_info->sectorsize);
897896
last_pos = round_up(pos + write_bytes, fs_info->sectorsize) - 1;
898897

899-
if (start_pos < inode->vfs_inode.i_size) {
900-
struct btrfs_ordered_extent *ordered;
901-
902-
if (nowait) {
903-
if (!btrfs_try_lock_extent(&inode->io_tree, start_pos,
904-
last_pos, cached_state)) {
905-
folio_unlock(folio);
906-
folio_put(folio);
907-
return -EAGAIN;
908-
}
909-
} else {
910-
btrfs_lock_extent(&inode->io_tree, start_pos, last_pos,
911-
cached_state);
912-
}
913-
914-
ordered = btrfs_lookup_ordered_range(inode, start_pos,
915-
last_pos - start_pos + 1);
916-
if (ordered &&
917-
ordered->file_offset + ordered->num_bytes > start_pos &&
918-
ordered->file_offset <= last_pos) {
919-
btrfs_unlock_extent(&inode->io_tree, start_pos, last_pos,
920-
cached_state);
898+
if (nowait) {
899+
if (!btrfs_try_lock_extent(&inode->io_tree, start_pos,
900+
last_pos, cached_state)) {
921901
folio_unlock(folio);
922902
folio_put(folio);
923-
btrfs_start_ordered_extent(ordered);
924-
btrfs_put_ordered_extent(ordered);
925903
return -EAGAIN;
926904
}
927-
if (ordered)
928-
btrfs_put_ordered_extent(ordered);
905+
} else {
906+
btrfs_lock_extent(&inode->io_tree, start_pos, last_pos,
907+
cached_state);
908+
}
929909

930-
*lockstart = start_pos;
931-
*lockend = last_pos;
932-
ret = 1;
910+
ordered = btrfs_lookup_ordered_range(inode, start_pos,
911+
last_pos - start_pos + 1);
912+
if (ordered &&
913+
ordered->file_offset + ordered->num_bytes > start_pos &&
914+
ordered->file_offset <= last_pos) {
915+
btrfs_unlock_extent(&inode->io_tree, start_pos, last_pos,
916+
cached_state);
917+
folio_unlock(folio);
918+
folio_put(folio);
919+
btrfs_start_ordered_extent(ordered);
920+
btrfs_put_ordered_extent(ordered);
921+
return -EAGAIN;
933922
}
923+
if (ordered)
924+
btrfs_put_ordered_extent(ordered);
925+
926+
*lockstart = start_pos;
927+
*lockend = last_pos;
934928

935929
/*
936930
* We should be called after prepare_one_folio() which should have locked
937931
* all pages in the range.
938932
*/
939933
WARN_ON(!folio_test_locked(folio));
940934

941-
return ret;
935+
return 0;
942936
}
943937

944938
/*
@@ -1195,7 +1189,6 @@ static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *iter,
11951189
const u64 reserved_start = round_down(start, fs_info->sectorsize);
11961190
u64 reserved_len;
11971191
struct folio *folio = NULL;
1198-
int extents_locked;
11991192
u64 lockstart;
12001193
u64 lockend;
12011194
bool only_release_metadata = false;
@@ -1253,18 +1246,16 @@ static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *iter,
12531246
reserved_len = last_block - reserved_start;
12541247
}
12551248

1256-
extents_locked = lock_and_cleanup_extent_if_need(inode, folio, start,
1257-
write_bytes, &lockstart,
1258-
&lockend, nowait,
1259-
&cached_state);
1260-
if (extents_locked < 0) {
1261-
if (!nowait && extents_locked == -EAGAIN)
1249+
ret = lock_and_cleanup_extent(inode, folio, start, write_bytes,
1250+
&lockstart, &lockend, nowait, &cached_state);
1251+
if (ret < 0) {
1252+
if (!nowait)
12621253
goto again;
12631254

12641255
btrfs_delalloc_release_extents(inode, reserved_len);
12651256
release_space(inode, *data_reserved, reserved_start, reserved_len,
12661257
only_release_metadata);
1267-
return extents_locked;
1258+
return ret;
12681259
}
12691260

12701261
copied = copy_folio_from_iter_atomic(folio, offset_in_folio(folio, start),
@@ -1288,11 +1279,8 @@ static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *iter,
12881279

12891280
/* No copied bytes, unlock, release reserved space and exit. */
12901281
if (copied == 0) {
1291-
if (extents_locked)
1292-
btrfs_unlock_extent(&inode->io_tree, lockstart, lockend,
1293-
&cached_state);
1294-
else
1295-
btrfs_free_extent_state(cached_state);
1282+
btrfs_unlock_extent(&inode->io_tree, lockstart, lockend,
1283+
&cached_state);
12961284
btrfs_delalloc_release_extents(inode, reserved_len);
12971285
release_space(inode, *data_reserved, reserved_start, reserved_len,
12981286
only_release_metadata);
@@ -1311,17 +1299,7 @@ static int copy_one_range(struct btrfs_inode *inode, struct iov_iter *iter,
13111299

13121300
ret = btrfs_dirty_folio(inode, folio, start, copied, &cached_state,
13131301
only_release_metadata);
1314-
/*
1315-
* If we have not locked the extent range, because the range's start
1316-
* offset is >= i_size, we might still have a non-NULL cached extent
1317-
* state, acquired while marking the extent range as delalloc through
1318-
* btrfs_dirty_page(). Therefore free any possible cached extent state
1319-
* to avoid a memory leak.
1320-
*/
1321-
if (extents_locked)
1322-
btrfs_unlock_extent(&inode->io_tree, lockstart, lockend, &cached_state);
1323-
else
1324-
btrfs_free_extent_state(cached_state);
1302+
btrfs_unlock_extent(&inode->io_tree, lockstart, lockend, &cached_state);
13251303

13261304
btrfs_delalloc_release_extents(inode, reserved_len);
13271305
if (ret) {

0 commit comments

Comments
 (0)