Re: [PATCH V3] f2fs: Avoid double lock for cp_rwsem during checkpoint

[Sayali Lokhande]

On 4/30/2020 6:41 AM, Chao Yu wrote:
> On 2020/4/30 1:00, Sayali Lokhande wrote:
> > There could be a scenario where f2fs_sync_node_pages gets
> > called during checkpoint, which in turn tries to flush
> > inline data and calls iput(). This results in deadlock as
> > iput() tries to hold cp_rwsem, which is already held at the
> > beginning by checkpoint->block_operations().
> >
> > Call stack :
> >
> > Thread A		Thread B
> > f2fs_write_checkpoint()
> > - block_operations(sbi)
> >   - f2fs_lock_all(sbi);
> >    - down_write(&sbi->cp_rwsem);
> >
> >                          - open()
> >                           - igrab()
> >                          - write() write inline data
> >                          - unlink()
> > - f2fs_sync_node_pages()
> >   - if (is_inline_node(page))
> >    - flush_inline_data()
> >     - ilookup()
> >       page = f2fs_pagecache_get_page()
> >       if (!page)
> >        goto iput_out;
> >       iput_out:
> > 			-close()
> > 			-iput()
> >         iput(inode);
> >         - f2fs_evict_inode()
> >          - f2fs_truncate_blocks()
> >           - f2fs_lock_op()
> >             - down_read(&sbi->cp_rwsem);
> >
> > Fixes: 399368372ed9 ("f2fs: introduce a new global lock scheme")
> IMO, it should be
>
> 2049d4fcb057 ("f2fs: avoid multiple node page writes due to inline_data")
>
> It brings iput() to checkpoint process for the first time.
>
> Thanks,
Agreed. will update it.
> > Signed-off-by: Sayali Lokhande <sayalil@xxxxxxxxxxxxxx>
> > ---
> >   fs/f2fs/checkpoint.c | 10 ++++------
> >   1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 5ba649e..97b6378 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -1219,21 +1219,19 @@ static int block_operations(struct f2fs_sb_info *sbi)
> >   		goto retry_flush_quotas;
> >   	}
> >   
> > -retry_flush_nodes:
> >   	down_write(&sbi->node_write);
> >   
> >   	if (get_pages(sbi, F2FS_DIRTY_NODES)) {
> >   		up_write(&sbi->node_write);
> > +		up_write(&sbi->node_change);
> > +		f2fs_unlock_all(sbi);
> >   		atomic_inc(&sbi->wb_sync_req[NODE]);
> >   		err = f2fs_sync_node_pages(sbi, &wbc, false, FS_CP_NODE_IO);
> >   		atomic_dec(&sbi->wb_sync_req[NODE]);
> > -		if (err) {
> > -			up_write(&sbi->node_change);
> > -			f2fs_unlock_all(sbi);
> > +		if (err)
> >   			goto out;
> > -		}
> >   		cond_resched();
> > -		goto retry_flush_nodes;
> > +		goto retry_flush_quotas;
> >   	}
> >   
> >   	/*