Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 16 Jun 2022 12:01:00 +0530
From:   Ritesh Harjani <ritesh.list@gmail.com>
To:     Jan Kara <jack@suse.cz>
Cc:     Ye Bin <yebin10@huawei.com>, tytso@mit.edu,
        adilger.kernel@dilger.ca, linux-ext4@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH -next] ext4: fix bug_on in ext4_iomap_begin as race
 between bmap and write
Message-ID: <20220616063100.mkuqs6cxcqf6u234@riteshh-domain>
References: <20220615135850.1961759-1-yebin10@huawei.com>
 <20220615152139.vp64tnv46enwnfcs@riteshh-domain>
 <20220615153123.ab32zt75q7yn7jc5@riteshh-domain>
 <20220615172609.oydxhjw7exas23hd@quack3.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220615172609.oydxhjw7exas23hd@quack3.lan>
Precedence: bulk

On 22/06/15 07:26PM, Jan Kara wrote:
> On Wed 15-06-22 21:01:23, Ritesh Harjani wrote:
> > On 22/06/15 08:51PM, Ritesh Harjani wrote:
> > > On 22/06/15 09:58PM, Ye Bin wrote:
> > > > We got issue as follows:
> > > > ------------[ cut here ]------------
> > > > WARNING: CPU: 3 PID: 9310 at fs/ext4/inode.c:3441 ext4_iomap_begin+0x182/0x5d0
> > > > RIP: 0010:ext4_iomap_begin+0x182/0x5d0
> > > > RSP: 0018:ffff88812460fa08 EFLAGS: 00010293
> > > > RAX: ffff88811f168000 RBX: 0000000000000000 RCX: ffffffff97793c12
> > > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
> > > > RBP: ffff88812c669160 R08: ffff88811f168000 R09: ffffed10258cd20f
> > > > R10: ffff88812c669077 R11: ffffed10258cd20e R12: 0000000000000001
> > > > R13: 00000000000000a4 R14: 000000000000000c R15: ffff88812c6691ee
> > > > FS:  00007fd0d6ff3740(0000) GS:ffff8883af180000(0000) knlGS:0000000000000000
> > > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 00007fd0d6dda290 CR3: 0000000104a62000 CR4: 00000000000006e0
> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > Call Trace:
> > > >  iomap_apply+0x119/0x570
> > > >  iomap_bmap+0x124/0x150
> > > >  ext4_bmap+0x14f/0x250
> > > >  bmap+0x55/0x80
> > > >  do_vfs_ioctl+0x952/0xbd0
> > > >  __x64_sys_ioctl+0xc6/0x170
> > > >  do_syscall_64+0x33/0x40
> > > >  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > >
> > > > Above issue may happen as follows:
> > > >           bmap                    write
> > > > bmap
> > > >   ext4_bmap
> > > >     iomap_bmap
> > > >       ext4_iomap_begin
> > > >                             ext4_file_write_iter
> > > > 			      ext4_buffered_write_iter
> > > > 			        generic_perform_write
> > > > 				  ext4_da_write_begin
> > > > 				    ext4_da_write_inline_data_begin
> > > > 				      ext4_prepare_inline_data
> > > > 				        ext4_create_inline_data
> > > > 					  ext4_set_inode_flag(inode,
> > > > 						EXT4_INODE_INLINE_DATA);
> > > >       if (WARN_ON_ONCE(ext4_has_inline_data(inode))) ->trigger bug_on

Actually it's only a WARN_ON_ONCE and not a bug_on.
(You might have made panic_on_warn set to 1 in your testing)

> > > >
> > > > To solved above issue hold inode lock in ext4_bamp.
> > > 											^^^ ext4_bmap()
> > >
> > > I checked the paths where bmap() kernel api can be called i.e. from jbd2/fc and
> > > generic_swapfile_activate() (apart from ioctl())
> > > For jbd2, it will be called with j_inode within bmap(), hence taking a inode lock
> > > of the inode passed within ext4_bmap() (j_inode in this case) should be safe here.
> > > Same goes with swapfile path as well.
> > >
> > > However I feel maybe we should hold inode_lock_shared() since there is no
> > > block/extent map layout changes that can happen via ext4_bmap().
> > > Hence read lock is what IMO should be used here.
> >
> > On second thoughts, shoudn't we use ext4_iomap_report_ops here?  Can't
> > recollect why we didn't use ext4_iomap_report_ops for iomap_bmap() in the
> > first place. Should be good to verify it once.
>
> Hum, but I guess there's a deeper problem than ext4_bmap(). Generally we
> have places doing block mapping (such as ext4_writepages(), readahead, or
> page fault) where we don't hold i_rwsem and racing
> ext4_create_inline_data() could confuse them? I guess we need to come up

You are right, i_rwsem won't be able to protect against such races which you
described. So, we actually use EXT4_I(inode)->xattr_sem for inline data
serialization.

So for this issue, I think if we should move from ext4_iomap_ops to
ext4_iomap_report_ops. ext4_iomap_begin_report does takes care of read locking
xattr_sem to properly report if it's a inline_data and similarly iomap_bmap
reports 0 (which it should) in case of iomap->type != IOMAP_MAPPED
(since in this case ext4_iomap_begin_report() will give IOMAP_INLINE)

Thoughts?

-ritesh


> with a sound scheme how inline data creation is serialized with these
> operations (or just decide to remove the inline data feature altogether as
> we already discussed once because the complexity likely is not worth the
> gain).
>
> 								Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR