On Fri 12-07-24 19:01:25, harshad shirwadkar wrote: > On Fri, Jun 28, 2024 at 7:18 AM Jan Kara <jack@xxxxxxx> 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@xxxxxxxxx> > > > > 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@xxxxxxxx> SUSE Labs, CR
