在 2021/10/1 0:43, Jan Kara 写道:
Let me improve English a bit:
On Fri 03-09-21 14:27:48, yangerkun wrote:
We get a BUG as follow:
We hit the following bug:
[52117.465187] ------------[ cut here ]------------
[52117.465686] kernel BUG at fs/ext4/extents.c:1756!
...
[52117.478306] Call Trace:
[52117.478565] ext4_ext_shift_extents+0x3ee/0x710
[52117.479020] ext4_fallocate+0x139c/0x1b40
[52117.479405] ? __do_sys_newfstat+0x6b/0x80
[52117.479805] vfs_fallocate+0x151/0x4b0
[52117.480177] ksys_fallocate+0x4a/0xa0
[52117.480533] __x64_sys_fallocate+0x22/0x30
[52117.480930] do_syscall_64+0x35/0x80
[52117.481277] entry_SYSCALL_64_after_hwframe+0x44/0xae
[52117.481769] RIP: 0033:0x7fa062f855ca
static int ext4_ext_try_to_merge_right(struct inode *inode,
struct ext4_ext_path *path,
struct ext4_extent *ex)
{
struct ext4_extent_header *eh;
unsigned int depth, len;
int merge_done = 0, unwritten;
depth = ext_depth(inode);
BUG_ON(path[depth].p_hdr == NULL); <=== trigger here
eh = path[depth].p_hdr;
Normally, we protect extent tree with i_data_sem, and once we really
need drop i_data_sem, we should reload the ext4_ext_path array after we
recatch i_data_sem since extent tree may has changed, the 'again' in
ext4_ext_remove_space give us a sample. But the other case
ext4_ext_shift_path_extents seems forget to do this(ext4_access_path may
drop i_data_sem and recatch it with not enough credits), and will lead
the upper BUG when there is a parallel extents split which will grow the
extent tree.
Normally, the extent tree is protected by i_data_sem and if we drop
i_data_sem in ext4_datasem_ensure_credits(), we need to reload
ext4_ext_path array after reacquiring i_data_sem since the extent tree may
have changed. The 'again' label in ext4_ext_remove_space() is an example of
this. But ext4_ext_shift_path_extents() forgets to reload ext4_ext_path and
thus can cause the above mentioned BUG when there is a parallel extents
split which will grow the extent tree.
Fix it by introduce the again in ext4_ext_shift_extents.
Signed-off-by: yangerkun <yangerkun@xxxxxxxxxx>
---
fs/ext4/extents.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index a6fb0350f062..0aa14f6ca914 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5009,8 +5009,11 @@ ext4_ext_shift_path_extents(struct ext4_ext_path *path, ext4_lblk_t shift,
restart_credits = ext4_writepage_trans_blocks(inode);
err = ext4_datasem_ensure_credits(handle, inode, credits,
restart_credits, 0);
- if (err)
+ if (err) {
+ if (err > 0)
+ err = -EAGAIN;
goto out;
+ }
Hum, I'd note that the previous patch actually broke
ext4_ext_shift_path_extents() which could return 1 after patch 2/3 and
probably confuse code upwards in the stack and now you fix it up in this
patch. Can you perhaps fixup the previous patch by changing the condition
to:
if (err < 0)
and then change it here?
Thanks for your review! Ted has fix this by add some comments in patch 2/3!
err = ext4_ext_get_access(handle, inode, path + depth);
if (err)
@@ -5084,6 +5087,7 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
int ret = 0, depth;
struct ext4_extent *extent;
ext4_lblk_t stop, *iterator, ex_start, ex_end;
+ ext4_lblk_t tmp = EXT_MAX_BLOCKS;
Can you perhaps name this more descriptively than 'tmp'? Something like
restart_lblk or something like that?
Agree. I'll pay attention next time!
/* Let path point to the last extent */
path = ext4_find_extent(inode, EXT_MAX_BLOCKS - 1, NULL,
@@ -5137,11 +5141,15 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
* till we reach stop. In case of right shift, iterator points to stop
* and it is decreased till we reach start.
*/
+again:
if (SHIFT == SHIFT_LEFT)
iterator = &start;
else
iterator = &stop;
+ if (tmp != EXT_MAX_BLOCKS)
+ *iterator = tmp;
+
/*
* Its safe to start updating extents. Start and stop are unsigned, so
* in case of right shift if extent with 0 block is reached, iterator
@@ -5170,6 +5178,7 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
}
}
+ tmp = *iterator;
if (SHIFT == SHIFT_LEFT) {
extent = EXT_LAST_EXTENT(path[depth].p_hdr);
*iterator = le32_to_cpu(extent->ee_block) +
@@ -5188,6 +5197,9 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,
}
ret = ext4_ext_shift_path_extents(path, shift, inode,
handle, SHIFT);
+ /* iterator can be NULL which means we should break */
+ if (ret == -EAGAIN)
+ goto again;
Hum, but while we dropped i_data_sem, the extent depth may have increased
so we may need larger 'path' now?
Otherwise the patch looks good.
The ext4_find_extent in ext4_ext_shift_extents can handle this case. It
seems OK.
Honza
