| Message ID | 101430650a35b55b7a32d895fd292226d13346eb.1716486455.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix crash on racing fsync and size-extending direct I/O into prealloc | expand |
On Thu, May 23, 2024 at 8:34 PM Omar Sandoval <osandov@osandov.com> wrote: > > From: Omar Sandoval <osandov@fb.com> > > We have been seeing crashes on duplicate keys in > btrfs_set_item_key_safe(): > > BTRFS critical (device vdb): slot 4 key (450 108 8192) new key (450 108 8192) > ------------[ cut here ]------------ > kernel BUG at fs/btrfs/ctree.c:2620! > invalid opcode: 0000 [#1] PREEMPT SMP PTI > CPU: 0 PID: 3139 Comm: xfs_io Kdump: loaded Not tainted 6.9.0 #6 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014 > RIP: 0010:btrfs_set_item_key_safe+0x11f/0x290 [btrfs] > > With the following stack trace: > > #0 btrfs_set_item_key_safe (fs/btrfs/ctree.c:2620:4) > #1 btrfs_drop_extents (fs/btrfs/file.c:411:4) > #2 log_one_extent (fs/btrfs/tree-log.c:4732:9) > #3 btrfs_log_changed_extents (fs/btrfs/tree-log.c:4955:9) > #4 btrfs_log_inode (fs/btrfs/tree-log.c:6626:9) > #5 btrfs_log_inode_parent (fs/btrfs/tree-log.c:7070:8) > #6 btrfs_log_dentry_safe (fs/btrfs/tree-log.c:7171:8) > #7 btrfs_sync_file (fs/btrfs/file.c:1933:8) > #8 vfs_fsync_range (fs/sync.c:188:9) > #9 vfs_fsync (fs/sync.c:202:9) > #10 do_fsync (fs/sync.c:212:9) > #11 __do_sys_fdatasync (fs/sync.c:225:9) > #12 __se_sys_fdatasync (fs/sync.c:223:1) > #13 __x64_sys_fdatasync (fs/sync.c:223:1) > #14 do_syscall_x64 (arch/x86/entry/common.c:52:14) > #15 do_syscall_64 (arch/x86/entry/common.c:83:7) > #16 entry_SYSCALL_64+0xaf/0x14c (arch/x86/entry/entry_64.S:121) > > So we're logging a changed extent from fsync, which is splitting an > extent in the log tree. But this split part already exists in the tree, > triggering the BUG(). > > This is the state of the log tree at the time of the crash, dumped with > drgn (https://github.com/osandov/drgn/blob/main/contrib/btrfs_tree.py) > to get more details than btrfs_print_leaf() gives us: > > >>> print_extent_buffer(prog.crashed_thread().stack_trace()[0]["eb"]) > leaf 33439744 level 0 items 72 generation 9 owner 18446744073709551610 > leaf 33439744 flags 0x100000000000000 > fs uuid e5bd3946-400c-4223-8923-190ef1f18677 > chunk uuid d58cb17e-6d02-494a-829a-18b7d8a399da > item 0 key (450 INODE_ITEM 0) itemoff 16123 itemsize 160 > generation 7 transid 9 size 8192 nbytes 8473563889606862198 > block group 0 mode 100600 links 1 uid 0 gid 0 rdev 0 > sequence 204 flags 0x10(PREALLOC) > atime 1716417703.220000000 (2024-05-22 15:41:43) > ctime 1716417704.983333333 (2024-05-22 15:41:44) > mtime 1716417704.983333333 (2024-05-22 15:41:44) > otime 17592186044416.000000000 (559444-03-08 01:40:16) > item 1 key (450 INODE_REF 256) itemoff 16110 itemsize 13 > index 195 namelen 3 name: 193 > item 2 key (450 XATTR_ITEM 1640047104) itemoff 16073 itemsize 37 > location key (0 UNKNOWN.0 0) type XATTR > transid 7 data_len 1 name_len 6 > name: user.a > data a > item 3 key (450 EXTENT_DATA 0) itemoff 16020 itemsize 53 > generation 9 type 1 (regular) > extent data disk byte 303144960 nr 12288 > extent data offset 0 nr 4096 ram 12288 > extent compression 0 (none) > item 4 key (450 EXTENT_DATA 4096) itemoff 15967 itemsize 53 > generation 9 type 2 (prealloc) > prealloc data disk byte 303144960 nr 12288 > prealloc data offset 4096 nr 8192 > item 5 key (450 EXTENT_DATA 8192) itemoff 15914 itemsize 53 > generation 9 type 2 (prealloc) > prealloc data disk byte 303144960 nr 12288 > prealloc data offset 8192 nr 4096 > ... > > So the real problem happened earlier: notice that items 4 (4k-12k) and 5 > (8k-12k) overlap. Both are prealloc extents. Item 4 straddles i_size and > item 5 starts at i_size. > > Here is the state of the filesystem tree at the time of the crash: > > >>> root = prog.crashed_thread().stack_trace()[2]["inode"].root > >>> ret, nodes, slots = btrfs_search_slot(root, BtrfsKey(450, 0, 0)) > >>> print_extent_buffer(nodes[0]) > leaf 30425088 level 0 items 184 generation 9 owner 5 > leaf 30425088 flags 0x100000000000000 > fs uuid e5bd3946-400c-4223-8923-190ef1f18677 > chunk uuid d58cb17e-6d02-494a-829a-18b7d8a399da > ... > item 179 key (450 INODE_ITEM 0) itemoff 4907 itemsize 160 > generation 7 transid 7 size 4096 nbytes 12288 > block group 0 mode 100600 links 1 uid 0 gid 0 rdev 0 > sequence 6 flags 0x10(PREALLOC) > atime 1716417703.220000000 (2024-05-22 15:41:43) > ctime 1716417703.220000000 (2024-05-22 15:41:43) > mtime 1716417703.220000000 (2024-05-22 15:41:43) > otime 1716417703.220000000 (2024-05-22 15:41:43) > item 180 key (450 INODE_REF 256) itemoff 4894 itemsize 13 > index 195 namelen 3 name: 193 > item 181 key (450 XATTR_ITEM 1640047104) itemoff 4857 itemsize 37 > location key (0 UNKNOWN.0 0) type XATTR > transid 7 data_len 1 name_len 6 > name: user.a > data a > item 182 key (450 EXTENT_DATA 0) itemoff 4804 itemsize 53 > generation 9 type 1 (regular) > extent data disk byte 303144960 nr 12288 > extent data offset 0 nr 8192 ram 12288 > extent compression 0 (none) > item 183 key (450 EXTENT_DATA 8192) itemoff 4751 itemsize 53 > generation 9 type 2 (prealloc) > prealloc data disk byte 303144960 nr 12288 > prealloc data offset 8192 nr 4096 > > Item 5 in the log tree corresponds to item 183 in the filesystem tree, > but nothing matches item 4. Furthermore, item 183 is the last item in > the leaf. > > btrfs_log_prealloc_extents() is responsible for logging prealloc extents > beyond i_size. It first truncates any previously logged prealloc extents > that start beyond i_size. Then, it walks the filesystem tree and copies > the prealloc extent items to the log tree. > > If it hits the end of a leaf, then it calls btrfs_next_leaf(), which > unlocks the tree and does another search. However, while the filesystem > tree is unlocked, an ordered extent completion may modify the tree. In > particular, it may insert an extent item that overlaps with an extent > item that was already copied to the log tree. > > This may manifest in several ways depending on the exact scenario, > including an EEXIST error that is silently translated to a full sync, > overlapping items in the log tree, or this crash. This particular crash > is triggered by the following sequence of events: > > - Initially, the file has i_size=4k, a regular extent from 0-4k, and a > prealloc extent beyond i_size from 4k-12k. The prealloc extent item is > the last item in its B-tree leaf. > - The file is fsync'd, which copies its inode item and both extent items > to the log tree. > - An xattr is set on the file, which sets the > BTRFS_INODE_COPY_EVERYTHING flag. > - The range 4k-8k in the file is written using direct I/O. i_size is > extended to 8k, but the ordered extent is still in flight. > - The file is fsync'd. Since BTRFS_INODE_COPY_EVERYTHING is set, this > calls copy_inode_items_to_log(), which calls > btrfs_log_prealloc_extents(). > - btrfs_log_prealloc_extents() finds the 4k-12k prealloc extent in the > filesystem tree. Since it starts before i_size, it skips it. Since it > is the last item in its B-tree leaf, it calls btrfs_next_leaf(). > - btrfs_next_leaf() unlocks the path. > - The ordered extent completion runs, which converts the 4k-8k part of > the prealloc extent to written and inserts the remaining prealloc part > from 8k-12k. > - btrfs_next_leaf() does a search and finds the new prealloc extent > 8k-12k. > - btrfs_log_prealloc_extents() copies the 8k-12k prealloc extent into > the log tree. Note that it overlaps with the 4k-12k prealloc extent > that was copied to the log tree by the first fsync. > - fsync calls btrfs_log_changed_extents(), which tries to log the 4k-8k > extent that was written. > - This tries to drop the range 4k-8k in the log tree, which requires > adjusting the start of the 4k-12k prealloc extent in the log tree to > 8k. > - btrfs_set_item_key_safe() sees that there is already an extent > starting at 8k in the log tree and calls BUG(). This is all correct. Thanks for the detailed explanation, this is one more instance of tricky cases involving the last item and btrfs_next_leaf(). So you mention direct IO but that's only because the issue happened to be triggered with direct IO, but there's really nothing specific to direct IO here, can happen with buffered IO too. So I suggest dropping the "direct IO" part in the subject and just say something like "... size extending write into prealloc". Then don't forget to update the subject in the test case for the _fixed_by_kernel_commit call. > > Fix this by detecting when we're about to insert an overlapping file > extent item in the log tree and truncating the part that would overlap. > > Signed-off-by: Omar Sandoval <osandov@fb.com> > --- > Hi, > > I'm not sure if this is the best way to fix the problem, but hopefully > the commit message has enough detail to brainstorm a better solution if > not. I've also included an fstest that reproduces the issue. That seems reasonable, and I would fix it myself in a very similar way. > > Based on misc-next. Btw, nowadays we use the "for-next" branch at https://github.com/btrfs/linux/commits/for-next/ However this applies just fine in that for-next branch. A few comments belows. > > Thanks, > Omar > > > fs/btrfs/tree-log.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c > index 51a167559ae8..a7efd23acf50 100644 > --- a/fs/btrfs/tree-log.c > +++ b/fs/btrfs/tree-log.c > @@ -4783,6 +4783,7 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans, > bool dropped_extents = false; > u64 truncate_offset = i_size; > struct extent_buffer *leaf; > + struct btrfs_file_extent_item *ei; > int slot; > int ins_nr = 0; > int start_slot = 0; > @@ -4811,8 +4812,6 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans, > goto out; > > if (ret == 0) { > - struct btrfs_file_extent_item *ei; > - > leaf = path->nodes[0]; > slot = path->slots[0]; > ei = btrfs_item_ptr(leaf, slot, struct btrfs_file_extent_item); > @@ -4863,18 +4862,26 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans, > path->slots[0]++; > continue; > } > - if (!dropped_extents) { > - /* > - * Avoid logging extent items logged in past fsync calls > - * and leading to duplicate keys in the log tree. > - */ > + /* > + * Avoid overlapping items in the log tree. The first time we > + * get here, get rid of everything from a past fsync. After > + * that, if the current extent starts before the end of the last > + * extent we copied, truncate the last one. This can happen if > + * an ordered extent completion modifies the subvolume tree > + * while btrfs_next_leaf() has the tree unlocked. > + */ > + if (!dropped_extents || key.offset < truncate_offset) { > ret = truncate_inode_items(trans, root->log_root, inode, > - truncate_offset, > + min(key.offset, > + truncate_offset), For readability you can keep the min() expression in a single line. That would result in a 84 characters wide line, which is tolerated nowadays. > BTRFS_EXTENT_DATA_KEY); > if (ret) > goto out; > dropped_extents = true; > } > + ei = btrfs_item_ptr(leaf, slot, struct btrfs_file_extent_item); > + truncate_offset = (key.offset + > + btrfs_file_extent_num_bytes(leaf, ei)); The parentheses here are a bit odd and unnecessary. You can also use btrfs_file_extent_end(path) instead of the expression. Thanks. > if (ins_nr == 0) > start_slot = slot; > ins_nr++; > -- > 2.45.1 > >
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 51a167559ae8..a7efd23acf50 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -4783,6 +4783,7 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans, bool dropped_extents = false; u64 truncate_offset = i_size; struct extent_buffer *leaf; + struct btrfs_file_extent_item *ei; int slot; int ins_nr = 0; int start_slot = 0; @@ -4811,8 +4812,6 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans, goto out; if (ret == 0) { - struct btrfs_file_extent_item *ei; - leaf = path->nodes[0]; slot = path->slots[0]; ei = btrfs_item_ptr(leaf, slot, struct btrfs_file_extent_item); @@ -4863,18 +4862,26 @@ static int btrfs_log_prealloc_extents(struct btrfs_trans_handle *trans, path->slots[0]++; continue; } - if (!dropped_extents) { - /* - * Avoid logging extent items logged in past fsync calls - * and leading to duplicate keys in the log tree. - */ + /* + * Avoid overlapping items in the log tree. The first time we + * get here, get rid of everything from a past fsync. After + * that, if the current extent starts before the end of the last + * extent we copied, truncate the last one. This can happen if + * an ordered extent completion modifies the subvolume tree + * while btrfs_next_leaf() has the tree unlocked. + */ + if (!dropped_extents || key.offset < truncate_offset) { ret = truncate_inode_items(trans, root->log_root, inode, - truncate_offset, + min(key.offset, + truncate_offset), BTRFS_EXTENT_DATA_KEY); if (ret) goto out; dropped_extents = true; } + ei = btrfs_item_ptr(leaf, slot, struct btrfs_file_extent_item); + truncate_offset = (key.offset + + btrfs_file_extent_num_bytes(leaf, ei)); if (ins_nr == 0) start_slot = slot; ins_nr++;