Re: [PATCH] btrfs: Add assert or condition

[Lizhi Xu]

On Sat, 31 Aug 2024 11:55:53 +0100, Filipe Manana wrote:
> > -       ASSERT(inode_is_locked(&inode->vfs_inode));
> > +       ASSERT(inode_is_locked(&inode->vfs_inode) ||
> > +              rwsem_is_locked(&inode->i_mmap_lock));
> 
> This definitely fixes the syzbot report, in the sense the assertion
> won't fail anymore.
> But it's wrong, very, very, very, very wrong.
> 
> The inode must be locked during the course of the fsync, that's why
> the assertion is there.
> 
> Why do you think it's ok to not have the inode locked?
> Have you done any analysis about that?
> 
> You mention "fsync_skip_inode_lock is true" in the changelog, but have
> you checked where and why it's set to true?
> 
> Where we set it to true, at btrfs_direct_write(), there's a comment
> which explains it's to avoid a deadlock on the inode lock at
> btrfs_sync_file().
> 
> This is a perfect example of trying a patch not only without having
> any idea how the code works but also being very lazy,
> as there's a very explicit comment in the code about why the variable
> is set to true, and even much more detailed in the
> change log of the commit that introduced it.
> 
> And btw, there's already a patch to fix this issue:
> 
> https://lore.kernel.org/linux-btrfs/717029440fe379747b9548a9c91eb7801bc5a813.1724972507.git.fdmanana@xxxxxxxx/
In your patch, I get what the mean of fsync_skip_inode_lock.

Thanks.