On 2018/11/15 9:59, Chao Yu wrote:
On 2018/11/14 19:34, 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. Signed-off-by: Sheng Yong <[email protected]> --- v2->v1: Sorry for late. We can use f2fs_lock_op directly, but it makes it a bit complicate to unlock. So v2 still uses __do_map_lock with different flags to avoid race. Use F2FS_GET_BLOCK_PRE_AIO and F2FS_GET_BLOCK_DEFAULT to tell different condition. Thanks --- fs/f2fs/data.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 6e0ffb1749ca..4abd47d82cdb 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -2376,6 +2376,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi, struct page *ipage; bool locked = false; struct extent_info ei = {0,0,0}; + int flag = F2FS_GET_BLOCK_PRE_AIO; int err = 0;/*@@ -2386,9 +2387,12 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi, !is_inode_flag_set(inode, FI_NO_PREALLOC)) return 0;+ if (f2fs_has_inline_data(inode) && pos + len > MAX_INLINE_DATA(inode))+ /* avoid race between write CP and convert_inline_page */ + flag = F2FS_GET_BLOCK_DEFAULT; if (f2fs_has_inline_data(inode) || (pos & PAGE_MASK) >= i_size_read(inode)) { - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, true); + __do_map_lock(sbi, flag, true); locked = true; } restart: @@ -2424,8 +2428,8 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi, err = f2fs_get_dnode_of_data(&dn, index, LOOKUP_NODE); 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);It looks there is no different after you changed above two lines? I guess you just want to keep parameter @flag be consistent in __do_{un,}map_lock pair, right?
Yes. The flag can only be F2FS_GET_BLOCK_PRE_AIO here (the same as initialized value), this is only used to ensure the flag is not changed (it is not for now) before unlock. :) thanks
Anyway, it's okay to me. Reviewed-by: Chao Yu <[email protected]> Thanks,locked = true; goto restart; } @@ -2439,7 +2443,7 @@ static int prepare_write_begin(struct f2fs_sb_info *sbi, f2fs_put_dnode(&dn); unlock_out: if (locked) - __do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, false); + __do_map_lock(sbi, flag, false); return err; }.
_______________________________________________ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
