f2fs: fix to cover sentry_lock for block allocation

PATCH In review Jaegeuk Kim 2 participants 10 messages Mar 17, 2015 – Mar 19, 2015

Subsystems (2)

VFS F2FS file system

lore

Share & tools

lore

Jaegeuk Kim

author maintainer

2015-03-17 16:57 UTC #0 lore raw

PATCH 1/4 f2fs: fix to cover sentry_lock for block allocation

In the following call stack, f2fs changes the bitmap for dirty segments and # of dirty sentries without grabbing sit_i->sentry_lock.
This can result in mismatch on bitmap and # of dirty sentries, since if there are some direct_io operations.

In allocate_data_block,
- __allocate_new_segments
- mutex_lock(&curseg->curseg_mutex);
- s_ops->allocate_segment
- new_curseg/change_curseg
- reset_curseg
- __set_sit_entry_type
- __mark_sit_entry_dirty
- set_bit(dirty_sentries_bitmap)
- dirty_sentries++;

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

---
 fs/f2fs/segment.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/f2fs/segment.c +1 -1

index 3e80bd6..eafaf72 100644
@@ -1167,6 +1167,7 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
 	curseg = CURSEG_I(sbi, type);
 
 	mutex_lock(&curseg->curseg_mutex);
+	mutex_lock(&sit_i->sentry_lock);
 
 	/* direct_io'ed data is aligned to the segment for better performance */
 	if (direct_io && curseg->next_blkoff)

@@ -1181,7 +1182,6 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
 	 */
 	__add_sum_entry(sbi, type, sum);
 
-	mutex_lock(&sit_i->sentry_lock);
 	__refresh_next_blkoff(sbi, curseg);
 
 	stat_inc_block_count(sbi, curseg);

Signature · 1 line

2.1.1

Jaegeuk Kim

author maintainer

2015-03-17 16:57 UTC #1 lore raw

PATCH 2/4 f2fs: set buffer_new when new blocks are allocated

This patch modifies to call set_buffer_new, if new blocks are allocated.

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

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

fs/f2fs/data.c +2 -2

index ba70a78..cefaa67 100644
@@ -257,7 +257,6 @@ static void f2fs_map_bh(struct super_block *sb, pgoff_t pgofs,
 	unsigned int blkbits = sb->s_blocksize_bits;
 	size_t count;
 
-	set_buffer_new(bh_result);
 	map_bh(bh_result, sb, ei->blk + pgofs - ei->fofs);
 	count = ei->fofs + ei->len - pgofs;
 	if (count < (UINT_MAX >> blkbits))

@@ -1139,7 +1138,7 @@ static int __get_data_block(struct inode *inode, sector_t iblock,
 		goto put_out;
 
 	if (dn.data_blkaddr != NULL_ADDR) {
-		set_buffer_new(bh_result);
+		clear_buffer_new(bh_result);
 		map_bh(bh_result, inode->i_sb, dn.data_blkaddr);
 	} else if (create) {
 		err = __allocate_data_block(&dn);

@@ -1184,6 +1183,7 @@ get_next:
 			if (err)
 				goto sync_out;
 			allocated = true;
+			set_buffer_new(bh_result);
 			blkaddr = dn.data_blkaddr;
 		}
 		/* Give more consecutive addresses for the readahead */

Signature · 9 lines

2.1.1


------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the 
conversation now. http://goparallel.sourceforge.net/

Chao Yu

2015-03-18 02:24 UTC #2 lore raw

RE: [f2fs-dev] [PATCH 2/4] f2fs: set buffer_new when new blocks are allocated

Hi Jaegeuk,

Quoted 28 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, March 18, 2015 12:58 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 2/4] f2fs: set buffer_new when new blocks are allocated
This patch modifies to call set_buffer_new, if new blocks are allocated.
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/data.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/f2fs/data.c +0 -1
index ba70a78..cefaa67 100644
@@ -257,7 +257,6 @@ static void f2fs_map_bh(struct super_block *sb, pgoff_t pgofs,
 	unsigned int blkbits = sb->s_blocksize_bits;
 	size_t count;
 
-	set_buffer_new(bh_result);
 	map_bh(bh_result, sb, ei->blk + pgofs - ei->fofs);
 	count = ei->fofs + ei->len - pgofs;
 	if (count < (UINT_MAX >> blkbits))

fs/f2fs/data.c static int __get_data_block(struct inode *inode, sector_t iblock, · line 1141

Quoted 5 lines

fs/f2fs/data.c +1 -1

@@ -1139,7 +1138,7 @@ static int __get_data_block(struct inode *inode, sector_t iblock,
 		goto put_out;
 
 	if (dn.data_blkaddr != NULL_ADDR) {
-		set_buffer_new(bh_result);
+		clear_buffer_new(bh_result);

Add clear_buffer_new in f2fs_map_bh?

Thanks,

Quoted 13 lines

fs/f2fs/data.c +1 -1

 		map_bh(bh_result, inode->i_sb, dn.data_blkaddr);
 	} else if (create) {
 		err = __allocate_data_block(&dn);

@@ -1184,6 +1183,7 @@ get_next:
 			if (err)
 				goto sync_out;
 			allocated = true;
+			set_buffer_new(bh_result);
 			blkaddr = dn.data_blkaddr;
 		}
 		/* Give more consecutive addresses for the readahead */
--

2.1.1

Jaegeuk Kim

author maintainer

2015-03-18 17:57 UTC #3 lore raw

Re: [f2fs-dev] [PATCH 2/4 v2] f2fs: set buffer_new when new blocks are allocated

Hi Chao,

Thanks for the review.

Change log from v1:
o add clear_buffer_new() in f2fs_map_bh

This patch modifies to call set_buffer_new, if new blocks are allocated.

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 ba70a78..4a416e7 100644
@@ -257,7 +257,7 @@ static void f2fs_map_bh(struct super_block *sb, pgoff_t pgofs,
 	unsigned int blkbits = sb->s_blocksize_bits;
 	size_t count;
 
-	set_buffer_new(bh_result);
+	clear_buffer_new(bh_result);
 	map_bh(bh_result, sb, ei->blk + pgofs - ei->fofs);
 	count = ei->fofs + ei->len - pgofs;
 	if (count < (UINT_MAX >> blkbits))

@@ -1139,7 +1139,7 @@ static int __get_data_block(struct inode *inode, sector_t iblock,
 		goto put_out;
 
 	if (dn.data_blkaddr != NULL_ADDR) {
-		set_buffer_new(bh_result);
+		clear_buffer_new(bh_result);
 		map_bh(bh_result, inode->i_sb, dn.data_blkaddr);
 	} else if (create) {
 		err = __allocate_data_block(&dn);

@@ -1184,6 +1184,7 @@ get_next:
 			if (err)
 				goto sync_out;
 			allocated = true;
+			set_buffer_new(bh_result);
 			blkaddr = dn.data_blkaddr;
 		}
 		/* Give more consecutive addresses for the readahead */

Signature · 1 line

2.1.1

Chao Yu

Reviewed-by

2015-03-19 11:18 UTC #4 lore raw

RE: [f2fs-dev] [PATCH 2/4 v2] f2fs: set buffer_new when new blocks are allocated

Quoted 10 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Thursday, March 19, 2015 1:57 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 2/4 v2] f2fs: set buffer_new when new blocks are allocated
Hi Chao,

Quoted 8 lines

Thanks for the review.
Change log from v1:
o add clear_buffer_new() in f2fs_map_bh
This patch modifies to call set_buffer_new, if new blocks are allocated.
Signed-off-by Jaegeuk Kim <[email protected]>

Reviewed-by Chao Yu <[email protected]>

Jaegeuk Kim

author maintainer

2015-03-17 16:57 UTC #5 lore raw

PATCH 3/4 f2fs: enhance multi-threads performance

In reply to Jaegeuk Kim

Previously, f2fs_write_data_pages has a mutex, sbi->writepages, to serialize data writes to maximize write bandwidth, while sacrificing multi-threads performance.
Practically, however, multi-threads environment is much more important for users. So this patch tries to remove the mutex.

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

---
 fs/f2fs/data.c  | 7 -------
 fs/f2fs/f2fs.h  | 1 -
 fs/f2fs/super.c | 1 -
 3 files changed, 9 deletions(-)

fs/f2fs/data.c +0 -7

index cefaa67..1a3d86a 100644
@@ -1396,7 +1396,6 @@ static int f2fs_write_data_pages(struct address_space *mapping,
 {
 	struct inode *inode = mapping->host;
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
-	bool locked = false;
 	int ret;
 	long diff;

@@ -1417,13 +1416,7 @@ static int f2fs_write_data_pages(struct address_space *mapping,
 
 	diff = nr_pages_to_write(sbi, DATA, wbc);
 
-	if (!S_ISDIR(inode->i_mode)) {
-		mutex_lock(&sbi->writepages);
-		locked = true;
-	}
 	ret = write_cache_pages(mapping, wbc, __f2fs_writepage, mapping);
-	if (locked)
-		mutex_unlock(&sbi->writepages);
 
 	f2fs_submit_merged_bio(sbi, DATA, WRITE);

fs/f2fs/f2fs.h +0 -1

index c1ad404..f2909c6 100644
@@ -604,7 +604,6 @@ struct f2fs_sb_info {
 	struct mutex cp_mutex;			/* checkpoint procedure lock */
 	struct rw_semaphore cp_rwsem;		/* blocking FS operations */
 	struct rw_semaphore node_write;		/* locking node writes */
-	struct mutex writepages;		/* mutex for writepages() */
 	wait_queue_head_t cp_wait;
 
 	struct inode_management im[MAX_INO_ENTRY];      /* manage inode cache */

fs/f2fs/super.c +0 -1

index 0b8a2d8..c9f4aef 100644
@@ -1027,7 +1027,6 @@ try_onemore:
 	sbi->raw_super = raw_super;
 	sbi->raw_super_buf = raw_super_buf;
 	mutex_init(&sbi->gc_mutex);
-	mutex_init(&sbi->writepages);
 	mutex_init(&sbi->cp_mutex);
 	init_rwsem(&sbi->node_write);
 	clear_sbi_flag(sbi, SBI_POR_DOING);

Signature · 9 lines

2.1.1


------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the 
conversation now. http://goparallel.sourceforge.net/

Jaegeuk Kim

author maintainer

2015-03-17 16:57 UTC #6 lore raw

PATCH 4/4 f2fs: avoid wrong f2fs_bug_on when truncating inline_data

In reply to Jaegeuk Kim

This patch removes wrong f2fs_bug_on in truncate_inline_inode.

When there is no space, it can happen a corner case where i_isze is over MAX_INLINE_SIZE while its inode is still inline_data.

The scenario is
1. write small data into file #A.
2. fill the whole partition to 100%.
3. truncate 4096 on file #A.
4. write data at 8192 offset.
--> f2fs_write_begin

    -> -ENOSPC = f2fs_convert_inline_page
    -> f2fs_write_failed
      -> truncate_blocks
        -> truncate_inline_inode
	  BUG_ON, since i_size is 4096.

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

---
 fs/f2fs/inline.c | 7 -------
 1 file changed, 7 deletions(-)

fs/f2fs/inline.c +0 -7

index 153c5e7..d3e0599 100644
@@ -54,13 +54,6 @@ bool truncate_inline_inode(struct page *ipage, u64 from)
 {
 	void *addr;
 
-	/*
-	 * we should never truncate inline data past max inline data size,
-	 * because we always convert inline inode to normal one before
-	 * truncating real data if new size is past max inline data size.
-	 */
-	f2fs_bug_on(F2FS_P_SB(ipage), from > MAX_INLINE_DATA);
-
 	if (from >= MAX_INLINE_DATA)
 		return false;

Signature · 1 line

2.1.1

Chao Yu

2015-03-18 01:12 UTC #7 lore raw

RE: [f2fs-dev] [PATCH 4/4] f2fs: avoid wrong f2fs_bug_on when truncating inline_data

Hi Jaegeuk,

Quoted 10 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, March 18, 2015 12:58 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 4/4] f2fs: avoid wrong f2fs_bug_on when truncating inline_data
This patch removes wrong f2fs_bug_on in truncate_inline_inode.

Quoted 7 lines

When there is no space, it can happen a corner case where i_isze is over MAX_INLINE_SIZE while its inode is still inline_data.
The scenario is
1. write small data into file #A.
2. fill the whole partition to 100%.
3. truncate 4096 on file #A.

If we truncate size over MAX_INLINE_DATA, we will convert inline data in f2fs_truncate rather than write_begin below. isn't it?

Thanks,

Quoted 45 lines

4. write data at 8192 offset.
--> f2fs_write_begin
    -> -ENOSPC = f2fs_convert_inline_page
    -> f2fs_write_failed
      -> truncate_blocks
        -> truncate_inline_inode
	  BUG_ON, since i_size is 4096.
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/inline.c | 7 -------
 1 file changed, 7 deletions(-)
fs/f2fs/inline.c +0 -8
index 153c5e7..d3e0599 100644
@@ -54,13 +54,6 @@ bool truncate_inline_inode(struct page *ipage, u64 from)
 {
 	void *addr;
 
-	/*
-	 * we should never truncate inline data past max inline data size,
-	 * because we always convert inline inode to normal one before
-	 * truncating real data if new size is past max inline data size.
-	 */
-	f2fs_bug_on(F2FS_P_SB(ipage), from > MAX_INLINE_DATA);
-
 	if (from >= MAX_INLINE_DATA)
 		return false;
 
--
2.1.1
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

author maintainer

2015-03-18 17:57 UTC #8 lore raw

Re: [f2fs-dev] [PATCH 4/4] f2fs: avoid wrong f2fs_bug_on when truncating inline_data

Hi,

Quoted 15 lines · Chao Yu

Hi Jaegeuk,
Quoted 13 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, March 18, 2015 12:58 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 4/4] f2fs: avoid wrong f2fs_bug_on when truncating inline_data
This patch removes wrong f2fs_bug_on in truncate_inline_inode.
When there is no space, it can happen a corner case where i_isze is over MAX_INLINE_SIZE while its inode is still inline_data.

Quoted 7 lines

Quoted 4 lines
The scenario is
1. write small data into file #A.
2. fill the whole partition to 100%.
3. truncate 4096 on file #A.
If we truncate size over MAX_INLINE_DATA, we will convert inline data in f2fs_truncate rather than write_begin below. isn't it?

But, it fails to convert that due to -ENOSPC. :)

Thanks,

Quoted 48 lines

Thanks,
Quoted 45 lines
4. write data at 8192 offset.
--> f2fs_write_begin
    -> -ENOSPC = f2fs_convert_inline_page
    -> f2fs_write_failed
      -> truncate_blocks
        -> truncate_inline_inode
	  BUG_ON, since i_size is 4096.
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/inline.c | 7 -------
 1 file changed, 7 deletions(-)
fs/f2fs/inline.c +0 -8
index 153c5e7..d3e0599 100644
@@ -54,13 +54,6 @@ bool truncate_inline_inode(struct page *ipage, u64 from)
 {
 	void *addr;
 
-	/*
-	 * we should never truncate inline data past max inline data size,
-	 * because we always convert inline inode to normal one before
-	 * truncating real data if new size is past max inline data size.
-	 */
-	f2fs_bug_on(F2FS_P_SB(ipage), from > MAX_INLINE_DATA);
-
 	if (from >= MAX_INLINE_DATA)
 		return false;
 
--
2.1.1
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

Reviewed-by

2015-03-19 11:19 UTC #9 lore raw

RE: [f2fs-dev] [PATCH 4/4] f2fs: avoid wrong f2fs_bug_on when truncating inline_data

Hi Jeageuk,

Quoted 31 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Thursday, March 19, 2015 1:58 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 4/4] f2fs: avoid wrong f2fs_bug_on when truncating inline_data
Hi,
Quoted 20 lines · Chao Yu
Hi Jaegeuk,
Quoted 17 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, March 18, 2015 12:58 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 4/4] f2fs: avoid wrong f2fs_bug_on when truncating inline_data
This patch removes wrong f2fs_bug_on in truncate_inline_inode.
When there is no space, it can happen a corner case where i_isze is over MAX_INLINE_SIZE while its inode is still inline_data.
The scenario is
1. write small data into file #A.
2. fill the whole partition to 100%.
3. truncate 4096 on file #A.

Quoted 4 lines

Quoted 2 lines
If we truncate size over MAX_INLINE_DATA, we will convert inline data in f2fs_truncate rather than write_begin below. isn't it?
But, it fails to convert that due to -ENOSPC. :)

That makes sense, :)

Reviewed-by Chao Yu <[email protected]>

Thank,