f2fs: make f2fs_filetype_table static

PATCH In review Chao Yu 3 participants 12 messages Sep 18, 2016 – Sep 20, 2016 F2FS file system

lore

Share & tools

lore

Chao Yu

author maintainer

2016-09-18 15:30 UTC #0 lore raw

PATCH 1/6 f2fs: make f2fs_filetype_table static

From: Chao Yu <[email protected]>

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

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/dir.c  | 2 +-
 fs/f2fs/f2fs.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

fs/f2fs/dir.c +1 -1

index 2fb20fc..39a850b 100644
@@ -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,

fs/f2fs/f2fs.h +0 -1

index af06303..9b4bbf2 100644
@@ -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 *,

Signature · 1 line

2.7.2

Chao Yu

author maintainer

2016-09-18 15:32 UTC #1 lore raw

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

From: Chao Yu <[email protected]>

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 <[email protected]>

---
 fs/f2fs/xattr.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

fs/f2fs/xattr.c +22 ~~-15~~

index 1660191..83234dd 100644
@@ -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);

Signature · 4 lines

2.7.2


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

Chao Yu

author maintainer

2016-09-18 15:32 UTC #2 lore raw

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

In reply to Chao Yu

From: Chao Yu <[email protected]>

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 <[email protected]>

---
 fs/f2fs/f2fs.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

fs/f2fs/f2fs.h +6 -4

index 9b4bbf2..c30f744b 100644
@@ -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)

Signature · 4 lines

2.7.2


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

Jaegeuk Kim

maintainer

2016-09-19 21:40 UTC #3 lore raw

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

Hi Chao,

Quoted 26 lines · Chao Yu

From: Chao Yu <[email protected]>

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 <[email protected]>

---
 fs/f2fs/f2fs.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

fs/f2fs/f2fs.h +1 -1

index 9b4bbf2..c30f744b 100644
@@ -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)

fs/f2fs/f2fs.h static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi) · line 1072

Quoted 10 lines

fs/f2fs/f2fs.h +3 -2

 {
-	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?

fs/f2fs/f2fs.h static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi) · line 1078

Quoted 7 lines

fs/f2fs/f2fs.h +2 -1

 }
 
 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,

Quoted 5 lines

fs/f2fs/f2fs.h +0 -0

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

Signature · 1 line

2.7.2

Chao Yu

2016-09-20 01:41 UTC #4 lore raw

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

Hi Jaegeuk,

Quoted 34 lines · Jaegeuk Kim

Hi Chao,

Quoted 31 lines · Chao Yu

From: Chao Yu <[email protected]>

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 <[email protected]>

---
 fs/f2fs/f2fs.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

fs/f2fs/f2fs.h +2 -2

index 9b4bbf2..c30f744b 100644
@@ -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);
 }

Quoted 7 lines

fs/f2fs/f2fs.h static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi) · line 1072
Quoted 5 lines
fs/f2fs/f2fs.h +2 -1
 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,

Quoted 21 lines

fs/f2fs/f2fs.h static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi) · line 1078
Quoted 8 lines
fs/f2fs/f2fs.h +2 -1
 }
 
 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,
Quoted 5 lines
fs/f2fs/f2fs.h +0 -0
 }
 
 static inline unsigned long long cur_cp_version(struct f2fs_checkpoint *cp)
Signature · 1 line
2.7.2
.

Chao Yu

author maintainer

2016-09-18 15:32 UTC #5 lore raw

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

In reply to Chao Yu

From: Chao Yu <[email protected]>

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 <[email protected]>

---
 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(-)

fs/f2fs/checkpoint.c +12 ~~-12~~

index df56a43..0338f8c 100644
@@ -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));

fs/f2fs/f2fs.h +21 -8

index c30f744b..b615f3f 100644
@@ -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)

fs/f2fs/recovery.c +1 -1

index ad748e5..37d99d2 100644
@@ -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 = {

fs/f2fs/segment.c +2 -2

index 101b58f..dc68f30 100644
@@ -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);

fs/f2fs/super.c +3 -2

index 555217f..f809729 100644
@@ -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;
 		}

Signature · 4 lines

2.7.2


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

Jaegeuk Kim

maintainer

2016-09-19 21:49 UTC #6 lore raw

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

Hi Chao,

Quoted 5 lines · Chao Yu

From: Chao Yu <[email protected]>
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,

Quoted 223 lines

Signed-off-by Chao Yu <[email protected]>

---
 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(-)

fs/f2fs/checkpoint.c +12 ~~-12~~

index df56a43..0338f8c 100644
@@ -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));

fs/f2fs/f2fs.h +21 -8

index c30f744b..b615f3f 100644
@@ -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)

fs/f2fs/recovery.c +1 -1

index ad748e5..37d99d2 100644
@@ -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 = {

fs/f2fs/segment.c +2 -2

index 101b58f..dc68f30 100644
@@ -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);

fs/f2fs/super.c +3 -2

index 555217f..f809729 100644
@@ -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;
 		}

Signature · 1 line

2.7.2

Chao Yu

