Inside the ext4_ext_insert_extent function, every error returned will
not destroy the consistency of the tree. Even if it fails after changing
half of the tree, can also ensure that the tree is self-consistent, like
function ext4_ext_create_new_leaf.
After ext4_ext_insert_extent fails, update extent status tree depends on
the incoming split_flag. So restore the len of extent to be split when
ext4_ext_insert_extent return failed in ext4_split_extent_at.
Diff v2 Vs v1:
1) return directly after inserting successfully
2) restore the length of extent in memory after inserting unsuccessfully
Diff v3 Vs v2:
Sorry for not taking into account the successful extent insertion. and I
reanalyzed the ext4_ext_insert_extent function and modified the conditions
for restoring the length.
Diff v4 Vs v3:
Clear the verified flag from the modified bh when failed inext4_ext_rm_idx
or ext4_ext_correct_indexes.
zhanchengbin (2):
ext4: fix inode tree inconsistency caused by ENOMEM in
ext4_split_extent_at
ext4: clear the verified flag of the modified leaf or idx if error
fs/ext4/extents.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
--
2.31.1
If ENOMEM fails when the extent is splitting, we need to restore the length
of the split extent.
In the call stack of the ext4_split_extent_at function, only in
ext4_ext_create_new_leaf will it alloc memory and change the shape of the
extent tree,even if an ENOMEM is returned at this time, the extent tree is
still self-consistent, Just restore the split extent lens in the function
ext4_split_extent_at.
ext4_split_extent_at
ext4_ext_insert_extent
ext4_ext_create_new_leaf
1)ext4_ext_split
ext4_find_extent
2)ext4_ext_grow_indepth
ext4_find_extent
Signed-off-by: zhanchengbin <[email protected]>
---
fs/ext4/extents.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 9de1c9d1a13d..0f95e857089e 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
if (IS_ERR(bh)) {
+ EXT4_ERROR_INODE(inode, "IO error reading extent block");
ret = PTR_ERR(bh);
goto err;
}
@@ -3251,7 +3252,7 @@ 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 && err != -ENOMEM)
goto out;
if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
--
2.31.1
Clear the verified flag from the modified bh when failed in ext4_ext_rm_idx
or ext4_ext_correct_indexes.
In this way, the start value of the logical block itself and its
parents' will be checked in ext4_valid_extent_entries.
Signed-off-by: zhanchengbin <[email protected]>
---
fs/ext4/extents.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 0f95e857089e..9013a05f524b 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1756,6 +1756,8 @@ static int ext4_ext_correct_indexes(handle_t *handle, struct inode *inode,
if (err)
break;
}
+ while (!(k < 0) && k++ < depth)
+ clear_buffer_verified(path[k]->p_bh);
return err;
}
@@ -2304,6 +2306,7 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
{
int err;
ext4_fsblk_t leaf;
+ int b_depth = depth;
/* free index block */
depth--;
@@ -2345,6 +2348,9 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
if (err)
break;
}
+ while (!(depth < 0) && depth++ < b_depth - 1)
+ clear_buffer_verified(path[depth]->p_bh);
+
return err;
}
--
2.31.1
Hi zhanchengbin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on jack-fs/for_next linus/master v6.2-rc8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/zhanchengbin/ext4-fix-inode-tree-inconsistency-caused-by-ENOMEM-in-ext4_split_extent_at/20230213-114334
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/r/20230213040522.3339406-3-zhanchengbin1%40huawei.com
patch subject: [PATCH v4 2/2] ext4: clear the verified flag of the modified leaf or idx if error
config: arm-randconfig-r024-20230213 (https://download.01.org/0day-ci/archive/20230213/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project db0e6591612b53910a1b366863348bdb9d7d2fb1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/c6de5d67952addd5ffa288574ed55ebe7aeba755
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review zhanchengbin/ext4-fix-inode-tree-inconsistency-caused-by-ENOMEM-in-ext4_split_extent_at/20230213-114334
git checkout c6de5d67952addd5ffa288574ed55ebe7aeba755
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash fs/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> fs/ext4/extents.c:1760:32: error: member reference type 'struct ext4_ext_path' is not a pointer; did you mean to use '.'?
clear_buffer_verified(path[k]->p_bh);
~~~~~~~^~
.
fs/ext4/extents.c:2352:36: error: member reference type 'struct ext4_ext_path' is not a pointer; did you mean to use '.'?
clear_buffer_verified(path[depth]->p_bh);
~~~~~~~~~~~^~
.
2 errors generated.
vim +1760 fs/ext4/extents.c
1699
1700 /*
1701 * ext4_ext_correct_indexes:
1702 * if leaf gets modified and modified extent is first in the leaf,
1703 * then we have to correct all indexes above.
1704 * TODO: do we need to correct tree in all cases?
1705 */
1706 static int ext4_ext_correct_indexes(handle_t *handle, struct inode *inode,
1707 struct ext4_ext_path *path)
1708 {
1709 struct ext4_extent_header *eh;
1710 int depth = ext_depth(inode);
1711 struct ext4_extent *ex;
1712 __le32 border;
1713 int k, err = 0;
1714
1715 eh = path[depth].p_hdr;
1716 ex = path[depth].p_ext;
1717
1718 if (unlikely(ex == NULL || eh == NULL)) {
1719 EXT4_ERROR_INODE(inode,
1720 "ex %p == NULL or eh %p == NULL", ex, eh);
1721 return -EFSCORRUPTED;
1722 }
1723
1724 if (depth == 0) {
1725 /* there is no tree at all */
1726 return 0;
1727 }
1728
1729 if (ex != EXT_FIRST_EXTENT(eh)) {
1730 /* we correct tree if first leaf got modified only */
1731 return 0;
1732 }
1733
1734 /*
1735 * TODO: we need correction if border is smaller than current one
1736 */
1737 k = depth - 1;
1738 border = path[depth].p_ext->ee_block;
1739 err = ext4_ext_get_access(handle, inode, path + k);
1740 if (err)
1741 return err;
1742 path[k].p_idx->ei_block = border;
1743 err = ext4_ext_dirty(handle, inode, path + k);
1744 if (err)
1745 return err;
1746
1747 while (k--) {
1748 /* change all left-side indexes */
1749 if (path[k+1].p_idx != EXT_FIRST_INDEX(path[k+1].p_hdr))
1750 break;
1751 err = ext4_ext_get_access(handle, inode, path + k);
1752 if (err)
1753 break;
1754 path[k].p_idx->ei_block = border;
1755 err = ext4_ext_dirty(handle, inode, path + k);
1756 if (err)
1757 break;
1758 }
1759 while (!(k < 0) && k++ < depth)
> 1760 clear_buffer_verified(path[k]->p_bh);
1761
1762 return err;
1763 }
1764
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi zhanchengbin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on jack-fs/for_next linus/master v6.2-rc8 next-20230210]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/zhanchengbin/ext4-fix-inode-tree-inconsistency-caused-by-ENOMEM-in-ext4_split_extent_at/20230213-114334
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/r/20230213040522.3339406-3-zhanchengbin1%40huawei.com
patch subject: [PATCH v4 2/2] ext4: clear the verified flag of the modified leaf or idx if error
config: i386-randconfig-a004-20230213 (https://download.01.org/0day-ci/archive/20230213/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c6de5d67952addd5ffa288574ed55ebe7aeba755
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review zhanchengbin/ext4-fix-inode-tree-inconsistency-caused-by-ENOMEM-in-ext4_split_extent_at/20230213-114334
git checkout c6de5d67952addd5ffa288574ed55ebe7aeba755
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/ext4/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> fs/ext4/extents.c:1760:32: error: member reference type 'struct ext4_ext_path' is not a pointer; did you mean to use '.'?
clear_buffer_verified(path[k]->p_bh);
~~~~~~~^~
.
fs/ext4/extents.c:2352:36: error: member reference type 'struct ext4_ext_path' is not a pointer; did you mean to use '.'?
clear_buffer_verified(path[depth]->p_bh);
~~~~~~~~~~~^~
.
2 errors generated.
vim +1760 fs/ext4/extents.c
1699
1700 /*
1701 * ext4_ext_correct_indexes:
1702 * if leaf gets modified and modified extent is first in the leaf,
1703 * then we have to correct all indexes above.
1704 * TODO: do we need to correct tree in all cases?
1705 */
1706 static int ext4_ext_correct_indexes(handle_t *handle, struct inode *inode,
1707 struct ext4_ext_path *path)
1708 {
1709 struct ext4_extent_header *eh;
1710 int depth = ext_depth(inode);
1711 struct ext4_extent *ex;
1712 __le32 border;
1713 int k, err = 0;
1714
1715 eh = path[depth].p_hdr;
1716 ex = path[depth].p_ext;
1717
1718 if (unlikely(ex == NULL || eh == NULL)) {
1719 EXT4_ERROR_INODE(inode,
1720 "ex %p == NULL or eh %p == NULL", ex, eh);
1721 return -EFSCORRUPTED;
1722 }
1723
1724 if (depth == 0) {
1725 /* there is no tree at all */
1726 return 0;
1727 }
1728
1729 if (ex != EXT_FIRST_EXTENT(eh)) {
1730 /* we correct tree if first leaf got modified only */
1731 return 0;
1732 }
1733
1734 /*
1735 * TODO: we need correction if border is smaller than current one
1736 */
1737 k = depth - 1;
1738 border = path[depth].p_ext->ee_block;
1739 err = ext4_ext_get_access(handle, inode, path + k);
1740 if (err)
1741 return err;
1742 path[k].p_idx->ei_block = border;
1743 err = ext4_ext_dirty(handle, inode, path + k);
1744 if (err)
1745 return err;
1746
1747 while (k--) {
1748 /* change all left-side indexes */
1749 if (path[k+1].p_idx != EXT_FIRST_INDEX(path[k+1].p_hdr))
1750 break;
1751 err = ext4_ext_get_access(handle, inode, path + k);
1752 if (err)
1753 break;
1754 path[k].p_idx->ei_block = border;
1755 err = ext4_ext_dirty(handle, inode, path + k);
1756 if (err)
1757 break;
1758 }
1759 while (!(k < 0) && k++ < depth)
> 1760 clear_buffer_verified(path[k]->p_bh);
1761
1762 return err;
1763 }
1764
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Mon 13-02-23 12:05:21, zhanchengbin wrote:
> If ENOMEM fails when the extent is splitting, we need to restore the length
> of the split extent.
> In the call stack of the ext4_split_extent_at function, only in
> ext4_ext_create_new_leaf will it alloc memory and change the shape of the
> extent tree,even if an ENOMEM is returned at this time, the extent tree is
> still self-consistent, Just restore the split extent lens in the function
> ext4_split_extent_at.
>
> ext4_split_extent_at
> ext4_ext_insert_extent
> ext4_ext_create_new_leaf
> 1)ext4_ext_split
> ext4_find_extent
> 2)ext4_ext_grow_indepth
> ext4_find_extent
>
> Signed-off-by: zhanchengbin <[email protected]>
> ---
> fs/ext4/extents.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 9de1c9d1a13d..0f95e857089e 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>
> bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
> if (IS_ERR(bh)) {
> + EXT4_ERROR_INODE(inode, "IO error reading extent block");
Why have you added this? Usually we don't log any additional errors for IO
errors because the storage layer already reports it... Furthermore this
would potentialy panic the system / remount the fs RO which we also usually
don't do in case of IO errors, only in case of FS corruption.
Honza
> ret = PTR_ERR(bh);
> goto err;
> }
> @@ -3251,7 +3252,7 @@ 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 && err != -ENOMEM)
> goto out;
>
> if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> --
> 2.31.1
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2023/2/14 19:48, Jan Kara wrote:
> On Mon 13-02-23 12:05:21, zhanchengbin wrote:
>> If ENOMEM fails when the extent is splitting, we need to restore the length
>> of the split extent.
>> In the call stack of the ext4_split_extent_at function, only in
>> ext4_ext_create_new_leaf will it alloc memory and change the shape of the
>> extent tree,even if an ENOMEM is returned at this time, the extent tree is
>> still self-consistent, Just restore the split extent lens in the function
>> ext4_split_extent_at.
>>
>> ext4_split_extent_at
>> ext4_ext_insert_extent
>> ext4_ext_create_new_leaf
>> 1)ext4_ext_split
>> ext4_find_extent
>> 2)ext4_ext_grow_indepth
>> ext4_find_extent
>>
>> Signed-off-by: zhanchengbin <[email protected]>
>> ---
>> fs/ext4/extents.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 9de1c9d1a13d..0f95e857089e 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
>>
>> bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
>> if (IS_ERR(bh)) {
>> + EXT4_ERROR_INODE(inode, "IO error reading extent block");
>
> Why have you added this? Usually we don't log any additional errors for IO
> errors because the storage layer already reports it... Furthermore this
> would potentialy panic the system / remount the fs RO which we also usually
> don't do in case of IO errors, only in case of FS corruption.
>
> Honza
Because failure of read_extent_tree_block indirectly leads to filesystem
inconsistency in ext4_split_extent_at, I want the filesystem to become
read-only after failure.
- bin.
>
>> ret = PTR_ERR(bh);
>> goto err;
>> }
>> @@ -3251,7 +3252,7 @@ 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 && err != -ENOMEM)
>> goto out;
>>
>> if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
>> --
>> 2.31.1
>>
On Wed 15-02-23 16:51:23, zhanchengbin wrote:
>
> On 2023/2/14 19:48, Jan Kara wrote:
> > On Mon 13-02-23 12:05:21, zhanchengbin wrote:
> > > If ENOMEM fails when the extent is splitting, we need to restore the length
> > > of the split extent.
> > > In the call stack of the ext4_split_extent_at function, only in
> > > ext4_ext_create_new_leaf will it alloc memory and change the shape of the
> > > extent tree,even if an ENOMEM is returned at this time, the extent tree is
> > > still self-consistent, Just restore the split extent lens in the function
> > > ext4_split_extent_at.
> > >
> > > ext4_split_extent_at
> > > ext4_ext_insert_extent
> > > ext4_ext_create_new_leaf
> > > 1)ext4_ext_split
> > > ext4_find_extent
> > > 2)ext4_ext_grow_indepth
> > > ext4_find_extent
> > >
> > > Signed-off-by: zhanchengbin <[email protected]>
> > > ---
> > > fs/ext4/extents.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > > index 9de1c9d1a13d..0f95e857089e 100644
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
> > > bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
> > > if (IS_ERR(bh)) {
> > > + EXT4_ERROR_INODE(inode, "IO error reading extent block");
> >
> > Why have you added this? Usually we don't log any additional errors for IO
> > errors because the storage layer already reports it... Furthermore this
> > would potentialy panic the system / remount the fs RO which we also usually
> > don't do in case of IO errors, only in case of FS corruption.
> >
> > Honza
>
> Because failure of read_extent_tree_block indirectly leads to filesystem
> inconsistency in ext4_split_extent_at, I want the filesystem to become
> read-only after failure.
Can you please describe how exactly? Because I'd rather declare the error
directly in ext4_split_extent_at() than in ext4_find_extent() unless it
gets too complicated...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, Feb 15, 2023 at 04:51:23PM +0800, zhanchengbin wrote:
> > >
> > >
>
> Because failure of read_extent_tree_block indirectly leads to filesystem
> inconsistency in ext4_split_extent_at, I want the filesystem to become
> read-only after failure.
If I understand your concern correctly, the problem you're trying to
solve is that in ext4_ext_create_new_leaf() we can't easily unwind the
file system mutation in process if ext4_find_extent() fails here:
> > > ext4_split_extent_at
> > > ext4_ext_insert_extent
> > > ext4_ext_create_new_leaf
> > > 1)ext4_ext_split
> > > ext4_find_extent
> > > 2)ext4_ext_grow_indepth
> > > ext4_find_extent <=======
Is that your concern?
If so, it seems to me that there are two reasons why
ext4_find_extent() could fail. The first is that it could be because
there is a memory allocation failure. The second is that there is an
I/O error when it actually tries to read the extent block.
The memory allocation failure case can be solved by passing in
EXT4_EX_NOFAIL to ext4_find_extent() in those cases where we can't
back out safely, and that certainly includes the this code path.
As far as the I/O failure, we shouldn't be forcing a file system error
in ext4_find_extent(), as you have in this patch:
> > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > > index 9de1c9d1a13d..0f95e857089e 100644
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -935,6 +935,7 @@ ext4_find_extent(struct inode *inode, ext4_lblk_t block,
> > > bh = read_extent_tree_block(inode, path[ppos].p_idx, --i, flags);
> > > if (IS_ERR(bh)) {
> > > + EXT4_ERROR_INODE(inode, "IO error reading extent block");
The reason for that is in the case where we are *not* modifying the
file system, and the I/O error is a transient one (for example, maybe
there is a network hiccup in an iSCSI or Fibre Channel attached disk)
we do *not* want to mark the file system as corrupted.
Now, if the *caller* of ext4_find_extent() is in the middle of making
a change to the file system, and we can't easily back out, at that
point, it's totaly fair to mark the file system as inconsistent. In
the ideal world, we'd try to figure out a way to pre-read in the
necessary bloccks before starting the file system mutation, to reduce
the chances of failing in the middle of the update operation.
Of course, the world is not perfect, and case where we are splitting a
leaf node, and it turns out we need to grow the depth of the tree is a
relatively rare case, and if it turns out we have an unlucky read
operation right when this happens, if we need to stop the system by
calling EXT4_ERROR*, I'm OK with that. But we should *only* be doing
this particular case, and not in other cases when we might be calling
ext4_find_extent() is a read-only operation (for example, while
looking up a logical to physical block assignment). After, all the
*vast* majority of calls to ext4_find_extent() will be in read-only
contexts, and so calling EXT4_ERROR_INODE() any time
read_extent_tree_block() might fail is not appropriate.
Does that make sense to you?
Cheers,
- Ted