Re: [PATCH v6 07/10] ext4: add nolock mode to ext4_map_blocks()

[Jan Kara <jack@suse.cz>]

On Fri 12-07-24 19:01:25, harshad shirwadkar wrote:
> On Fri, Jun 28, 2024 at 7:18 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 29-05-24 01:20:00, Harshad Shirwadkar wrote:
> > > Add nolock flag to ext4_map_blocks() which skips grabbing
> > > i_data_sem in ext4_map_blocks. In FC commit path, we first
> > > mark the inode as committing and thereby prevent any mutations
> > > on it. Thus, it should be safe to call ext4_map_blocks()
> > > without i_data_sem in this case. This is a workaround to
> > > the problem mentioned in RFC V4 version cover letter[1] of this
> > > patch series which pointed out that there is in incosistency between
> > > ext4_map_blocks() behavior when EXT4_GET_BLOCKS_CACHED_NOWAIT is
> > > passed. This patch gets rid of the need to call ext4_map_blocks()
> > > with EXT4_GET_BLOCKS_CACHED_NOWAIT and instead call it with
> > > EXT4_GET_BLOCKS_NOLOCK. I verified that generic/311 which failed
> > > in cached_nowait mode passes with nolock mode.
> > >
> > > [1] https://lwn.net/Articles/902022/
> > >
> > > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
> >
> > I'm sorry I forgot since last time - can you remind me why we cannot we
> > grab i_data_sem from ext4_fc_write_inode_data()? Because as you write
> > above, nobody should really be holding that lock while inode is
> > EXT4_STATE_FC_COMMITTING anyway...
> >
> The original reason was that the commit path calls ext4_map_blocks()
> which needs i_data_sem. But other places might grab i_data_sem and
> then call ext4_mark_inode_dirty(). Ext4_mark_inode_dirty() can block
> for a fast commit to finish, causing a deadlock.
> 
> In this patchset I'm attacking this problem 2 ways:
> (1) Ensure i_data_sem is always grabbed before ext4_mark_inode_dirty()

I think this rather should be: Make sure the inode is properly tracked with
fastcommit code (which waits for EXT4_STATE_FC_COMMITTING) before grabbing
i_data_sem, shouldn't it?

> (2) (This patch) Remove the need of grabbing i_data_sem in
> ext4_map_blocks() when in the commit path.
> 
> I am now realizing either (1) or (2) is sufficient -- both are not
> needed.

Yes, this is what was confusing me somewhat.

> (2) is more maintainable. (1) seems fragile and future code
> paths can potentially break that rule which can cause hard to debug
> failures. So, how about just keeping this patch and dropping the need
> to remove grab i_data_sem before ext4_mark_inode_dirty()? If no
> concerns, I'll handle this in V7.

Well, you have added assertions into ext4_mark_inode_dirty() exactly to
catch possible problems with inode not being tracked with fastcommit code.
I agree 1) needs changes in more places but long term, it actually seems
*less* fragile with the assertions added. Because adding conditional
locking to our core block mapping function and relying on the fact that
nobody can modify the mapping structures while EXT4_STATE_FC_COMMITTING is
set is quite hard to assert for and the failures are going to be hard to
debug as they will result in random memory corruptions, oopses etc. So I
believe you should rather remove 2).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR