[PATCH 1/6] f2fs: make f2fs_filetype_table static

[PATCH 1/6] f2fs: make f2fs_filetype_table static

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

There is no more user of f2fs_filetype_table outside of dir.c, make it
static.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/dir.c  | 2 +-
 fs/f2fs/f2fs.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 2fb20fc..39a850b 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -37,7 +37,7 @@ static unsigned int bucket_blocks(unsigned int level)
 		return 4;
 }
 
-unsigned char f2fs_filetype_table[F2FS_FT_MAX] = {
+static unsigned char f2fs_filetype_table[F2FS_FT_MAX] = {
 	[F2FS_FT_UNKNOWN]	= DT_UNKNOWN,
 	[F2FS_FT_REG_FILE]	= DT_REG,
 	[F2FS_FT_DIR]		= DT_DIR,
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index af06303..9b4bbf2 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1946,7 +1946,6 @@ struct dentry *f2fs_get_parent(struct dentry *child);
 /*
  * dir.c
  */
-extern unsigned char f2fs_filetype_table[F2FS_FT_MAX];
 void set_de_type(struct f2fs_dir_entry *, umode_t);
 unsigned char get_de_type(struct f2fs_dir_entry *);
 struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *,
-- 
2.7.2

[PATCH 2/6] f2fs: fix to return error number of read_all_xattrs correctly

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

We treat all error in read_all_xattrs as a no memory error, which covers
the real reason of failure in it. Fix it by return correct errno in order
to reflect the real cause.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/xattr.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 1660191..83234dd 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -217,18 +217,20 @@ static struct f2fs_xattr_entry *__find_xattr(void *base_addr, int index,
 	return entry;
 }
 
-static void *read_all_xattrs(struct inode *inode, struct page *ipage)
+static int read_all_xattrs(struct inode *inode, struct page *ipage,
+							void **base_addr)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct f2fs_xattr_header *header;
 	size_t size = PAGE_SIZE, inline_size = 0;
 	void *txattr_addr;
+	int err;
 
 	inline_size = inline_xattr_size(inode);
 
 	txattr_addr = kzalloc(inline_size + size, GFP_F2FS_ZERO);
 	if (!txattr_addr)
-		return NULL;
+		return -ENOMEM;
 
 	/* read from inline xattr */
 	if (inline_size) {
@@ -239,8 +241,10 @@ static void *read_all_xattrs(struct inode *inode, struct page *ipage)
 			inline_addr = inline_xattr_addr(ipage);
 		} else {
 			page = get_node_page(sbi, inode->i_ino);
-			if (IS_ERR(page))
+			if (IS_ERR(page)) {
+				err = PTR_ERR(page);
 				goto fail;
+			}
 			inline_addr = inline_xattr_addr(page);
 		}
 		memcpy(txattr_addr, inline_addr, inline_size);
@@ -254,8 +258,10 @@ static void *read_all_xattrs(struct inode *inode, struct page *ipage)
 
 		/* The inode already has an extended attribute block. */
 		xpage = get_node_page(sbi, F2FS_I(inode)->i_xattr_nid);
-		if (IS_ERR(xpage))
+		if (IS_ERR(xpage)) {
+			err = PTR_ERR(xpage);
 			goto fail;
+		}
 
 		xattr_addr = page_address(xpage);
 		memcpy(txattr_addr + inline_size, xattr_addr, PAGE_SIZE);
@@ -269,10 +275,11 @@ static void *read_all_xattrs(struct inode *inode, struct page *ipage)
 		header->h_magic = cpu_to_le32(F2FS_XATTR_MAGIC);
 		header->h_refcount = cpu_to_le32(1);
 	}
-	return txattr_addr;
+	*base_addr = txattr_addr;
+	return 0;
 fail:
 	kzfree(txattr_addr);
-	return NULL;
+	return err;
 }
 
 static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
@@ -366,9 +373,9 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
 	if (len > F2FS_NAME_LEN)
 		return -ERANGE;
 
