f2fs: avoid deadlock caused by lock order of page and lock_op

PATCH In review Jaegeuk Kim 3 participants 16 messages Jun 24, 2017 – Jul 7, 2017

Subsystems (2)

VFS F2FS file system

lore

Share & tools

lore

Jaegeuk Kim

author maintainer

2017-06-24 16:25 UTC #0 lore raw

PATCH 1/2 f2fs: avoid deadlock caused by lock order of page and lock_op

- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page

- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/f2fs/data.c +3 -2

index 7d3af48d34a9..9141bd19a902 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)

Signature · 1 line

2.13.0.rc1.294.g07d810a77f-goog

Jaegeuk Kim

author maintainer

2017-06-24 16:25 UTC #1 lore raw

PATCH 2/2 f2fs: report # of free inodes more precisely

If the partition is small, we don't need to report total # of inodes including hidden free nodes.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/super.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

fs/f2fs/super.c +11 -3

index 8e39b850bfc0..3da6fb276f8b 100644
@@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
 	block_t total_count, user_block_count, start_count, ovp_count;
+	u64 avail_node_count;
 
 	total_count = le64_to_cpu(sbi->raw_super->block_count);
 	user_block_count = sbi->user_block_count;

@@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
 	buf->f_bavail = user_block_count - valid_user_blocks(sbi);
 
-	buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
-	buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
-							buf->f_bavail);
+	avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
+
+	if (avail_node_count > user_block_count) {
+		buf->f_files = user_block_count;
+		buf->f_ffree = buf->f_bavail;
+	} else {
+		buf->f_files = avail_node_count;
+		buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
+					buf->f_bavail);
+	}
 
 	buf->f_namelen = F2FS_NAME_LEN;
 	buf->f_fsid.val[0] = (u32)id;

Signature · 1 line

2.13.0.rc1.294.g07d810a77f-goog

Chao Yu

2017-06-26 10:52 UTC #2 lore raw

Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely

Hi Jaegeuk,

Quoted 22 lines · Jaegeuk Kim

If the partition is small, we don't need to report total # of inodes including hidden free nodes.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/super.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

fs/f2fs/super.c +1 -0

index 8e39b850bfc0..3da6fb276f8b 100644
@@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
 	block_t total_count, user_block_count, start_count, ovp_count;
+	u64 avail_node_count;
 
 	total_count = le64_to_cpu(sbi->raw_super->block_count);
 	user_block_count = sbi->user_block_count;

@@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;

fs/f2fs/super.c static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf) · line 700

Quoted 10 lines

fs/f2fs/super.c +5 -3

 	buf->f_bavail = user_block_count - valid_user_blocks(sbi);
 
-	buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
-	buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
-							buf->f_bavail);
+	avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
+
+	if (avail_node_count > user_block_count) {
+		buf->f_files = user_block_count;
+		buf->f_ffree = buf->f_bavail;

f_ffree is limited both by remained free nid count and free block count, so it needs to change like this?

if (avail_node_count > user_block_count)

	avail_node_count = user_block_count;

buf->f_files = avail_node_count;
buf->f_ffree = min(avail_node_count - valid_node_count(sbi),

					buf->f_bavail);

Thanks,

Quoted 9 lines

fs/f2fs/super.c +5 -0

+	} else {
+		buf->f_files = avail_node_count;
+		buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
+					buf->f_bavail);
+	}
 
 	buf->f_namelen = F2FS_NAME_LEN;
 	buf->f_fsid.val[0] = (u32)id;

Jaegeuk Kim

author maintainer

2017-06-26 14:58 UTC #3 lore raw

Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely

Quoted 27 lines · Chao Yu

Hi Jaegeuk,

Quoted 24 lines · Jaegeuk Kim

If the partition is small, we don't need to report total # of inodes including hidden free nodes.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/super.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

fs/f2fs/super.c +1 -0

index 8e39b850bfc0..3da6fb276f8b 100644
@@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
 	block_t total_count, user_block_count, start_count, ovp_count;
+	u64 avail_node_count;
 
 	total_count = le64_to_cpu(sbi->raw_super->block_count);
 	user_block_count = sbi->user_block_count;

@@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
 	buf->f_bavail = user_block_count - valid_user_blocks(sbi);

Quoted 11 lines

fs/f2fs/super.c static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf) · line 700
Quoted 8 lines
fs/f2fs/super.c +5 -3
-	buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
-	buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
-							buf->f_bavail);
+	avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
+
+	if (avail_node_count > user_block_count) {
+		buf->f_files = user_block_count;
+		buf->f_ffree = buf->f_bavail;
f_ffree is limited both by remained free nid count and free block count, so it needs to change like this?

I thought both of them are same, since node block will consume user block. So, we don't need to do min() again.

Quoted 19 lines

if (avail_node_count > user_block_count)
	avail_node_count = user_block_count;
buf->f_files = avail_node_count;
buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
					buf->f_bavail);
Thanks,
Quoted 9 lines
fs/f2fs/super.c +5 -0
+	} else {
+		buf->f_files = avail_node_count;
+		buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
+					buf->f_bavail);
+	}
 
 	buf->f_namelen = F2FS_NAME_LEN;
 	buf->f_fsid.val[0] = (u32)id;

Chao Yu

maintainer

2017-06-28 13:01 UTC #4 lore raw

Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely

Quoted 40 lines · Jaegeuk Kim

Quoted 27 lines · Chao Yu

Hi Jaegeuk,

Quoted 24 lines · Jaegeuk Kim

If the partition is small, we don't need to report total # of inodes including hidden free nodes.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/super.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

fs/f2fs/super.c +1 -0

index 8e39b850bfc0..3da6fb276f8b 100644
@@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
 	block_t total_count, user_block_count, start_count, ovp_count;
+	u64 avail_node_count;
 
 	total_count = le64_to_cpu(sbi->raw_super->block_count);
 	user_block_count = sbi->user_block_count;

@@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
 	buf->f_bavail = user_block_count - valid_user_blocks(sbi);

Quoted 11 lines

fs/f2fs/super.c static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf) · line 700
Quoted 8 lines
fs/f2fs/super.c +5 -3
-	buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
-	buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
-							buf->f_bavail);
+	avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
+
+	if (avail_node_count > user_block_count) {
+		buf->f_files = user_block_count;
+		buf->f_ffree = buf->f_bavail;
f_ffree is limited both by remained free nid count and free block count, so it needs to change like this?

Quoted 2 lines

I thought both of them are same, since node block will consume user block. So, we don't need to do min() again.

avail_node_count comes from total free nid counts which is limited with nid_bitmap size, buf->f_bavail comes from total user block count which can both cosumed by node and data. So the value of them may not be the same.

Thanks,

Quoted 29 lines

Quoted 17 lines
if (avail_node_count > user_block_count)
	avail_node_count = user_block_count;
buf->f_files = avail_node_count;
buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
					buf->f_bavail);
Thanks,
Quoted 6 lines
fs/f2fs/super.c +5 -0
+	} else {
+		buf->f_files = avail_node_count;
+		buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
+					buf->f_bavail);
+	}
 
Quoted 3 lines
fs/f2fs/super.c static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf) · line 708
Quoted 3 lines
fs/f2fs/super.c +0 -0
 	buf->f_namelen = F2FS_NAME_LEN;
 	buf->f_fsid.val[0] = (u32)id;
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

author maintainer

2017-07-01 07:27 UTC #5 lore raw

Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely

Quoted 41 lines · Chao Yu

Quoted 40 lines · Jaegeuk Kim

Quoted 38 lines · Chao Yu

Hi Jaegeuk,

Quoted 22 lines · Jaegeuk Kim

If the partition is small, we don't need to report total # of inodes including hidden free nodes.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/super.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

fs/f2fs/super.c +1 -0

index 8e39b850bfc0..3da6fb276f8b 100644
@@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
 	block_t total_count, user_block_count, start_count, ovp_count;
+	u64 avail_node_count;
 
 	total_count = le64_to_cpu(sbi->raw_super->block_count);
 	user_block_count = sbi->user_block_count;

@@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;

fs/f2fs/super.c static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf) · line 700

Quoted 10 lines

fs/f2fs/super.c +5 -3

 	buf->f_bavail = user_block_count - valid_user_blocks(sbi);
 
-	buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
-	buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
-							buf->f_bavail);
+	avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
+
+	if (avail_node_count > user_block_count) {
+		buf->f_files = user_block_count;
+		buf->f_ffree = buf->f_bavail;

f_ffree is limited both by remained free nid count and free block count, so it needs to change like this?

Quoted 6 lines

Quoted 2 lines
I thought both of them are same, since node block will consume user block. So, we don't need to do min() again.
avail_node_count comes from total free nid counts which is limited with nid_bitmap size, buf->f_bavail comes from total user block count which can both cosumed by node and data. So the value of them may not be the same.

What I mean was, if avail_node_count is larger than user_block_count, we can see buf->f_bavail is always smaller than avali_node_count - valid_node_count, since node blocks concumes blocks as well.

Quoted 32 lines

Thanks,
Quoted 29 lines
Quoted 17 lines
if (avail_node_count > user_block_count)
	avail_node_count = user_block_count;
buf->f_files = avail_node_count;
buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
					buf->f_bavail);
Thanks,
Quoted 6 lines
fs/f2fs/super.c +5 -0
+	} else {
+		buf->f_files = avail_node_count;
+		buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
+					buf->f_bavail);
+	}
 
Quoted 3 lines
fs/f2fs/super.c static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf) · line 708
Quoted 3 lines
fs/f2fs/super.c +0 -0
 	buf->f_namelen = F2FS_NAME_LEN;
 	buf->f_fsid.val[0] = (u32)id;
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

maintainer Reviewed-by

2017-07-01 08:28 UTC #6 lore raw

Re: [f2fs-dev] [PATCH 2/2] f2fs: report # of free inodes more precisely

Quoted 45 lines · Jaegeuk Kim

Quoted 44 lines · Chao Yu
Quoted 42 lines · Jaegeuk Kim
Quoted 27 lines · Chao Yu
Hi Jaegeuk,
Quoted 24 lines · Jaegeuk Kim
If the partition is small, we don't need to report total # of inodes including hidden free nodes.
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/super.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
fs/f2fs/super.c +1 -0
index 8e39b850bfc0..3da6fb276f8b 100644
@@ -680,6 +680,7 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	u64 id = huge_encode_dev(sb->s_bdev->bd_dev);
 	block_t total_count, user_block_count, start_count, ovp_count;
+	u64 avail_node_count;
 
 	total_count = le64_to_cpu(sbi->raw_super->block_count);
 	user_block_count = sbi->user_block_count;
@@ -692,9 +693,16 @@ static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_bfree = user_block_count - valid_user_blocks(sbi) + ovp_count;
 	buf->f_bavail = user_block_count - valid_user_blocks(sbi);
 
Quoted 11 lines
fs/f2fs/super.c static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf) · line 700
Quoted 8 lines
fs/f2fs/super.c +5 -3
-	buf->f_files = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
-	buf->f_ffree = min(buf->f_files - valid_node_count(sbi),
-							buf->f_bavail);
+	avail_node_count = sbi->total_node_count - F2FS_RESERVED_NODE_NUM;
+
+	if (avail_node_count > user_block_count) {
+		buf->f_files = user_block_count;
+		buf->f_ffree = buf->f_bavail;
f_ffree is limited both by remained free nid count and free block count, so it needs to change like this?
I thought both of them are same, since node block will consume user block. So, we don't need to do min() again.

Quoted 7 lines

Quoted 3 lines
avail_node_count comes from total free nid counts which is limited with nid_bitmap size, buf->f_bavail comes from total user block count which can both cosumed by node and data. So the value of them may not be the same.
What I mean was, if avail_node_count is larger than user_block_count, we can see buf->f_bavail is always smaller than avali_node_count - valid_node_count, since node blocks concumes blocks as well.

Got you. :)

Reviewed-by Chao Yu <[email protected]>

Thanks,

Quoted 33 lines

Quoted 33 lines
Thanks,
Quoted 29 lines
Quoted 17 lines
if (avail_node_count > user_block_count)
	avail_node_count = user_block_count;
buf->f_files = avail_node_count;
buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
					buf->f_bavail);
Thanks,
Quoted 6 lines
fs/f2fs/super.c +5 -0
+	} else {
+		buf->f_files = avail_node_count;
+		buf->f_ffree = min(avail_node_count - valid_node_count(sbi),
+					buf->f_bavail);
+	}
 
Quoted 3 lines
fs/f2fs/super.c static int f2fs_statfs(struct dentry *dentry, struct kstatfs *buf) · line 708
Quoted 3 lines
fs/f2fs/super.c +0 -0
 	buf->f_namelen = F2FS_NAME_LEN;
 	buf->f_fsid.val[0] = (u32)id;
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

2017-06-26 08:15 UTC #7 lore raw

In reply to Jaegeuk Kim

Hi Jaegeuk,

Quoted 10 lines · Jaegeuk Kim

- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op

Good catch!

With this implementation, page writeback can fail due to concurrent checkpoint, this will make fsync/atomic_commit which trigger synchronous write failed randomly.

How about unifying the lock order in punch_hole as one in writepages for regular inode? We can add one more parameter in get_new_data_page to indicate whether callee needs to lock cp_rwsem.

Thanks,

Quoted 23 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/f2fs/data.c +3 -2

index 7d3af48d34a9..9141bd19a902 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)

Jaegeuk Kim

author maintainer

2017-06-26 14:54 UTC #8 lore raw

Hi Chao,

Quoted 14 lines · Chao Yu

Hi Jaegeuk,
Quoted 10 lines · Jaegeuk Kim
- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op

Quoted 8 lines

Good catch!
With this implementation, page writeback can fail due to concurrent checkpoint, this will make fsync/atomic_commit which trigger synchronous write failed randomly.
How about unifying the lock order in punch_hole as one in writepages for regular inode? We can add one more parameter in get_new_data_page to indicate whether callee needs to lock cp_rwsem.

Currently, there would be some places to keep cp_rwsem -> page.lock, which seems not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry flushing data in f2fs_sync_file, once it gets -EAGAIN.

Any thoughts?

Quoted 26 lines

Thanks,

Quoted 23 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/f2fs/data.c +3 -2

index 7d3af48d34a9..9141bd19a902 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)

Chao Yu

maintainer

2017-06-26 15:42 UTC #9 lore raw

Hi Jaegeuk,

Quoted 26 lines · Jaegeuk Kim

Hi Chao,
Quoted 14 lines · Chao Yu
Hi Jaegeuk,
Quoted 10 lines · Jaegeuk Kim
- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op
Quoted 8 lines
Good catch!
With this implementation, page writeback can fail due to concurrent checkpoint, this will make fsync/atomic_commit which trigger synchronous write failed randomly.
How about unifying the lock order in punch_hole as one in writepages for regular inode? We can add one more parameter in get_new_data_page to indicate whether callee needs to lock cp_rwsem.

Quoted 5 lines

Currently, there would be some places to keep cp_rwsem -> page.lock, which seems not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry flushing data in f2fs_sync_file, once it gets -EAGAIN.
Any thoughts?

What about adding inode_lock in f2fs_sync_file to exclude other foreground operation which have reversed lock order? Atomic_commit is OK since it has inode_lock in its path.

Thanks,

Quoted 36 lines

Quoted 24 lines
Thanks,
Quoted 20 lines
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/f2fs/data.c +3 -2
index 7d3af48d34a9..9141bd19a902 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
Quoted 3 lines
fs/f2fs/data.c int do_write_data_page(struct f2fs_io_info *fio) · line 1412
Quoted 3 lines
fs/f2fs/data.c +0 -0
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

author maintainer

2017-07-01 07:28 UTC #10 lore raw

Quoted 33 lines · Chao Yu

Hi Jaegeuk,
Quoted 30 lines · Jaegeuk Kim
Hi Chao,
Quoted 14 lines · Chao Yu
Hi Jaegeuk,
Quoted 10 lines · Jaegeuk Kim
- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op
Quoted 8 lines
Good catch!
With this implementation, page writeback can fail due to concurrent checkpoint, this will make fsync/atomic_commit which trigger synchronous write failed randomly.
How about unifying the lock order in punch_hole as one in writepages for regular inode? We can add one more parameter in get_new_data_page to indicate whether callee needs to lock cp_rwsem.
Currently, there would be some places to keep cp_rwsem -> page.lock, which seems not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry flushing data in f2fs_sync_file, once it gets -EAGAIN.

Quoted 5 lines

Quoted 1 line
Any thoughts?
What about adding inode_lock in f2fs_sync_file to exclude other foreground operation which have reversed lock order? Atomic_commit is OK since it has inode_lock in its path.

I have concerned about performance regression, if we do that.

Quoted 39 lines

Thanks,
Quoted 36 lines
Quoted 24 lines
Thanks,
Quoted 20 lines
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/f2fs/data.c +3 -2
index 7d3af48d34a9..9141bd19a902 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
Quoted 3 lines
fs/f2fs/data.c int do_write_data_page(struct f2fs_io_info *fio) · line 1412
Quoted 3 lines
fs/f2fs/data.c +0 -0
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

maintainer

2017-07-01 08:41 UTC #11 lore raw

Quoted 34 lines · Jaegeuk Kim

Quoted 33 lines · Chao Yu
Hi Jaegeuk,
Quoted 30 lines · Jaegeuk Kim
Hi Chao,
Quoted 14 lines · Chao Yu
Hi Jaegeuk,
Quoted 10 lines · Jaegeuk Kim
- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op
Quoted 8 lines
Good catch!
With this implementation, page writeback can fail due to concurrent checkpoint, this will make fsync/atomic_commit which trigger synchronous write failed randomly.
How about unifying the lock order in punch_hole as one in writepages for regular inode? We can add one more parameter in get_new_data_page to indicate whether callee needs to lock cp_rwsem.
Currently, there would be some places to keep cp_rwsem -> page.lock, which seems not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry flushing data in f2fs_sync_file, once it gets -EAGAIN.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Any thoughts?
What about adding inode_lock in f2fs_sync_file to exclude other foreground operation which have reversed lock order? Atomic_commit is OK since it has inode_lock in its path.
I have concerned about performance regression, if we do that.

I think fsync vs write or fsync vs fsync scenarios are unusual, so is there any usecase?

Thanks,

Quoted 40 lines

Quoted 40 lines
Thanks,
Quoted 36 lines
Quoted 24 lines
Thanks,
Quoted 20 lines
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/f2fs/data.c +3 -2
index 7d3af48d34a9..9141bd19a902 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
Quoted 3 lines
fs/f2fs/data.c int do_write_data_page(struct f2fs_io_info *fio) · line 1412
Quoted 3 lines
fs/f2fs/data.c +0 -0
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

author maintainer

2017-07-01 14:27 UTC #12 lore raw

Quoted 37 lines · Chao Yu

