Hi Chao, On Sun, Sep 18, 2016 at 11:30:06PM +0800, Chao Yu wrote: > From: Chao Yu <yuch...@huawei.com> > > This patch introduces spinlock to protect updating process of ckpt_flags > field in struct f2fs_checkpoint, it avoids incorrectly updating in race > condition.
So, I'm seeing a race condition between f2fs_stop_checkpoint(), write_checkpoint(), and f2fs_fill_super(). How about just adding f2fs_lock_op() in f2fs_stop_checkpoint()? Thanks, > Signed-off-by: Chao Yu <yuch...@huawei.com> > --- > fs/f2fs/checkpoint.c | 24 ++++++++++++------------ > fs/f2fs/f2fs.h | 29 +++++++++++++++++++++-------- > fs/f2fs/recovery.c | 2 +- > fs/f2fs/segment.c | 4 ++-- > fs/f2fs/super.c | 5 +++-- > 5 files changed, 39 insertions(+), 25 deletions(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index df56a43..0338f8c 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -28,7 +28,7 @@ struct kmem_cache *inode_entry_slab; > > void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi, bool end_io) > { > - set_ckpt_flags(sbi->ckpt, CP_ERROR_FLAG); > + set_ckpt_flags(sbi, CP_ERROR_FLAG); > sbi->sb->s_flags |= MS_RDONLY; > if (!end_io) > f2fs_flush_merged_bios(sbi); > @@ -571,7 +571,7 @@ int recover_orphan_inodes(struct f2fs_sb_info *sbi) > block_t start_blk, orphan_blocks, i, j; > int err; > > - if (!is_set_ckpt_flags(F2FS_CKPT(sbi), CP_ORPHAN_PRESENT_FLAG)) > + if (!is_set_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG)) > return 0; > > start_blk = __start_cp_addr(sbi) + 1 + __cp_payload(sbi); > @@ -595,7 +595,7 @@ int recover_orphan_inodes(struct f2fs_sb_info *sbi) > f2fs_put_page(page, 1); > } > /* clear Orphan Flag */ > - clear_ckpt_flags(F2FS_CKPT(sbi), CP_ORPHAN_PRESENT_FLAG); > + clear_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG); > return 0; > } > > @@ -1054,9 +1054,9 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, > struct cp_control *cpc) > /* 2 cp + n data seg summary + orphan inode blocks */ > data_sum_blocks = npages_for_summary_flush(sbi, false); > if (data_sum_blocks < NR_CURSEG_DATA_TYPE) > - set_ckpt_flags(ckpt, CP_COMPACT_SUM_FLAG); > + set_ckpt_flags(sbi, CP_COMPACT_SUM_FLAG); > else > - clear_ckpt_flags(ckpt, CP_COMPACT_SUM_FLAG); > + clear_ckpt_flags(sbi, CP_COMPACT_SUM_FLAG); > > orphan_blocks = GET_ORPHAN_BLOCKS(orphan_num); > ckpt->cp_pack_start_sum = cpu_to_le32(1 + cp_payload_blks + > @@ -1072,22 +1072,22 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, > struct cp_control *cpc) > orphan_blocks); > > if (cpc->reason == CP_UMOUNT) > - set_ckpt_flags(ckpt, CP_UMOUNT_FLAG); > + set_ckpt_flags(sbi, CP_UMOUNT_FLAG); > else > - clear_ckpt_flags(ckpt, CP_UMOUNT_FLAG); > + clear_ckpt_flags(sbi, CP_UMOUNT_FLAG); > > if (cpc->reason == CP_FASTBOOT) > - set_ckpt_flags(ckpt, CP_FASTBOOT_FLAG); > + set_ckpt_flags(sbi, CP_FASTBOOT_FLAG); > else > - clear_ckpt_flags(ckpt, CP_FASTBOOT_FLAG); > + clear_ckpt_flags(sbi, CP_FASTBOOT_FLAG); > > if (orphan_num) > - set_ckpt_flags(ckpt, CP_ORPHAN_PRESENT_FLAG); > + set_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG); > else > - clear_ckpt_flags(ckpt, CP_ORPHAN_PRESENT_FLAG); > + clear_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG); > > if (is_sbi_flag_set(sbi, SBI_NEED_FSCK)) > - set_ckpt_flags(ckpt, CP_FSCK_FLAG); > + set_ckpt_flags(sbi, CP_FSCK_FLAG); > > /* update SIT/NAT bitmap */ > get_sit_bitmap(sbi, __bitmap_ptr(sbi, SIT_BITMAP)); > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index c30f744b..b615f3f 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -814,6 +814,7 @@ struct f2fs_sb_info { > > /* for checkpoint */ > struct f2fs_checkpoint *ckpt; /* raw checkpoint pointer */ > + spinlock_t cp_lock; /* for flag in ckpt */ > struct inode *meta_inode; /* cache meta blocks */ > struct mutex cp_mutex; /* checkpoint procedure lock */ > struct rw_semaphore cp_rwsem; /* blocking FS operations */ > @@ -1083,24 +1084,36 @@ static inline unsigned long long > cur_cp_version(struct f2fs_checkpoint *cp) > return le64_to_cpu(cp->checkpoint_ver); > } > > -static inline bool is_set_ckpt_flags(struct f2fs_checkpoint *cp, unsigned > int f) > +static inline bool is_set_ckpt_flags(struct f2fs_sb_info *sbi, unsigned int > f) > { > + struct f2fs_checkpoint *cp = F2FS_CKPT(sbi); > unsigned int ckpt_flags = le32_to_cpu(cp->ckpt_flags); > + > return ckpt_flags & f; > } > > -static inline void set_ckpt_flags(struct f2fs_checkpoint *cp, unsigned int f) > +static inline void set_ckpt_flags(struct f2fs_sb_info *sbi, unsigned int f) > { > - unsigned int ckpt_flags = le32_to_cpu(cp->ckpt_flags); > + struct f2fs_checkpoint *cp = F2FS_CKPT(sbi); > + unsigned int ckpt_flags; > + > + spin_lock(&sbi->cp_lock); > + ckpt_flags = le32_to_cpu(cp->ckpt_flags); > ckpt_flags |= f; > cp->ckpt_flags = cpu_to_le32(ckpt_flags); > + spin_unlock(&sbi->cp_lock); > } > > -static inline void clear_ckpt_flags(struct f2fs_checkpoint *cp, unsigned int > f) > +static inline void clear_ckpt_flags(struct f2fs_sb_info *sbi, unsigned int f) > { > - unsigned int ckpt_flags = le32_to_cpu(cp->ckpt_flags); > + struct f2fs_checkpoint *cp = F2FS_CKPT(sbi); > + unsigned int ckpt_flags; > + > + spin_lock(&sbi->cp_lock); > + ckpt_flags = le32_to_cpu(cp->ckpt_flags); > ckpt_flags &= (~f); > cp->ckpt_flags = cpu_to_le32(ckpt_flags); > + spin_unlock(&sbi->cp_lock); > } > > static inline bool f2fs_discard_en(struct f2fs_sb_info *sbi) > @@ -1148,8 +1161,8 @@ static inline bool __remain_node_summaries(int reason) > > static inline bool __exist_node_summaries(struct f2fs_sb_info *sbi) > { > - return (is_set_ckpt_flags(F2FS_CKPT(sbi), CP_UMOUNT_FLAG) || > - is_set_ckpt_flags(F2FS_CKPT(sbi), CP_FASTBOOT_FLAG)); > + return (is_set_ckpt_flags(sbi, CP_UMOUNT_FLAG) || > + is_set_ckpt_flags(sbi, CP_FASTBOOT_FLAG)); > } > > /* > @@ -1851,7 +1864,7 @@ static inline int f2fs_readonly(struct super_block *sb) > > static inline bool f2fs_cp_error(struct f2fs_sb_info *sbi) > { > - return is_set_ckpt_flags(sbi->ckpt, CP_ERROR_FLAG); > + return is_set_ckpt_flags(sbi, CP_ERROR_FLAG); > } > > static inline bool is_dot_dotdot(const struct qstr *str) > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c > index ad748e5..37d99d2 100644 > --- a/fs/f2fs/recovery.c > +++ b/fs/f2fs/recovery.c > @@ -649,7 +649,7 @@ out: > invalidate_mapping_pages(META_MAPPING(sbi), > blkaddr, blkaddr); > > - set_ckpt_flags(sbi->ckpt, CP_ERROR_FLAG); > + set_ckpt_flags(sbi, CP_ERROR_FLAG); > mutex_unlock(&sbi->cp_mutex); > } else if (need_writecp) { > struct cp_control cpc = { > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 101b58f..dc68f30 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -1825,7 +1825,7 @@ static int restore_curseg_summaries(struct f2fs_sb_info > *sbi) > int type = CURSEG_HOT_DATA; > int err; > > - if (is_set_ckpt_flags(F2FS_CKPT(sbi), CP_COMPACT_SUM_FLAG)) { > + if (is_set_ckpt_flags(sbi, CP_COMPACT_SUM_FLAG)) { > int npages = npages_for_summary_flush(sbi, true); > > if (npages >= 2) > @@ -1922,7 +1922,7 @@ static void write_normal_summaries(struct f2fs_sb_info > *sbi, > > void write_data_summaries(struct f2fs_sb_info *sbi, block_t start_blk) > { > - if (is_set_ckpt_flags(F2FS_CKPT(sbi), CP_COMPACT_SUM_FLAG)) > + if (is_set_ckpt_flags(sbi, CP_COMPACT_SUM_FLAG)) > write_compacted_summaries(sbi, start_blk); > else > write_normal_summaries(sbi, start_blk, CURSEG_HOT_DATA); > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 555217f..f809729 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -735,7 +735,7 @@ static void f2fs_put_super(struct super_block *sb) > * clean checkpoint again. > */ > if (is_sbi_flag_set(sbi, SBI_IS_DIRTY) || > - !is_set_ckpt_flags(F2FS_CKPT(sbi), CP_UMOUNT_FLAG)) { > + !is_set_ckpt_flags(sbi, CP_UMOUNT_FLAG)) { > struct cp_control cpc = { > .reason = CP_UMOUNT, > }; > @@ -1477,6 +1477,7 @@ static void init_sb_info(struct f2fs_sb_info *sbi) > mutex_init(&sbi->umount_mutex); > mutex_init(&sbi->wio_mutex[NODE]); > mutex_init(&sbi->wio_mutex[DATA]); > + spin_lock_init(&sbi->cp_lock); > > #ifdef CONFIG_F2FS_FS_ENCRYPTION > memcpy(sbi->key_prefix, F2FS_KEY_DESC_PREFIX, > @@ -1818,7 +1819,7 @@ try_onemore: > * previous checkpoint was not done by clean system shutdown. > */ > if (bdev_read_only(sb->s_bdev) && > - !is_set_ckpt_flags(sbi->ckpt, CP_UMOUNT_FLAG)) { > + !is_set_ckpt_flags(sbi, CP_UMOUNT_FLAG)) { > err = -EROFS; > goto free_kobj; > } > -- > 2.7.2