On Thu, Feb 13, 2025 at 09:20:21PM +0800, Baokun Li wrote:
> 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.
>
Ah, indeed.. I glossed over the use of rc in there on my quick read.
Thanks for the clarification!
Brian
>
> Cheers,
> Baokun
> > > handle = ext4_journal_start(inode, EXT4_HT_INODE, 3);
> > > --
> > > 2.39.2
> > >
> > >
>
