On 11/12, Chao Yu wrote:
> On 2018-11-12 20:01, Sheng Yong wrote:
> > The following race could lead to inconsistent SIT bitmap:
> >
> > Task A Task B
> > ====== ======
> > f2fs_write_checkpoint
> > block_operations
> > f2fs_lock_all
> > down_write(node_change)
> > down_write(node_write)
> > ... sync ...
> > up_write(node_change)
> > f2fs_file_write_iter
> > set_inode_flag(FI_NO_PREALLOC)
> > ......
> > f2fs_write_begin(index=0, has inline data)
> > prepare_write_begin
> > __do_map_lock(AIO) =>
> > down_read(node_change)
> > f2fs_convert_inline_page => update SIT
> > __do_map_lock(AIO) =>
> > up_read(node_change)
> > f2fs_flush_sit_entries <= inconsistent SIT
> > finish write checkpoint
> > sudden-power-off
> >
> > If SPO occurs after checkpoint is finished, SIT bitmap will be set
> > incorrectly. This patch uses node_write to avoid the race condition.
>
> Good catch!
>
> >
> > Signed-off-by: Sheng Yong <[email protected]>
> > ---
> > fs/f2fs/data.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 106f116466bf..dc5b251aee86 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -2355,7 +2355,9 @@ static int prepare_write_begin(struct f2fs_sb_info
> > *sbi,
> > if (inode->i_nlink)
> > set_inline_node(ipage);
> > } else {
> > + down_read(&sbi->node_write);
>
> This makes lock dependence more complicated, how about calling f2fs_lock_op
> for
> inline data conversion case:
>
> struct extent_info ei = {0,0,0};
> + int flag;
> int err = 0;
>
> - if (f2fs_has_inline_data(inode) ||
> - (pos & PAGE_MASK) >= i_size_read(inode)) {
> - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, true);
> + if (f2fs_has_inline_data(inode)) {
> + if (pos + len > MAX_INLINE_DATA(inode))
> + flag = F2FS_GET_BLOCK_PRE_DIO;
This is not DIO case. I think calling f2fs_lock_op() explicitly might be better.
> + else if (pos & PAGE_MASK) >= i_size_read(inode))
> + flag = F2FS_GET_BLOCK_PRE_AIO;
> + __do_map_lock(sbi, flag, true);
>
> if (err || dn.data_blkaddr == NULL_ADDR) {
> f2fs_put_dnode(&dn);
> - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO,
> - true);
> + flag = F2FS_GET_BLOCK_PRE_AIO;
> + __do_map_lock(sbi, flag, true);
> locked = true;
>
> unlock_out:
> if (locked)
> - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, false);
> + __do_map_lock(sbi, flag, false);
>
> Thanks,
>
> > err = f2fs_convert_inline_page(&dn, page);
> > + up_read(&sbi->node_write);
> > if (err)
> > goto out;
> > if (dn.data_blkaddr == NULL_ADDR)
> >
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