Quoted 36 lines · Jaegeuk Kim
Quoted 35 lines · Chao Yu
Hi Jaegeuk,
Quoted 31 lines · Jaegeuk Kim
Hi Chao,
Quoted 14 lines · Chao Yu
Hi Jaegeuk,
Quoted 10 lines · Jaegeuk Kim
- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op
Quoted 8 lines
Good catch!
With this implementation, page writeback can fail due to concurrent checkpoint, this will make fsync/atomic_commit which trigger synchronous write failed randomly.
How about unifying the lock order in punch_hole as one in writepages for regular inode? We can add one more parameter in get_new_data_page to indicate whether callee needs to lock cp_rwsem.
Currently, there would be some places to keep cp_rwsem -> page.lock, which seems not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry flushing data in f2fs_sync_file, once it gets -EAGAIN.
Any thoughts?

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
What about adding inode_lock in f2fs_sync_file to exclude other foreground operation which have reversed lock order? Atomic_commit is OK since it has inode_lock in its path.
I have concerned about performance regression, if we do that.
I think fsync vs write or fsync vs fsync scenarios are unusual, so is there any usecase?

Well, that'd be common to call multiple fsync calls at the same time. Like dbench or tiotest?

Quoted 43 lines

Thanks,
Quoted 40 lines
Quoted 40 lines
Thanks,
Quoted 36 lines
Quoted 24 lines
Thanks,
Quoted 20 lines
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/f2fs/data.c +3 -2
index 7d3af48d34a9..9141bd19a902 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
Quoted 3 lines
fs/f2fs/data.c int do_write_data_page(struct f2fs_io_info *fio) · line 1412
Quoted 3 lines
fs/f2fs/data.c +0 -0
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

maintainer

2017-07-05 03:00 UTC #13 lore raw

Quoted 42 lines · Jaegeuk Kim

Quoted 41 lines · Chao Yu
Quoted 40 lines · Jaegeuk Kim
Quoted 38 lines · Chao Yu
Hi Jaegeuk,
Quoted 26 lines · Jaegeuk Kim
Hi Chao,
Quoted 14 lines · Chao Yu
Hi Jaegeuk,
Quoted 10 lines · Jaegeuk Kim
- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op
Quoted 8 lines
Good catch!
With this implementation, page writeback can fail due to concurrent checkpoint, this will make fsync/atomic_commit which trigger synchronous write failed randomly.
How about unifying the lock order in punch_hole as one in writepages for regular inode? We can add one more parameter in get_new_data_page to indicate whether callee needs to lock cp_rwsem.
Quoted 5 lines
Currently, there would be some places to keep cp_rwsem -> page.lock, which seems not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry flushing data in f2fs_sync_file, once it gets -EAGAIN.
Any thoughts?
What about adding inode_lock in f2fs_sync_file to exclude other foreground operation which have reversed lock order? Atomic_commit is OK since it has inode_lock in its path.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
I have concerned about performance regression, if we do that.
I think fsync vs write or fsync vs fsync scenarios are unusual, so is there any usecase?
Well, that'd be common to call multiple fsync calls at the same time. Like dbench or tiotest?

Do you have test numbers of dbench/tiotest with inode:lock in fsync?

Thanks,

Quoted 44 lines

Quoted 44 lines
Thanks,
Quoted 40 lines
Quoted 40 lines
Thanks,
Quoted 36 lines
Quoted 24 lines
Thanks,
Quoted 20 lines
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/f2fs/data.c +3 -2
index 7d3af48d34a9..9141bd19a902 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
Quoted 3 lines
fs/f2fs/data.c int do_write_data_page(struct f2fs_io_info *fio) · line 1412
Quoted 3 lines
fs/f2fs/data.c +0 -0
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

author maintainer

2017-07-05 03:28 UTC #14 lore raw

Quoted 45 lines · Chao Yu