-	base_addr = read_all_xattrs(inode, ipage);
-	if (!base_addr)
-		return -ENOMEM;
+	error = read_all_xattrs(inode, ipage, &base_addr);
+	if (error)
+		return error;
 
 	entry = __find_xattr(base_addr, index, len, name);
 	if (IS_XATTR_LAST_ENTRY(entry)) {
@@ -402,9 +409,9 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 	int error = 0;
 	size_t rest = buffer_size;
 
-	base_addr = read_all_xattrs(inode, NULL);
-	if (!base_addr)
-		return -ENOMEM;
+	error = read_all_xattrs(inode, NULL, &base_addr);
+	if (error)
+		return error;
 
 	list_for_each_xattr(entry, base_addr) {
 		const struct xattr_handler *handler =
@@ -463,9 +470,9 @@ static int __f2fs_setxattr(struct inode *inode, int index,
 	if (size > MAX_VALUE_LEN(inode))
 		return -E2BIG;
 
-	base_addr = read_all_xattrs(inode, ipage);
-	if (!base_addr)
-		return -ENOMEM;
+	error = read_all_xattrs(inode, ipage, &base_addr);
+	if (error)
+		return error;
 
 	/* find entry with wanted name. */
 	here = __find_xattr(base_addr, index, len, name);
-- 
2.7.2

[PATCH 2/6] f2fs: fix to return error number of read_all_xattrs correctly

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

We treat all error in read_all_xattrs as a no memory error, which covers
the real reason of failure in it. Fix it by return correct errno in order
to reflect the real cause.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/xattr.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index 1660191..83234dd 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -217,18 +217,20 @@ static struct f2fs_xattr_entry *__find_xattr(void *base_addr, int index,
 	return entry;
 }
 
-static void *read_all_xattrs(struct inode *inode, struct page *ipage)
+static int read_all_xattrs(struct inode *inode, struct page *ipage,
+							void **base_addr)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct f2fs_xattr_header *header;
 	size_t size = PAGE_SIZE, inline_size = 0;
 	void *txattr_addr;
+	int err;
 
 	inline_size = inline_xattr_size(inode);
 
 	txattr_addr = kzalloc(inline_size + size, GFP_F2FS_ZERO);
 	if (!txattr_addr)
-		return NULL;
+		return -ENOMEM;
 
 	/* read from inline xattr */
 	if (inline_size) {
@@ -239,8 +241,10 @@ static void *read_all_xattrs(struct inode *inode, struct page *ipage)
 			inline_addr = inline_xattr_addr(ipage);
 		} else {
 			page = get_node_page(sbi, inode->i_ino);
-			if (IS_ERR(page))
+			if (IS_ERR(page)) {
+				err = PTR_ERR(page);
 				goto fail;
+			}
 			inline_addr = inline_xattr_addr(page);
 		}
 		memcpy(txattr_addr, inline_addr, inline_size);
@@ -254,8 +258,10 @@ static void *read_all_xattrs(struct inode *inode, struct page *ipage)
 
 		/* The inode already has an extended attribute block. */
 		xpage = get_node_page(sbi, F2FS_I(inode)->i_xattr_nid);
-		if (IS_ERR(xpage))
+		if (IS_ERR(xpage)) {
+			err = PTR_ERR(xpage);
 			goto fail;
+		}
 
 		xattr_addr = page_address(xpage);
 		memcpy(txattr_addr + inline_size, xattr_addr, PAGE_SIZE);
@@ -269,10 +275,11 @@ static void *read_all_xattrs(struct inode *inode, struct page *ipage)
 		header->h_magic = cpu_to_le32(F2FS_XATTR_MAGIC);
 		header->h_refcount = cpu_to_le32(1);
 	}
-	return txattr_addr;
+	*base_addr = txattr_addr;
+	return 0;
 fail:
 	kzfree(txattr_addr);
-	return NULL;
+	return err;
 }
 
 static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
@@ -366,9 +373,9 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
 	if (len > F2FS_NAME_LEN)
 		return -ERANGE;
 
-	base_addr = read_all_xattrs(inode, ipage);
-	if (!base_addr)
-		return -ENOMEM;
+	error = read_all_xattrs(inode, ipage, &base_addr);
+	if (error)
+		return error;
 
 	entry = __find_xattr(base_addr, index, len, name);
 	if (IS_XATTR_LAST_ENTRY(entry)) {
@@ -402,9 +409,9 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 	int error = 0;
 	size_t rest = buffer_size;
 
-	base_addr = read_all_xattrs(inode, NULL);
-	if (!base_addr)
-		return -ENOMEM;
+	error = read_all_xattrs(inode, NULL, &base_addr);
+	if (error)
+		return error;
 
 	list_for_each_xattr(entry, base_addr) {
 		const struct xattr_handler *handler =
@@ -463,9 +470,9 @@ static int __f2fs_setxattr(struct inode *inode, int index,
 	if (size > MAX_VALUE_LEN(inode))
 		return -E2BIG;
 
-	base_addr = read_all_xattrs(inode, ipage);
-	if (!base_addr)
-		return -ENOMEM;
+	error = read_all_xattrs(inode, ipage, &base_addr);
+	if (error)
+		return error;
 
 	/* find entry with wanted name. */
 	here = __find_xattr(base_addr, index, len, name);
-- 
2.7.2


------------------------------------------------------------------------------

[PATCH 3/6] f2fs: fix to avoid race condition when updating sbi flag

From: Chao Yu <yuchao0@huawei.com>

Making updating of sbi flag atomic by using {test,set,clear}_bit,
otherwise in concurrency scenario, the flag could be updated incorrectly.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/f2fs.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 9b4bbf2..c30f744b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -794,7 +794,7 @@ struct f2fs_sb_info {
 	struct proc_dir_entry *s_proc;		/* proc entry */
 	struct f2fs_super_block *raw_super;	/* raw super block pointer */
 	int valid_super_block;			/* valid super block no */
-	int s_flag;				/* flags for sbi */
+	unsigned long s_flag;				/* flags for sbi */
 
 #ifdef CONFIG_F2FS_FS_ENCRYPTION
 	u8 key_prefix[F2FS_KEY_DESC_PREFIX_SIZE];
@@ -1063,17 +1063,19 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
 
 static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
 {
-	return sbi->s_flag & (0x01 << type);
+	return test_bit(type, &sbi->s_flag);
 }
 
 static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
 {
-	sbi->s_flag |= (0x01 << type);
+	if (!test_bit(type, &sbi->s_flag))
+		set_bit(type, &sbi->s_flag);
 }
 
 static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
 {
-	sbi->s_flag &= ~(0x01 << type);
+	if (test_bit(type, &sbi->s_flag))
+		clear_bit(type, &sbi->s_flag);
 }
 
 static inline unsigned long long cur_cp_version(struct f2fs_checkpoint *cp)
-- 
2.7.2

[PATCH 3/6] f2fs: fix to avoid race condition when updating sbi flag

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

Making updating of sbi flag atomic by using {test,set,clear}_bit,
otherwise in concurrency scenario, the flag could be updated incorrectly.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/f2fs.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 9b4bbf2..c30f744b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -794,7 +794,7 @@ struct f2fs_sb_info {
 	struct proc_dir_entry *s_proc;		/* proc entry */
 	struct f2fs_super_block *raw_super;	/* raw super block pointer */
 	int valid_super_block;			/* valid super block no */
-	int s_flag;				/* flags for sbi */
+	unsigned long s_flag;				/* flags for sbi */
 
 #ifdef CONFIG_F2FS_FS_ENCRYPTION
 	u8 key_prefix[F2FS_KEY_DESC_PREFIX_SIZE];
@@ -1063,17 +1063,19 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
 
 static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
 {
-	return sbi->s_flag & (0x01 << type);
+	return test_bit(type, &sbi->s_flag);
 }
 
 static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
 {
-	sbi->s_flag |= (0x01 << type);
+	if (!test_bit(type, &sbi->s_flag))
+		set_bit(type, &sbi->s_flag);
 }
 
 static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
 {
-	sbi->s_flag &= ~(0x01 << type);
+	if (test_bit(type, &sbi->s_flag))
+		clear_bit(type, &sbi->s_flag);
 }
 
 static inline unsigned long long cur_cp_version(struct f2fs_checkpoint *cp)
-- 
2.7.2


------------------------------------------------------------------------------

[PATCH 4/6] f2fs: introduce cp_lock to protect updating of ckpt_flags

[Chao Yu]

From: Chao Yu <yuchao0@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.

Signed-off-by: Chao Yu <yuchao0@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

[PATCH 4/6] f2fs: introduce cp_lock to protect updating of ckpt_flags

[Chao Yu]

From: Chao Yu <yuchao0@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.

Signed-off-by: Chao Yu <yuchao0@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


------------------------------------------------------------------------------

[PATCH 5/6] f2fs: support IO error injection

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

This patch adds to support IO error injection for testing IO error
tolerance of f2fs.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/data.c  | 5 +++++
 fs/f2fs/f2fs.h  | 3 +++
 fs/f2fs/super.c | 1 +
 3 files changed, 9 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 120a995..251cc33 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -34,6 +34,11 @@ static void f2fs_read_end_io(struct bio *bio)
 	struct bio_vec *bvec;
 	int i;
 
+#ifdef CONFIG_F2FS_FAULT_INJECTION
+	if (time_to_inject(FAULT_IO))
+		bio->bi_error = -EIO;
+#endif
+
 	if (f2fs_bio_encrypted(bio)) {
 		if (bio->bi_error) {
 			fscrypt_release_ctx(bio->bi_private);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b615f3f..a1a68bf 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -46,6 +46,7 @@ enum {
 	FAULT_BLOCK,
 	FAULT_DIR_DEPTH,
 	FAULT_EVICT_INODE,
+	FAULT_IO,
 	FAULT_MAX,
 };
 
@@ -77,6 +78,8 @@ static inline bool time_to_inject(int type)
 		return false;
 	else if (type == FAULT_EVICT_INODE && !IS_FAULT_SET(type))
 		return false;
+	else if (type == FAULT_IO && !IS_FAULT_SET(type))
+		return false;
 
 	atomic_inc(&f2fs_fault.inject_ops);
 	if (atomic_read(&f2fs_fault.inject_ops) >= f2fs_fault.inject_rate) {
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index f809729..fafa34e 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -50,6 +50,7 @@ char *fault_name[FAULT_MAX] = {
 	[FAULT_BLOCK]		= "no more block",
 	[FAULT_DIR_DEPTH]	= "too big dir depth",
 	[FAULT_EVICT_INODE]	= "evict_inode fail",
+	[FAULT_IO]		= "IO error",
 };
 
 static void f2fs_build_fault_attr(unsigned int rate)
-- 
2.7.2

[PATCH 5/6] f2fs: support IO error injection

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

This patch adds to support IO error injection for testing IO error
tolerance of f2fs.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/data.c  | 5 +++++
 fs/f2fs/f2fs.h  | 3 +++
 fs/f2fs/super.c | 1 +
 3 files changed, 9 insertions(+)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 120a995..251cc33 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -34,6 +34,11 @@ static void f2fs_read_end_io(struct bio *bio)
 	struct bio_vec *bvec;
 	int i;
 
+#ifdef CONFIG_F2FS_FAULT_INJECTION
+	if (time_to_inject(FAULT_IO))
+		bio->bi_error = -EIO;
+#endif
+
 	if (f2fs_bio_encrypted(bio)) {
 		if (bio->bi_error) {
 			fscrypt_release_ctx(bio->bi_private);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b615f3f..a1a68bf 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -46,6 +46,7 @@ enum {
 	FAULT_BLOCK,
 	FAULT_DIR_DEPTH,
 	FAULT_EVICT_INODE,
+	FAULT_IO,
 	FAULT_MAX,
 };
 
@@ -77,6 +78,8 @@ static inline bool time_to_inject(int type)
 		return false;
 	else if (type == FAULT_EVICT_INODE && !IS_FAULT_SET(type))
 		return false;
+	else if (type == FAULT_IO && !IS_FAULT_SET(type))
+		return false;
 
 	atomic_inc(&f2fs_fault.inject_ops);
 	if (atomic_read(&f2fs_fault.inject_ops) >= f2fs_fault.inject_rate) {
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index f809729..fafa34e 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -50,6 +50,7 @@ char *fault_name[FAULT_MAX] = {
 	[FAULT_BLOCK]		= "no more block",
 	[FAULT_DIR_DEPTH]	= "too big dir depth",
 	[FAULT_EVICT_INODE]	= "evict_inode fail",
+	[FAULT_IO]		= "IO error",
 };
 
 static void f2fs_build_fault_attr(unsigned int rate)
-- 
2.7.2


------------------------------------------------------------------------------

[PATCH 6/6] f2fs: show dirty inode number

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

This patch enables showing dirty inode number in procfs.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/debug.c | 3 +++
 fs/f2fs/f2fs.h  | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
index ae13521..fb245bd 100644
--- a/fs/f2fs/debug.c
+++ b/fs/f2fs/debug.c
@@ -45,6 +45,7 @@ static void update_general_status(struct f2fs_sb_info *sbi)
 	si->ndirty_dent = get_pages(sbi, F2FS_DIRTY_DENTS);
 	si->ndirty_meta = get_pages(sbi, F2FS_DIRTY_META);
 	si->ndirty_data = get_pages(sbi, F2FS_DIRTY_DATA);
+	si->ndirty_imeta = get_pages(sbi, F2FS_DIRTY_IMETA);
 	si->ndirty_dirs = sbi->ndirty_inode[DIR_INODE];
 	si->ndirty_files = sbi->ndirty_inode[FILE_INODE];
 	si->ndirty_all = sbi->ndirty_inode[DIRTY_META];
@@ -319,6 +320,8 @@ static int stat_show(struct seq_file *s, void *v)
 			   si->ndirty_data, si->ndirty_files);
 		seq_printf(s, "  - meta: %4lld in %4d\n",
 			   si->ndirty_meta, si->meta_pages);
+		seq_printf(s, "  - imeta: %4lld\n",
+			   si->ndirty_imeta);
 		seq_printf(s, "  - NATs: %9d/%9d\n  - SITs: %9d/%9d\n",
 			   si->dirty_nats, si->nats, si->dirty_sits, si->sits);
 		seq_printf(s, "  - free_nids: %9d\n",
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index a1a68bf..be23825 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2191,7 +2191,8 @@ struct f2fs_stat_info {
 	unsigned long long hit_largest, hit_cached, hit_rbtree;
 	unsigned long long hit_total, total_ext;
 	int ext_tree, zombie_tree, ext_node;
-	s64 ndirty_node, ndirty_dent, ndirty_meta, ndirty_data, inmem_pages;
+	s64 ndirty_node, ndirty_dent, ndirty_meta, ndirty_data, ndirty_imeta;
+	s64 inmem_pages;
 	unsigned int ndirty_dirs, ndirty_files, ndirty_all;
 	int nats, dirty_nats, sits, dirty_sits, fnids;
 	int total_count, utilization;
-- 
2.7.2

[PATCH 6/6] f2fs: show dirty inode number

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

This patch enables showing dirty inode number in procfs.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/debug.c | 3 +++
 fs/f2fs/f2fs.h  | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
index ae13521..fb245bd 100644
--- a/fs/f2fs/debug.c
+++ b/fs/f2fs/debug.c
@@ -45,6 +45,7 @@ static void update_general_status(struct f2fs_sb_info *sbi)
 	si->ndirty_dent = get_pages(sbi, F2FS_DIRTY_DENTS);
 	si->ndirty_meta = get_pages(sbi, F2FS_DIRTY_META);
 	si->ndirty_data = get_pages(sbi, F2FS_DIRTY_DATA);
+	si->ndirty_imeta = get_pages(sbi, F2FS_DIRTY_IMETA);
 	si->ndirty_dirs = sbi->ndirty_inode[DIR_INODE];
 	si->ndirty_files = sbi->ndirty_inode[FILE_INODE];
 	si->ndirty_all = sbi->ndirty_inode[DIRTY_META];
@@ -319,6 +320,8 @@ static int stat_show(struct seq_file *s, void *v)
 			   si->ndirty_data, si->ndirty_files);
 		seq_printf(s, "  - meta: %4lld in %4d\n",
 			   si->ndirty_meta, si->meta_pages);
+		seq_printf(s, "  - imeta: %4lld\n",
+			   si->ndirty_imeta);
 		seq_printf(s, "  - NATs: %9d/%9d\n  - SITs: %9d/%9d\n",
 			   si->dirty_nats, si->nats, si->dirty_sits, si->sits);
 		seq_printf(s, "  - free_nids: %9d\n",
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index a1a68bf..be23825 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2191,7 +2191,8 @@ struct f2fs_stat_info {
 	unsigned long long hit_largest, hit_cached, hit_rbtree;
 	unsigned long long hit_total, total_ext;
 	int ext_tree, zombie_tree, ext_node;
-	s64 ndirty_node, ndirty_dent, ndirty_meta, ndirty_data, inmem_pages;
+	s64 ndirty_node, ndirty_dent, ndirty_meta, ndirty_data, ndirty_imeta;
+	s64 inmem_pages;
 	unsigned int ndirty_dirs, ndirty_files, ndirty_all;
 	int nats, dirty_nats, sits, dirty_sits, fnids;
 	int total_count, utilization;
-- 
2.7.2


------------------------------------------------------------------------------

Re: [PATCH 3/6] f2fs: fix to avoid race condition when updating sbi flag

[Jaegeuk Kim]

Hi Chao,

On Sun, Sep 18, 2016 at 11:30:05PM +0800, Chao Yu wrote:
> From: Chao Yu <yuchao0@huawei.com>
> 
> Making updating of sbi flag atomic by using {test,set,clear}_bit,
> otherwise in concurrency scenario, the flag could be updated incorrectly.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/f2fs.h | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 9b4bbf2..c30f744b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -794,7 +794,7 @@ struct f2fs_sb_info {
>  	struct proc_dir_entry *s_proc;		/* proc entry */
>  	struct f2fs_super_block *raw_super;	/* raw super block pointer */
>  	int valid_super_block;			/* valid super block no */
> -	int s_flag;				/* flags for sbi */
> +	unsigned long s_flag;				/* flags for sbi */
>  
>  #ifdef CONFIG_F2FS_FS_ENCRYPTION
>  	u8 key_prefix[F2FS_KEY_DESC_PREFIX_SIZE];
> @@ -1063,17 +1063,19 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>  
>  static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>  {
> -	return sbi->s_flag & (0x01 << type);
> +	return test_bit(type, &sbi->s_flag);
>  }
>  
>  static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>  {
> -	sbi->s_flag |= (0x01 << type);
> +	if (!test_bit(type, &sbi->s_flag))
> +		set_bit(type, &sbi->s_flag);

The set_bit() is enough, no?

>  }
>  
>  static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>  {
> -	sbi->s_flag &= ~(0x01 << type);
> +	if (test_bit(type, &sbi->s_flag))
> +		clear_bit(type, &sbi->s_flag);

ditto.

Thanks,

>  }
>  
>  static inline unsigned long long cur_cp_version(struct f2fs_checkpoint *cp)
> -- 
> 2.7.2

Re: [PATCH 4/6] f2fs: introduce cp_lock to protect updating of ckpt_flags

[Jaegeuk Kim]

Hi Chao,

On Sun, Sep 18, 2016 at 11:30:06PM +0800, Chao Yu wrote:
> From: Chao Yu <yuchao0@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 <yuchao0@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

Re: [PATCH 4/6] f2fs: introduce cp_lock to protect updating of ckpt_flags

[Jaegeuk Kim]

Hi Chao,

On Sun, Sep 18, 2016 at 11:30:06PM +0800, Chao Yu wrote:
> From: Chao Yu <yuchao0@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 <yuchao0@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

------------------------------------------------------------------------------

Re: [PATCH 3/6] f2fs: fix to avoid race condition when updating sbi flag

[Chao Yu]

Hi Jaegeuk,

On 2016/9/20 5:40, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On Sun, Sep 18, 2016 at 11:30:05PM +0800, Chao Yu wrote:
>> From: Chao Yu <yuchao0@huawei.com>
>>
>> Making updating of sbi flag atomic by using {test,set,clear}_bit,
>> otherwise in concurrency scenario, the flag could be updated incorrectly.
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  fs/f2fs/f2fs.h | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 9b4bbf2..c30f744b 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -794,7 +794,7 @@ struct f2fs_sb_info {
>>  	struct proc_dir_entry *s_proc;		/* proc entry */
>>  	struct f2fs_super_block *raw_super;	/* raw super block pointer */
>>  	int valid_super_block;			/* valid super block no */
>> -	int s_flag;				/* flags for sbi */
>> +	unsigned long s_flag;				/* flags for sbi */
>>  
>>  #ifdef CONFIG_F2FS_FS_ENCRYPTION
>>  	u8 key_prefix[F2FS_KEY_DESC_PREFIX_SIZE];
>> @@ -1063,17 +1063,19 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>>  
>>  static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>  {
>> -	return sbi->s_flag & (0x01 << type);
>> +	return test_bit(type, &sbi->s_flag);
>>  }
>>  
>>  static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>  {
>> -	sbi->s_flag |= (0x01 << type);
>> +	if (!test_bit(type, &sbi->s_flag))
>> +		set_bit(type, &sbi->s_flag);
> 
> The set_bit() is enough, no?

It seems OK to me, let me send v2.

Thanks,

> 
>>  }
>>  
>>  static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>  {
>> -	sbi->s_flag &= ~(0x01 << type);
>> +	if (test_bit(type, &sbi->s_flag))
>> +		clear_bit(type, &sbi->s_flag);
> 
> ditto.
> 
> Thanks,
> 
>>  }
>>  
>>  static inline unsigned long long cur_cp_version(struct f2fs_checkpoint *cp)
>> -- 
>> 2.7.2
> 
> .
> 

Re: [PATCH 3/6] f2fs: fix to avoid race condition when updating sbi flag

[Chao Yu]

Hi Jaegeuk,

On 2016/9/20 5:40, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On Sun, Sep 18, 2016 at 11:30:05PM +0800, Chao Yu wrote:
>> From: Chao Yu <yuchao0@huawei.com>
>>
>> Making updating of sbi flag atomic by using {test,set,clear}_bit,
>> otherwise in concurrency scenario, the flag could be updated incorrectly.
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  fs/f2fs/f2fs.h | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 9b4bbf2..c30f744b 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -794,7 +794,7 @@ struct f2fs_sb_info {
>>  	struct proc_dir_entry *s_proc;		/* proc entry */
>>  	struct f2fs_super_block *raw_super;	/* raw super block pointer */
>>  	int valid_super_block;			/* valid super block no */
>> -	int s_flag;				/* flags for sbi */
>> +	unsigned long s_flag;				/* flags for sbi */
>>  
>>  #ifdef CONFIG_F2FS_FS_ENCRYPTION
>>  	u8 key_prefix[F2FS_KEY_DESC_PREFIX_SIZE];
>> @@ -1063,17 +1063,19 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>>  
>>  static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>  {
>> -	return sbi->s_flag & (0x01 << type);
>> +	return test_bit(type, &sbi->s_flag);
>>  }
>>  
>>  static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>  {
>> -	sbi->s_flag |= (0x01 << type);
>> +	if (!test_bit(type, &sbi->s_flag))
>> +		set_bit(type, &sbi->s_flag);
> 
> The set_bit() is enough, no?

It seems OK to me, let me send v2.

Thanks,

> 
>>  }
>>  
>>  static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>  {
>> -	sbi->s_flag &= ~(0x01 << type);
>> +	if (test_bit(type, &sbi->s_flag))
>> +		clear_bit(type, &sbi->s_flag);
> 
> ditto.
> 
> Thanks,
> 
>>  }
>>  
>>  static inline unsigned long long cur_cp_version(struct f2fs_checkpoint *cp)
>> -- 
>> 2.7.2
> 
> .
> 

Re: [PATCH 4/6] f2fs: introduce cp_lock to protect updating of ckpt_flags

[Chao Yu]

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

Re: [PATCH 4/6] f2fs: introduce cp_lock to protect updating of ckpt_flags

[Chao Yu]

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

Re: [PATCH 4/6] f2fs: introduce cp_lock to protect updating of ckpt_flags

[Jaegeuk Kim]

On Tue, Sep 20, 2016 at 09:47:20AM +0800, Chao Yu wrote:
> 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 <yuchao0@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?

Okay, I see. Let me try to understand in more details.
Basically, there'd be no problem if there is no f2fs_stop_checkpoint(), since
every {set|clear}_ckpt_flags() are called under f2fs_lock_op() or in
fill_super(). And, you're probably concerned about any breakage of ckpt->flags
due to accidental f2fs_stop_checkpoint() trigger. So, we're able to lose
ERROR_FLAG because of data race.

Oh, I found one potential corruption case in f2fs_write_checkpoint().
Before writing the last checkpoint block, we used to check its IO error.
But, if set_ckpt_flags() and f2fs_stop_checkpoint() were called concurrently,
ckpt_flags was able to lose ERROR_FLAG, resulting in finalizing its checkpoint
pack. So, we can get valid checkpoint pack with EIO'ed metadata block.

BTW, we can do multiple set/clear flags in do_checkpoint() with single spin_lock
call, tho.

Thanks,

> > 
> >> Signed-off-by: Chao Yu <yuchao0@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
> > 
> > .
> > 

Re: [PATCH 4/6] f2fs: introduce cp_lock to protect updating of ckpt_flags

[Chao Yu]

On 2016/9/20 10:41, Jaegeuk Kim wrote:
> On Tue, Sep 20, 2016 at 09:47:20AM +0800, Chao Yu wrote:
>> 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 <yuchao0@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?
> 
> Okay, I see. Let me try to understand in more details.
> Basically, there'd be no problem if there is no f2fs_stop_checkpoint(), since
> every {set|clear}_ckpt_flags() are called under f2fs_lock_op() or in
> fill_super(). And, you're probably concerned about any breakage of ckpt->flags
> due to accidental f2fs_stop_checkpoint() trigger. So, we're able to lose
> ERROR_FLAG because of data race.
> 
> Oh, I found one potential corruption case in f2fs_write_checkpoint().
> Before writing the last checkpoint block, we used to check its IO error.
> But, if set_ckpt_flags() and f2fs_stop_checkpoint() were called concurrently,
> ckpt_flags was able to lose ERROR_FLAG, resulting in finalizing its checkpoint
> pack. So, we can get valid checkpoint pack with EIO'ed metadata block.

That's right.

> 
> BTW, we can do multiple set/clear flags in do_checkpoint() with single spin_lock
> call, tho.

Agree, let me refactor the code. :)

Thanks,

> 
> Thanks,
> 
>>>
>>>> Signed-off-by: Chao Yu <yuchao0@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
>>>
>>> .
>>>
> 
> .
> 

Re: [PATCH 4/6] f2fs: introduce cp_lock to protect updating of ckpt_flags

[Chao Yu]

On 2016/9/20 10:41, Jaegeuk Kim wrote:
> On Tue, Sep 20, 2016 at 09:47:20AM +0800, Chao Yu wrote:
>> 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 <yuchao0@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?
> 
> Okay, I see. Let me try to understand in more details.
> Basically, there'd be no problem if there is no f2fs_stop_checkpoint(), since
> every {set|clear}_ckpt_flags() are called under f2fs_lock_op() or in
> fill_super(). And, you're probably concerned about any breakage of ckpt->flags
> due to accidental f2fs_stop_checkpoint() trigger. So, we're able to lose
> ERROR_FLAG because of data race.
> 
> Oh, I found one potential corruption case in f2fs_write_checkpoint().
> Before writing the last checkpoint block, we used to check its IO error.
> But, if set_ckpt_flags() and f2fs_stop_checkpoint() were called concurrently,
> ckpt_flags was able to lose ERROR_FLAG, resulting in finalizing its checkpoint
> pack. So, we can get valid checkpoint pack with EIO'ed metadata block.

That's right.

> 
> BTW, we can do multiple set/clear flags in do_checkpoint() with single spin_lock
> call, tho.

Agree, let me refactor the code. :)

Thanks,

> 
> Thanks,
> 
>>>
>>>> Signed-off-by: Chao Yu <yuchao0@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
>>>
>>> .
>>>
> 
> .
>