Hi,
On 2025/2/13 20:51, Brian Foster wrote:
On Thu, Feb 13, 2025 at 07:22:47PM +0800, libaokun@xxxxxxxxxxxxxxx wrote:
From: Baokun Li <libaokun1@xxxxxxxxxx>
Otherwise, if ext4_inode_attach_jinode() fails, a hung task will
happen because filemap_invalidate_unlock() isn't called to unlock
mapping->invalidate_lock. Like this:
EXT4-fs error (device sda) in ext4_setattr:5557: Out of memory
INFO: task fsstress:374 blocked for more than 122 seconds.
Not tainted 6.14.0-rc1-next-20250206-xfstests-dirty #726
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:fsstress state:D stack:0 pid:374 tgid:374 ppid:373
task_flags:0x440140 flags:0x00000000
Call Trace:
<TASK>
__schedule+0x2c9/0x7f0
schedule+0x27/0xa0
schedule_preempt_disabled+0x15/0x30
rwsem_down_read_slowpath+0x278/0x4c0
down_read+0x59/0xb0
page_cache_ra_unbounded+0x65/0x1b0
filemap_get_pages+0x124/0x3e0
filemap_read+0x114/0x3d0
vfs_read+0x297/0x360
ksys_read+0x6c/0xe0
do_syscall_64+0x4b/0x110
entry_SYSCALL_64_after_hwframe+0x76/0x7e
Fixes: c7fc0366c656 ("ext4: partial zero eof block on unaligned inode size extension")
Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
---
First off, thank you for catching this. :)
Thanks for your review!
fs/ext4/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3cc8da6357aa..04ffd802dbde 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5452,7 +5452,7 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
oldsize & (inode->i_sb->s_blocksize - 1)) {
error = ext4_inode_attach_jinode(inode);
if (error)
- goto err_out;
+ goto out_mmap_sem;
}
This looks reasonable to me, but I notice that the immediate previous
error check looks like this:
...
rc = ext4_break_layouts(inode);
if (rc) {
filemap_invalidate_unlock(inode->i_mapping);
goto err_out;
}
...
... and then the following after the broken logic uses out_mmap_sem.
Could we be a little more consistent here one way or the other? The
change looks functionally correct to me either way:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
Brian
Indeed, this is confusing.
The reason is that we don't want to call ext4_std_error() when
ext4_break_layouts() fails. So we first store the error in 'rc', and then
pass the error to 'error' at the end. (See b9c1c26739ec
("ext4: gracefully handle ext4_break_layouts() failure during truncate"))
However, because 'error' is not assigned, the goto out_mmap_sem label will
execute some code that shouldn't be executed. Therefore, in the error
handling of ext4_break_layouts(), we unlock and then goto err_out label.
While under normal error conditions, 'error' is assigned, and it should
enter the out_mmap_sem label. Therefore, in the error handling of
ext4_inode_attach_jinode(), we directly goto out_mmap_sem label.
The handling of 'rc' in this function is indeed very subtle.
Cheers,
Baokun
handle = ext4_journal_start(inode, EXT4_HT_INODE, 3);
--
2.39.2