2016-09-20 01:47 UTC #7 lore raw

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

Hi Jaegeuk,

Quoted 5 lines · Jaegeuk Kim

Hi Chao,
Quoted 2 lines · Chao Yu
From: Chao Yu <[email protected]>

Quoted 8 lines

Quoted 3 lines
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,

Quoted 229 lines

Thanks,

Quoted 223 lines

Signed-off-by Chao Yu <[email protected]>

---
 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(-)

fs/f2fs/checkpoint.c +12 ~~-12~~

index df56a43..0338f8c 100644
@@ -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));

fs/f2fs/f2fs.h +21 -8

index c30f744b..b615f3f 100644
@@ -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)

fs/f2fs/recovery.c +1 -1

index ad748e5..37d99d2 100644
@@ -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 = {

fs/f2fs/segment.c +2 -2

index 101b58f..dc68f30 100644
@@ -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);

fs/f2fs/super.c +3 -2

index 555217f..f809729 100644
@@ -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;
 		}

Signature · 1 line

2.7.2

Jaegeuk Kim

maintainer

2016-09-20 02:41 UTC #8 lore raw

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

Quoted 32 lines · Chao Yu

Hi Jaegeuk,
Quoted 5 lines · Jaegeuk Kim
Hi Chao,
Quoted 2 lines · Chao Yu
From: Chao Yu <[email protected]>
Quoted 8 lines
Quoted 3 lines
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

Quoted 1 line

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,

Quoted 227 lines

Quoted 224 lines

Signed-off-by Chao Yu <[email protected]>

---
 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(-)

fs/f2fs/checkpoint.c +12 ~~-12~~

index df56a43..0338f8c 100644
@@ -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));

fs/f2fs/f2fs.h +21 -8

index c30f744b..b615f3f 100644
@@ -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)

fs/f2fs/recovery.c +1 -1

index ad748e5..37d99d2 100644
@@ -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 = {

fs/f2fs/segment.c +2 -2

index 101b58f..dc68f30 100644
@@ -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);

fs/f2fs/super.c +3 -2

index 555217f..f809729 100644
@@ -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;
 		}

Signature · 1 line

2.7.2

Chao Yu

2016-09-20 02:50 UTC #9 lore raw

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

Quoted 42 lines · Jaegeuk Kim

Quoted 32 lines · Chao Yu
Hi Jaegeuk,
Quoted 5 lines · Jaegeuk Kim
Hi Chao,
Quoted 2 lines · Chao Yu
From: Chao Yu <[email protected]>
Quoted 8 lines
Quoted 3 lines
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
Quoted 1 line
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.

Quoted 5 lines

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.

Quoted 3 lines

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

Agree, let me refactor the code. :)

Thanks,

Quoted 233 lines

Thanks,

Quoted 211 lines

Signed-off-by Chao Yu <[email protected]>

---
 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(-)

fs/f2fs/checkpoint.c +12 ~~-12~~

index df56a43..0338f8c 100644
@@ -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));

fs/f2fs/f2fs.h +21 -8

index c30f744b..b615f3f 100644
@@ -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)

fs/f2fs/recovery.c +1 -1

index ad748e5..37d99d2 100644
@@ -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 = {

fs/f2fs/segment.c +2 -2

index 101b58f..dc68f30 100644
@@ -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);

fs/f2fs/super.c +2 -1

index 555217f..f809729 100644
@@ -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);

Quoted 16 lines

Quoted 7 lines

fs/f2fs/super.c +0 -1

 #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)) {

Quoted 9 lines

Quoted 6 lines

fs/f2fs/super.c +1 -0

+				!is_set_ckpt_flags(sbi, CP_UMOUNT_FLAG)) {
 			err = -EROFS;
 			goto free_kobj;
 		}

Signature · 1 line

2.7.2

Chao Yu

author maintainer

2016-09-18 15:32 UTC #10 lore raw

PATCH 5/6 f2fs: support IO error injection

In reply to Chao Yu

From: Chao Yu <[email protected]>

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

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/data.c  | 5 +++++
 fs/f2fs/f2fs.h  | 3 +++
 fs/f2fs/super.c | 1 +
 3 files changed, 9 insertions(+)

fs/f2fs/data.c +5 -0

index 120a995..251cc33 100644
@@ -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);

fs/f2fs/f2fs.h +3 -0

index b615f3f..a1a68bf 100644
@@ -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) {

fs/f2fs/super.c +1 -0

index f809729..fafa34e 100644
@@ -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)

Signature · 4 lines

2.7.2


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

Chao Yu

author maintainer

2016-09-18 15:32 UTC #11 lore raw

PATCH 6/6 f2fs: show dirty inode number

In reply to Chao Yu

From: Chao Yu <[email protected]>

This patch enables showing dirty inode number in procfs.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/debug.c | 3 +++
 fs/f2fs/f2fs.h  | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/debug.c +3 -0

index ae13521..fb245bd 100644
@@ -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",

fs/f2fs/f2fs.h +2 -1

index a1a68bf..be23825 100644
@@ -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;

Signature · 4 lines

2.7.2


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