We find a checksum error and a inode corruption problem while doing
stress test, this 6 patches address to fix them. The first patch is
relate to the error simulation, and the 2-5 patches are prepare to
do the fix. The last patch fix these two issue.
- Checksum error
EXT4-fs error (device sda): ext4_lookup:1784: inode #131074: comm cat: iget: checksum invalid
- Inode corruption
e2fsck 1.46.0 (29-Jan-2020)
Pass 1: Checking inodes, blocks, and sizes
Pass 2: Checking directory structure
Entry 'foo' in / (2) has deleted/unused inode 17. Clear<y>? yes
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Pass 5: Checking group summary information
Inode bitmap differences: -17
Fix<y>? yes
Free inodes count wrong for group #0 (32750, counted=32751).
Fix<y>? yes
Free inodes count wrong (32750, counted=32751).
Fix<y>? yes
Changes since v3:
- Postpone initialization to ext4_do_update_inode() may cause zeroout
newly set xattr entry. So switch to do initialization in
__ext4_get_inode_loc().
Changes since v2:
- Instead of using WARN_ON_ONCE to prevent ext4_do_update_inode()
return before filling the inode buffer, keep the error and postpone
the report after the updating in the third patch.
- Fix some language mistacks in the last patch.
Changes since v1:
- Add a patch to prevent ext4_do_update_inode() return before filling
the inode buffer.
- Do not use BH_New flag to indicate the empty buffer, postpone the
zero and uptodate logic into ext4_do_update_inode() before filling
the inode buffer.
Thanks,
Yi.
Zhang Yi (6):
ext4: move inode eio simulation behind io completeion
ext4: remove an unnecessary if statement in __ext4_get_inode_loc()
ext4: make the updating inode data procedure atomic
ext4: factor out ext4_fill_raw_inode()
ext4: move ext4_fill_raw_inode() related functions
ext4: prevent getting empty inode buffer
fs/ext4/inode.c | 490 +++++++++++++++++++++++++-----------------------
1 file changed, 257 insertions(+), 233 deletions(-)
--
2.31.1
The "if (!buffer_uptodate(bh))" hunk covered almost the whole code after
getting buffer in __ext4_get_inode_loc() which seems unnecessary, remove
it and switch to check ext4_buffer_uptodate(), it simplify code and make
it more readable.
Signed-off-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/ext4/inode.c | 162 +++++++++++++++++++++++-------------------------
1 file changed, 78 insertions(+), 84 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index eb2526a35254..eae1b2d0b550 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4330,99 +4330,93 @@ static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,
bh = sb_getblk(sb, block);
if (unlikely(!bh))
return -ENOMEM;
- if (!buffer_uptodate(bh)) {
- lock_buffer(bh);
-
- if (ext4_buffer_uptodate(bh)) {
- /* someone brought it uptodate while we waited */
- unlock_buffer(bh);
- goto has_buffer;
- }
+ if (ext4_buffer_uptodate(bh))
+ goto has_buffer;
- /*
- * If we have all information of the inode in memory and this
- * is the only valid inode in the block, we need not read the
- * block.
- */
- if (in_mem) {
- struct buffer_head *bitmap_bh;
- int i, start;
+ lock_buffer(bh);
+ /*
+ * If we have all information of the inode in memory and this
+ * is the only valid inode in the block, we need not read the
+ * block.
+ */
+ if (in_mem) {
+ struct buffer_head *bitmap_bh;
+ int i, start;
- start = inode_offset & ~(inodes_per_block - 1);
+ start = inode_offset & ~(inodes_per_block - 1);
- /* Is the inode bitmap in cache? */
- bitmap_bh = sb_getblk(sb, ext4_inode_bitmap(sb, gdp));
- if (unlikely(!bitmap_bh))
- goto make_io;
+ /* Is the inode bitmap in cache? */
+ bitmap_bh = sb_getblk(sb, ext4_inode_bitmap(sb, gdp));
+ if (unlikely(!bitmap_bh))
+ goto make_io;
- /*
- * If the inode bitmap isn't in cache then the
- * optimisation may end up performing two reads instead
- * of one, so skip it.
- */
- if (!buffer_uptodate(bitmap_bh)) {
- brelse(bitmap_bh);
- goto make_io;
- }
- for (i = start; i < start + inodes_per_block; i++) {
- if (i == inode_offset)
- continue;
- if (ext4_test_bit(i, bitmap_bh->b_data))
- break;
- }
+ /*
+ * If the inode bitmap isn't in cache then the
+ * optimisation may end up performing two reads instead
+ * of one, so skip it.
+ */
+ if (!buffer_uptodate(bitmap_bh)) {
brelse(bitmap_bh);
- if (i == start + inodes_per_block) {
- /* all other inodes are free, so skip I/O */
- memset(bh->b_data, 0, bh->b_size);
- set_buffer_uptodate(bh);
- unlock_buffer(bh);
- goto has_buffer;
- }
+ goto make_io;
+ }
+ for (i = start; i < start + inodes_per_block; i++) {
+ if (i == inode_offset)
+ continue;
+ if (ext4_test_bit(i, bitmap_bh->b_data))
+ break;
}
+ brelse(bitmap_bh);
+ if (i == start + inodes_per_block) {
+ /* all other inodes are free, so skip I/O */
+ memset(bh->b_data, 0, bh->b_size);
+ set_buffer_uptodate(bh);
+ unlock_buffer(bh);
+ goto has_buffer;
+ }
+ }
make_io:
- /*
- * If we need to do any I/O, try to pre-readahead extra
- * blocks from the inode table.
- */
- blk_start_plug(&plug);
- if (EXT4_SB(sb)->s_inode_readahead_blks) {
- ext4_fsblk_t b, end, table;
- unsigned num;
- __u32 ra_blks = EXT4_SB(sb)->s_inode_readahead_blks;
-
- table = ext4_inode_table(sb, gdp);
- /* s_inode_readahead_blks is always a power of 2 */
- b = block & ~((ext4_fsblk_t) ra_blks - 1);
- if (table > b)
- b = table;
- end = b + ra_blks;
- num = EXT4_INODES_PER_GROUP(sb);
- if (ext4_has_group_desc_csum(sb))
- num -= ext4_itable_unused_count(sb, gdp);
- table += num / inodes_per_block;
- if (end > table)
- end = table;
- while (b <= end)
- ext4_sb_breadahead_unmovable(sb, b++);
- }
+ /*
+ * If we need to do any I/O, try to pre-readahead extra
+ * blocks from the inode table.
+ */
+ blk_start_plug(&plug);
+ if (EXT4_SB(sb)->s_inode_readahead_blks) {
+ ext4_fsblk_t b, end, table;
+ unsigned num;
+ __u32 ra_blks = EXT4_SB(sb)->s_inode_readahead_blks;
+
+ table = ext4_inode_table(sb, gdp);
+ /* s_inode_readahead_blks is always a power of 2 */
+ b = block & ~((ext4_fsblk_t) ra_blks - 1);
+ if (table > b)
+ b = table;
+ end = b + ra_blks;
+ num = EXT4_INODES_PER_GROUP(sb);
+ if (ext4_has_group_desc_csum(sb))
+ num -= ext4_itable_unused_count(sb, gdp);
+ table += num / inodes_per_block;
+ if (end > table)
+ end = table;
+ while (b <= end)
+ ext4_sb_breadahead_unmovable(sb, b++);
+ }
- /*
- * There are other valid inodes in the buffer, this inode
- * has in-inode xattrs, or we don't have this inode in memory.
- * Read the block from disk.
- */
- trace_ext4_load_inode(sb, ino);
- ext4_read_bh_nowait(bh, REQ_META | REQ_PRIO, NULL);
- blk_finish_plug(&plug);
- wait_on_buffer(bh);
- ext4_simulate_fail_bh(sb, bh, EXT4_SIM_INODE_EIO);
- if (!buffer_uptodate(bh)) {
- if (ret_block)
- *ret_block = block;
- brelse(bh);
- return -EIO;
- }
+ /*
+ * There are other valid inodes in the buffer, this inode
+ * has in-inode xattrs, or we don't have this inode in memory.
+ * Read the block from disk.
+ */
+ trace_ext4_load_inode(sb, ino);
+ ext4_read_bh_nowait(bh, REQ_META | REQ_PRIO, NULL);
+ blk_finish_plug(&plug);
+ wait_on_buffer(bh);
+ ext4_simulate_fail_bh(sb, bh, EXT4_SIM_INODE_EIO);
+ if (!buffer_uptodate(bh)) {
+ if (ret_block)
+ *ret_block = block;
+ brelse(bh);
+ return -EIO;
}
has_buffer:
iloc->bh = bh;
--
2.31.1
Now that ext4_do_update_inode() return error before filling the whole
inode data if we fail to set inode blocks in ext4_inode_blocks_set().
This error should never happen in theory since sb->s_maxbytes should not
have allowed this, we have already init sb->s_maxbytes according to this
feature in ext4_fill_super(). So even through that could only happen due
to the filesystem corruption, we'd better to return after we finish
updating the inode because it may left an uninitialized buffer and we
could read this buffer later in "errors=continue" mode.
This patch make the updating inode data procedure atomic, call
EXT4_ERROR_INODE() after we dropping i_raw_lock after something bad
happened, make sure that the inode is integrated, and also drop a BUG_ON
and do some small cleanups.
Signed-off-by: Zhang Yi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
fs/ext4/inode.c | 44 ++++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index eae1b2d0b550..8323d3e8f393 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4920,8 +4920,14 @@ static int ext4_inode_blocks_set(handle_t *handle,
ext4_clear_inode_flag(inode, EXT4_INODE_HUGE_FILE);
return 0;
}
+
+ /*
+ * This should never happen since sb->s_maxbytes should not have
+ * allowed this, sb->s_maxbytes was set according to the huge_file
+ * feature in ext4_fill_super().
+ */
if (!ext4_has_feature_huge_file(sb))
- return -EFBIG;
+ return -EFSCORRUPTED;
if (i_blocks <= 0xffffffffffffULL) {
/*
@@ -5024,16 +5030,14 @@ static int ext4_do_update_inode(handle_t *handle,
spin_lock(&ei->i_raw_lock);
- /* For fields not tracked in the in-memory inode,
- * initialise them to zero for new inodes. */
+ /*
+ * For fields not tracked in the in-memory inode, initialise them
+ * to zero for new inodes.
+ */
if (ext4_test_inode_state(inode, EXT4_STATE_NEW))
memset(raw_inode, 0, EXT4_SB(inode->i_sb)->s_inode_size);
err = ext4_inode_blocks_set(handle, raw_inode, ei);
- if (err) {
- spin_unlock(&ei->i_raw_lock);
- goto out_brelse;
- }
raw_inode->i_mode = cpu_to_le16(inode->i_mode);
i_uid = i_uid_read(inode);
@@ -5042,10 +5046,11 @@ static int ext4_do_update_inode(handle_t *handle,
if (!(test_opt(inode->i_sb, NO_UID32))) {
raw_inode->i_uid_low = cpu_to_le16(low_16_bits(i_uid));
raw_inode->i_gid_low = cpu_to_le16(low_16_bits(i_gid));
-/*
- * Fix up interoperability with old kernels. Otherwise, old inodes get
- * re-used with the upper 16 bits of the uid/gid intact
- */
+ /*
+ * Fix up interoperability with old kernels. Otherwise,
+ * old inodes get re-used with the upper 16 bits of the
+ * uid/gid intact.
+ */
if (ei->i_dtime && list_empty(&ei->i_orphan)) {
raw_inode->i_uid_high = 0;
raw_inode->i_gid_high = 0;
@@ -5114,8 +5119,9 @@ static int ext4_do_update_inode(handle_t *handle,
}
}
- BUG_ON(!ext4_has_feature_project(inode->i_sb) &&
- i_projid != EXT4_DEF_PROJID);
+ if (i_projid != EXT4_DEF_PROJID &&
+ !ext4_has_feature_project(inode->i_sb))
+ err = err ?: -EFSCORRUPTED;
if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE &&
EXT4_FITS_IN_INODE(raw_inode, ei, i_projid))
@@ -5123,6 +5129,11 @@ static int ext4_do_update_inode(handle_t *handle,
ext4_inode_csum_set(inode, raw_inode, ei);
spin_unlock(&ei->i_raw_lock);
+ if (err) {
+ EXT4_ERROR_INODE(inode, "corrupted inode contents");
+ goto out_brelse;
+ }
+
if (inode->i_sb->s_flags & SB_LAZYTIME)
ext4_update_other_inodes_time(inode->i_sb, inode->i_ino,
bh->b_data);
@@ -5130,13 +5141,13 @@ static int ext4_do_update_inode(handle_t *handle,
BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");
err = ext4_handle_dirty_metadata(handle, NULL, bh);
if (err)
- goto out_brelse;
+ goto out_error;
ext4_clear_inode_state(inode, EXT4_STATE_NEW);
if (set_large_file) {
BUFFER_TRACE(EXT4_SB(sb)->s_sbh, "get write access");
err = ext4_journal_get_write_access(handle, EXT4_SB(sb)->s_sbh);
if (err)
- goto out_brelse;
+ goto out_error;
lock_buffer(EXT4_SB(sb)->s_sbh);
ext4_set_feature_large_file(sb);
ext4_superblock_csum_set(sb);
@@ -5146,9 +5157,10 @@ static int ext4_do_update_inode(handle_t *handle,
EXT4_SB(sb)->s_sbh);
}
ext4_update_inode_fsync_trans(handle, inode, need_datasync);
+out_error:
+ ext4_std_error(inode->i_sb, err);
out_brelse:
brelse(bh);
- ext4_std_error(inode->i_sb, err);
return err;
}
--
2.31.1
In ext4_get_inode_loc(), we may skip IO and get an zero && uptodate
inode buffer when the inode monopolize an inode block for performance
reason. For most cases, ext4_mark_iloc_dirty() will fill the inode
buffer to make it fine, but we could miss this call if something bad
happened. Finally, __ext4_get_inode_loc_noinmem() may probably get an
empty inode buffer and trigger ext4 error.
For example, if we remove a nonexistent xattr on inode A,
ext4_xattr_set_handle() will return ENODATA before invoking
ext4_mark_iloc_dirty(), it will left an uptodate but zero buffer. We
will get checksum error message in ext4_iget() when getting inode again.
EXT4-fs error (device sda): ext4_lookup:1784: inode #131074: comm cat:
iget: checksum invalid
Even worse, if we allocate another inode B at the same inode block, it
will corrupt the inode A on disk when write back inode B.
So this patch initialize the inode buffer by filling the in-mem inode
contents if we skip read I/O, ensure that the buffer is really uptodate.
Signed-off-by: Zhang Yi <[email protected]>
---
fs/ext4/inode.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3c36e701e30e..8b37f55b04ad 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4446,8 +4446,8 @@ static int ext4_fill_raw_inode(struct inode *inode, struct ext4_inode *raw_inode
* inode.
*/
static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,
- struct ext4_iloc *iloc, int in_mem,
- ext4_fsblk_t *ret_block)
+ struct inode *inode, struct ext4_iloc *iloc,
+ int in_mem, ext4_fsblk_t *ret_block)
{
struct ext4_group_desc *gdp;
struct buffer_head *bh;
@@ -4514,8 +4514,13 @@ static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,
}
brelse(bitmap_bh);
if (i == start + inodes_per_block) {
+ struct ext4_inode *raw_inode =
+ (struct ext4_inode *) (bh->b_data + iloc->offset);
+
/* all other inodes are free, so skip I/O */
memset(bh->b_data, 0, bh->b_size);
+ if (!ext4_test_inode_state(inode, EXT4_STATE_NEW))
+ ext4_fill_raw_inode(inode, raw_inode);
set_buffer_uptodate(bh);
unlock_buffer(bh);
goto has_buffer;
@@ -4576,7 +4581,7 @@ static int __ext4_get_inode_loc_noinmem(struct inode *inode,
ext4_fsblk_t err_blk;
int ret;
- ret = __ext4_get_inode_loc(inode->i_sb, inode->i_ino, iloc, 0,
+ ret = __ext4_get_inode_loc(inode->i_sb, inode->i_ino, NULL, iloc, 0,
&err_blk);
if (ret == -EIO)
@@ -4592,8 +4597,13 @@ int ext4_get_inode_loc(struct inode *inode, struct ext4_iloc *iloc)
int ret;
/* We have all inode data except xattrs in memory here. */
- ret = __ext4_get_inode_loc(inode->i_sb, inode->i_ino, iloc,
- !ext4_test_inode_state(inode, EXT4_STATE_XATTR), &err_blk);
+ if (ext4_test_inode_state(inode, EXT4_STATE_XATTR)) {
+ ret = __ext4_get_inode_loc(inode->i_sb, inode->i_ino, NULL,
+ iloc, false, &err_blk);
+ } else {
+ ret = __ext4_get_inode_loc(inode->i_sb, inode->i_ino, inode,
+ iloc, true, &err_blk);
+ }
if (ret == -EIO)
ext4_error_inode_block(inode, err_blk, EIO,
@@ -4606,7 +4616,7 @@ int ext4_get_inode_loc(struct inode *inode, struct ext4_iloc *iloc)
int ext4_get_fc_inode_loc(struct super_block *sb, unsigned long ino,
struct ext4_iloc *iloc)
{
- return __ext4_get_inode_loc(sb, ino, iloc, 0, NULL);
+ return __ext4_get_inode_loc(sb, ino, NULL, iloc, 0, NULL);
}
static bool ext4_should_enable_dax(struct inode *inode)
--
2.31.1
On Thu, Aug 26, 2021 at 09:04:12PM +0800, Zhang Yi wrote:
>
> So this patch initialize the inode buffer by filling the in-mem inode
> contents if we skip read I/O, ensure that the buffer is really uptodate.
>
> Signed-off-by: Zhang Yi <[email protected]>
> ---
> fs/ext4/inode.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3c36e701e30e..8b37f55b04ad 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4446,8 +4446,8 @@ static int ext4_fill_raw_inode(struct inode *inode, struct ext4_inode *raw_inode
> * inode.
> */
> static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,
> - struct ext4_iloc *iloc, int in_mem,
> - ext4_fsblk_t *ret_block)
> + struct inode *inode, struct ext4_iloc *iloc,
> + int in_mem, ext4_fsblk_t *ret_block)
In this patch you've added a new argument 'inode'. However, if in_mem
is true, and inode is NULL, the kernel will crash with a null pointer
dereference. Furthermore, whenever in_mem is false, the callers pass
in NULL for inode.
Given that, perhaps we should just drop the in_mem argument, and then
instead of
if (in_mem) {
we do:
if (inode && !ext4_test_inode_state(inode, EXT4_STATE_XATTR) {
with the comments adjusted accordingly?
I think it will make the code a bit simpler and readable.
What do you think?
- Ted
On Thu, Aug 26, 2021 at 09:04:09PM +0800, Zhang Yi wrote:
> Now that ext4_do_update_inode() return error before filling the whole
> inode data if we fail to set inode blocks in ext4_inode_blocks_set().
> This error should never happen in theory since sb->s_maxbytes should not
> have allowed this, we have already init sb->s_maxbytes according to this
> feature in ext4_fill_super(). So even through that could only happen due
> to the filesystem corruption, we'd better to return after we finish
> updating the inode because it may left an uninitialized buffer and we
> could read this buffer later in "errors=continue" mode.
>
> This patch make the updating inode data procedure atomic, call
> EXT4_ERROR_INODE() after we dropping i_raw_lock after something bad
> happened, make sure that the inode is integrated, and also drop a BUG_ON
> and do some small cleanups.
>
> Signed-off-by: Zhang Yi <[email protected]>
> Reviewed-by: Jan Kara <[email protected]>
Thanks, applied.
- Ted
On Thu, Aug 26, 2021 at 09:04:08PM +0800, Zhang Yi wrote:
> The "if (!buffer_uptodate(bh))" hunk covered almost the whole code after
> getting buffer in __ext4_get_inode_loc() which seems unnecessary, remove
> it and switch to check ext4_buffer_uptodate(), it simplify code and make
> it more readable.
>
> Signed-off-by: Zhang Yi <[email protected]>
> Reviewed-by: Jan Kara <[email protected]>
Thanks, applied.
- Ted
On 2021/8/31 11:02, Theodore Ts'o wrote:
> On Thu, Aug 26, 2021 at 09:04:12PM +0800, Zhang Yi wrote:
>>
>> So this patch initialize the inode buffer by filling the in-mem inode
>> contents if we skip read I/O, ensure that the buffer is really uptodate.
>>
>> Signed-off-by: Zhang Yi <[email protected]>
>> ---
>> fs/ext4/inode.c | 22 ++++++++++++++++------
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 3c36e701e30e..8b37f55b04ad 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -4446,8 +4446,8 @@ static int ext4_fill_raw_inode(struct inode *inode, struct ext4_inode *raw_inode
>> * inode.
>> */
>> static int __ext4_get_inode_loc(struct super_block *sb, unsigned long ino,
>> - struct ext4_iloc *iloc, int in_mem,
>> - ext4_fsblk_t *ret_block)
>> + struct inode *inode, struct ext4_iloc *iloc,
>> + int in_mem, ext4_fsblk_t *ret_block)
>
>
> In this patch you've added a new argument 'inode'. However, if in_mem
> is true, and inode is NULL, the kernel will crash with a null pointer
> dereference. Furthermore, whenever in_mem is false, the callers pass
> in NULL for inode.
>
> Given that, perhaps we should just drop the in_mem argument, and then
> instead of
>
> if (in_mem) {
>
> we do:
>
> if (inode && !ext4_test_inode_state(inode, EXT4_STATE_XATTR) {
>
> with the comments adjusted accordingly?
>
> I think it will make the code a bit simpler and readable.
>
> What do you think?
>
Yes，although I have already prevent passing 'in_mem' is true but 'inode' is
NULL in ext4_get_inode_loc(), using two arguments show the inode in-mem case
is not safe. I will remove the 'in_mem' parameter.
Thanks,
Yi.