From: Ye Bin <[email protected]>
There's issue as follows when do test with memory fault injection:
[localhost]# fsck.ext4 -a image
image: clean, 45595/655360 files, 466841/2621440 blocks
[localhost]# fsck.ext4 -fn image
Pass 1: Checking inodes, blocks, and sizes
Pass 2: Checking directory structure
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Pass 5: Checking group summary information
Block bitmap differences: -(1457230--1457256)
Fix? no
image: ********** WARNING: Filesystem still has errors **********
image: 45595/655360 files (12.4% non-contiguous), 466841/2621440 blocks
Inject context:
-----------------------------------------------------------
Inject function:kmem_cache_alloc (pid:177858) (return: 0)
Calltrace Context:
mem_cache_allock+0x73/0xcc
ext4_mb_new_blocks+0x32e/0x540 [ext4]
ext4_new_meta_blocks+0xc4/0x110 [ext4]
ext4_ext_grow_indepth+0x68/0x250 [ext4]
ext4_ext_create_new_leaf+0xc5/0x120 [ext4]
ext4_ext_insert_extent+0x1bf/0x670 [ext4]
ext4_split_extent_at+0x212/0x530 [ext4]
ext4_split_extent+0x13a/0x1a0 [ext4]
ext4_ext_handle_unwritten_extents+0x13d/0x240 [ext4]
ext4_ext_map_blocks+0x459/0x8f0 [ext4]
ext4_map_blocks+0x18e/0x5a0 [ext4]
ext4_iomap_alloc+0xb0/0x1b0 [ext4]
ext4_iomap_begin+0xb0/0x130 [ext4]
iomap_apply+0x95/0x2e0
__iomap_dio_rw+0x1cc/0x4b0
iomap_dio_rw+0xe/0x40
ext4_dio_write_iter+0x1a9/0x390 [ext4]
new_sync_write+0x113/0x1b0
vfs_write+0x1b7/0x250
ksys_write+0x5f/0xe0
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x61/0xc6
Compare extent change in journal:
Start:
ee_block ee_len ee_start
75 32798 1457227 -> unwritten len=30
308 12 434489
355 5 442492
=>
ee_block ee_len ee_start
11 2 951584
74 32769 951647 -> unwritten len=1
75 32771 1457227 -> unwritten len=3, length decreased 27
211 15 960906
308 12 434489
355 5 442492
Acctually, above issue can repaired by 'fsck -fa'. But file system is 'clean',
'fsck' will not do deep repair.
Obviously, final lost 27 blocks. Above issue may happens as follows:
ext4_split_extent_at
...
err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags); -> return -ENOMEM
if (err != -ENOSPC && err != -EDQUOT）
goto out; -> goto 'out' will not fix extent length, will
...
fix_extent_len:
ex->ee_len = orig_ex.ee_len;
/*
* Ignore ext4_ext_dirty return value since we are already in error path
* and err is a non-zero error code.
*/
ext4_ext_dirty(handle, inode, path + path->p_depth);
return err;
out:
ext4_ext_show_leaf(inode, path);
return err;
If 'ext4_ext_insert_extent' return '-ENOMEM' which will not fix 'ex->ee_len' by
old length. 'ext4_ext_insert_extent' will trigger extent tree merge, fix like
'ex->ee_len = orig_ex.ee_len' may lead to new issues.
To solve above issue, record error messages when 'ext4_ext_insert_extent' return
'err' not equal '(-ENOSPC && -EDQUOT)'. If filesysten is mounted with 'errors=continue'
as filesystem is not clean 'fsck' will repair issue. If filesystem is mounted with
'errors=remount-ro' filesystem will be remounted by read-only.
Signed-off-by: Ye Bin <[email protected]>
---
fs/ext4/extents.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index f1956288307f..582a7d59d6e3 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3252,8 +3252,13 @@ static int ext4_split_extent_at(handle_t *handle,
ext4_ext_mark_unwritten(ex2);
err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
- if (err != -ENOSPC && err != -EDQUOT)
+ if (err != -ENOSPC && err != -EDQUOT) {
+ if (err)
+ EXT4_ERROR_INODE_ERR(inode, -err,
+ "insert extent failed block = %d len = %d",
+ ex2->ee_block, ex2->ee_len);
goto out;
+ }
if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
if (split_flag & (EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
--
2.31.1
There have been a lot of problems here before, but the problem has not
been fundamentally solved.
On 2022/10/24 20:27, Ye Bin wrote:
> From: Ye Bin <[email protected]>
>
> There's issue as follows when do test with memory fault injection:
> [localhost]# fsck.ext4 -a image
> image: clean, 45595/655360 files, 466841/2621440 blocks
> [localhost]# fsck.ext4 -fn image
> Pass 1: Checking inodes, blocks, and sizes
> Pass 2: Checking directory structure
> Pass 3: Checking directory connectivity
> Pass 4: Checking reference counts
> Pass 5: Checking group summary information
> Block bitmap differences: -(1457230--1457256)
> Fix? no
>
> image: ********** WARNING: Filesystem still has errors **********
>
> image: 45595/655360 files (12.4% non-contiguous), 466841/2621440 blocks
>
> Inject context:
> -----------------------------------------------------------
> Inject function:kmem_cache_alloc (pid:177858) (return: 0)
> Calltrace Context:
> mem_cache_allock+0x73/0xcc
> ext4_mb_new_blocks+0x32e/0x540 [ext4]
> ext4_new_meta_blocks+0xc4/0x110 [ext4]
> ext4_ext_grow_indepth+0x68/0x250 [ext4]
> ext4_ext_create_new_leaf+0xc5/0x120 [ext4]
> ext4_ext_insert_extent+0x1bf/0x670 [ext4]
> ext4_split_extent_at+0x212/0x530 [ext4]
> ext4_split_extent+0x13a/0x1a0 [ext4]
> ext4_ext_handle_unwritten_extents+0x13d/0x240 [ext4]
> ext4_ext_map_blocks+0x459/0x8f0 [ext4]
> ext4_map_blocks+0x18e/0x5a0 [ext4]
> ext4_iomap_alloc+0xb0/0x1b0 [ext4]
> ext4_iomap_begin+0xb0/0x130 [ext4]
> iomap_apply+0x95/0x2e0
> __iomap_dio_rw+0x1cc/0x4b0
> iomap_dio_rw+0xe/0x40
> ext4_dio_write_iter+0x1a9/0x390 [ext4]
> new_sync_write+0x113/0x1b0
> vfs_write+0x1b7/0x250
> ksys_write+0x5f/0xe0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x61/0xc6
>
> Compare extent change in journal:
> Start:
> ee_block ee_len ee_start
> 75 32798 1457227 -> unwritten len=30
> 308 12 434489
> 355 5 442492
> =>
> ee_block ee_len ee_start
> 11 2 951584
> 74 32769 951647 -> unwritten len=1
> 75 32771 1457227 -> unwritten len=3, length decreased 27
> 211 15 960906
> 308 12 434489
> 355 5 442492
>
> Acctually, above issue can repaired by 'fsck -fa'. But file system is 'clean',
> 'fsck' will not do deep repair.
> Obviously, final lost 27 blocks. Above issue may happens as follows:
> ext4_split_extent_at
> ...
> err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags); -> return -ENOMEM
> if (err != -ENOSPC && err != -EDQUOT）
> goto out; -> goto 'out' will not fix extent length, will
> ...
> fix_extent_len:
> ex->ee_len = orig_ex.ee_len;
> /*
> * Ignore ext4_ext_dirty return value since we are already in error path
> * and err is a non-zero error code.
> */
> ext4_ext_dirty(handle, inode, path + path->p_depth);
> return err;
> out:
> ext4_ext_show_leaf(inode, path);
> return err;
> If 'ext4_ext_insert_extent' return '-ENOMEM' which will not fix 'ex->ee_len' by
> old length. 'ext4_ext_insert_extent' will trigger extent tree merge, fix like
> 'ex->ee_len = orig_ex.ee_len' may lead to new issues.
> To solve above issue, record error messages when 'ext4_ext_insert_extent' return
> 'err' not equal '(-ENOSPC && -EDQUOT)'. If filesysten is mounted with 'errors=continue'
> as filesystem is not clean 'fsck' will repair issue. If filesystem is mounted with
> 'errors=remount-ro' filesystem will be remounted by read-only.
>
> Signed-off-by: Ye Bin <[email protected]>
> ---
> fs/ext4/extents.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index f1956288307f..582a7d59d6e3 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3252,8 +3252,13 @@ static int ext4_split_extent_at(handle_t *handle,
> ext4_ext_mark_unwritten(ex2);
>
> err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
> - if (err != -ENOSPC && err != -EDQUOT)
> + if (err != -ENOSPC && err != -EDQUOT) {
> + if (err)
> + EXT4_ERROR_INODE_ERR(inode, -err,
> + "insert extent failed block = %d len = %d",
> + ex2->ee_block, ex2->ee_len);
> goto out;
> + }
>
> if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> if (split_flag & (EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
>
On Fri, Oct 28, 2022 at 09:41:55AM +0800, zhanchengbin wrote:
> There have been a lot of problems here before, but the problem has not been
> fundamentally solved.
Indeed, this only papers only the problem. The commit description is
pretty good, in that it describes the the root cause of the problem:
> > If 'ext4_ext_insert_extent' return '-ENOMEM' which will not fix 'ex->ee_len' by
> > old length. 'ext4_ext_insert_extent' will trigger extent tree merge, fix like
> > 'ex->ee_len = orig_ex.ee_len' may lead to new issues.
> > To solve above issue, record error messages when 'ext4_ext_insert_extent' return
> > 'err' not equal '(-ENOSPC && -EDQUOT)'. If filesysten is mounted with 'errors=continue'
> > as filesystem is not clean 'fsck' will repair issue. If filesystem is mounted with
> > 'errors=remount-ro' filesystem will be remounted by read-only.
What should actually happen here is that we undo the change in orig_ex
if ext4_ext_insert_extent fails. As you point out, in an earlier part
of the code path, this gets handled by a "goto fix_extent_len".
The problem is that it's possible that the shape of the extent tree
may have changed before ext4_insert_extent() fails with the ENOMEM.
So the simplistic fix of just jumping to fix_extent_len isn't going to
work. But fixing it by much marking the file system is corrupt is a
bit of a cop-out. Consider that if you were running ext4 in a Huawei
Cloud, and you run into the memory allocation failure, what would you
prefer? For the individual system call to fail, and propagating the
failure to userspace? Or to leave the file system corrupted? (And
then either forcing a reboot so the file system to be fixed, or allow
the system to stumble along, with further unknown unexpected behaviour
from userspace?)
The real correct fix is that ext4_ext_insert_extent() needs to
rollback any changes to the extent tree that was made, *or* it needs
to make sure that the operation will succeed before starting to make
any changes, *or* we need to look up the orig_extent via
orig_ex->ee_block, and then undo the change.
It might be that we can't always reliably rollback the change, or we
might think that the operation will succeed, but then it fail due to
an I/O error. If it's due to an I/O error, then it's fine to bail and
mark the file system as corrupted. But if the failure is caused by an
ENOMEM, we should be able to handle this case more gracefully.
Can you look into a better fix?
Thanks,
- Ted