Hi Jaegeuk,

On 2016/9/20 5:49, Jaegeuk Kim wrote:
> 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()?

I'm afraid there will be a potential deadlock here:

Thread A                                Thread B
- write_checkpoint
 - block_operations
  - f2fs_lock_all
 - do_checkpoint
 - wait_on_all_pages_writeback
                                        - f2fs_write_end_io
                                         - f2fs_stop_checkpoint
                                          - f2fs_lock_op
                                         - end_page_writeback
                                         - atomic_dec_and_test
                                         - wake_up

Right?

Thanks,

> 
> 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
> 
> .
> 


------------------------------------------------------------------------------
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Reply via email to