Quoted 44 lines · Jaegeuk Kim
Quoted 43 lines · Chao Yu
Quoted 41 lines · Jaegeuk Kim
Quoted 33 lines · Chao Yu
Hi Jaegeuk,
Quoted 30 lines · Jaegeuk Kim
Hi Chao,
Quoted 14 lines · Chao Yu
Hi Jaegeuk,
Quoted 10 lines · Jaegeuk Kim
- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page
- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op
Quoted 8 lines
Good catch!
With this implementation, page writeback can fail due to concurrent checkpoint, this will make fsync/atomic_commit which trigger synchronous write failed randomly.
How about unifying the lock order in punch_hole as one in writepages for regular inode? We can add one more parameter in get_new_data_page to indicate whether callee needs to lock cp_rwsem.
Currently, there would be some places to keep cp_rwsem -> page.lock, which seems not simple to change the lock order with page.lock -> cp_rwsem. IMO, we can retry flushing data in f2fs_sync_file, once it gets -EAGAIN.
Quoted 5 lines
Quoted 1 line
Any thoughts?
What about adding inode_lock in f2fs_sync_file to exclude other foreground operation which have reversed lock order? Atomic_commit is OK since it has inode_lock in its path.
I have concerned about performance regression, if we do that.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
I think fsync vs write or fsync vs fsync scenarios are unusual, so is there any usecase?
Well, that'd be common to call multiple fsync calls at the same time. Like dbench or tiotest?
Do you have test numbers of dbench/tiotest with inode:lock in fsync?

No, do we need?

Quoted 47 lines

Thanks,
Quoted 44 lines
Quoted 44 lines
Thanks,
Quoted 40 lines
Quoted 40 lines
Thanks,
Quoted 36 lines
Quoted 24 lines
Thanks,
Quoted 20 lines
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/f2fs/data.c +3 -2
index 7d3af48d34a9..9141bd19a902 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
Quoted 3 lines
fs/f2fs/data.c int do_write_data_page(struct f2fs_io_info *fio) · line 1412
Quoted 3 lines
fs/f2fs/data.c +0 -0
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

author maintainer

2017-07-07 19:13 UTC #15 lore raw

- punch_hole
- fill_zero
- f2fs_lock_op
- get_new_data_page
- lock_page

- f2fs_write_data_pages
- lock_page
- do_write_data_page
- f2fs_lock_op

Signed-off-by Jaegeuk Kim <[email protected]>

---

Change log from v1:
- hide EAGAIN to users

 fs/f2fs/data.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

fs/f2fs/data.c +13 -3

index 72fc866cad19..7dd5fb647d43 100644
@@ -1404,8 +1404,9 @@ int do_write_data_page(struct f2fs_io_info *fio)
 		}
 	}
 
-	if (fio->need_lock == LOCK_REQ)
-		f2fs_lock_op(fio->sbi);
+	/* Deadlock due to between page->lock and f2fs_lock_op */
+	if (fio->need_lock == LOCK_REQ && !f2fs_trylock_op(fio->sbi))
+		return -EAGAIN;
 
 	err = get_dnode_of_data(&dn, page->index, LOOKUP_NODE);
 	if (err)

@@ -1667,7 +1668,7 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
 			}
 
 			done_index = page->index;
-
+retry_write:
 			lock_page(page);
 
 			if (unlikely(page->mapping != mapping)) {

@@ -1703,6 +1704,15 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
 					unlock_page(page);
 					ret = 0;
 					continue;
+				} else if (ret == -EAGAIN) {
+					ret = 0;
+					if (wbc->sync_mode == WB_SYNC_ALL) {
+						cond_resched();
+						congestion_wait(BLK_RW_ASYNC,
+									HZ/50);
+						goto retry_write;
+					}
+					continue;
 				}
 				done_index = page->index + 1;
 				done = 1;

Signature · 1 line

2.13.0.rc1.294.g07d810a77f-goog