f2fs: count writeback pages correctly

PATCH In review Jaegeuk Kim 2 participants 5 messages Apr 5, 2017 – Apr 10, 2017

Subsystems (3)

Stable branch VFS F2FS file system

lore

Share & tools

lore

Jaegeuk Kim

author maintainer

2017-04-05 18:02 UTC #0 lore raw

PATCH 1/2 f2fs: count writeback pages correctly

This patch fixes # of outstanding writeback pages by checking its original page.

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

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

fs/f2fs/data.c +3 -4

index 3d74c0ffa4c7..af3e849e96d2 100644
@@ -94,7 +94,6 @@ static void f2fs_write_end_io(struct bio *bio)
 
 	bio_for_each_segment_all(bvec, bio, i) {
 		struct page *page = bvec->bv_page;
-		enum count_type type = WB_DATA_TYPE(page);
 
 		if (IS_DUMMY_WRITTEN_PAGE(page)) {
 			set_page_private(page, (unsigned long)NULL);

@@ -113,7 +112,7 @@ static void f2fs_write_end_io(struct bio *bio)
 			mapping_set_error(page->mapping, -EIO);
 			f2fs_stop_checkpoint(sbi, true);
 		}
-		dec_page_count(sbi, type);
+		dec_page_count(sbi, WB_DATA_TYPE(page));
 		clear_cold_data(page);
 		end_page_writeback(page);
 	}

@@ -389,7 +388,7 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 	fio->submitted = 1;
 
 	if (!is_read)
-		inc_page_count(sbi, WB_DATA_TYPE(bio_page));
+		inc_page_count(sbi, WB_DATA_TYPE(fio->page));
 
 	down_write(&io->io_rwsem);

@@ -403,7 +402,7 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 				fio->new_blkaddr & F2FS_IO_SIZE_MASK(sbi)) {
 			err = -EAGAIN;
 			if (!is_read)
-				dec_page_count(sbi, WB_DATA_TYPE(bio_page));
+				dec_page_count(sbi, WB_DATA_TYPE(fio->page));
 			goto out_fail;
 		}
 		io->bio = __bio_alloc(sbi, fio->new_blkaddr,

Signature · 1 line

2.11.0

Jaegeuk Kim

author maintainer

2017-04-05 18:02 UTC #1 lore raw

PATCH 2/2 Revert "f2fs: put allocate_segment after refresh_sit_entry"

This reverts commit 3436c4bdb30de421d46f58c9174669fbcfd40ce0.

This makes a leak to register dirty segments. I reproduced the issue by modified postmark which injects a lot of file create/delete/update and finally triggers huge number of SSR allocations.

Cc <[email protected]> # v4.10+

[Jaegeuk Kim: Change missing incorrect comment]

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

---
 fs/f2fs/segment.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

fs/f2fs/segment.c +4 -5

index eedbed62947f..ed81fa5d64bf 100644
@@ -1893,15 +1893,14 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
 
 	stat_inc_block_count(sbi, curseg);
 
+	if (!__has_curseg_space(sbi, type))
+		sit_i->s_ops->allocate_segment(sbi, type, false);
 	/*
-	 * SIT information should be updated before segment allocation,
-	 * since SSR needs latest valid block information.
+	 * SIT information should be updated after segment allocation,
+	 * since we need to keep dirty segments precisely under SSR.
 	 */
 	refresh_sit_entry(sbi, old_blkaddr, *new_blkaddr);
 
-	if (!__has_curseg_space(sbi, type))
-		sit_i->s_ops->allocate_segment(sbi, type, false);
-
 	mutex_unlock(&sit_i->sentry_lock);
 
 	if (page && IS_NODESEG(type))

Signature · 1 line

2.11.0

Chao Yu

2017-04-06 10:30 UTC #2 lore raw

In reply to Jaegeuk Kim

Hi Jaegeuk,

Quoted 1 line · Jaegeuk Kim

This patch fixes # of outstanding writeback pages by checking its original page.

Originally, we use bio_page to track encrypted page which is been writebacked, there are two cases:
1. background flush through .writepages: encrypted_page has no mapping info, will not be tracked.
2. synchronized GC through move_encrypted_block: encrypted_page is meta page, will be tracked.

So there is no problem with old code?

Thanks,

Quoted 46 lines

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

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

fs/f2fs/data.c +3 -4

index 3d74c0ffa4c7..af3e849e96d2 100644
@@ -94,7 +94,6 @@ static void f2fs_write_end_io(struct bio *bio)
 
 	bio_for_each_segment_all(bvec, bio, i) {
 		struct page *page = bvec->bv_page;
-		enum count_type type = WB_DATA_TYPE(page);
 
 		if (IS_DUMMY_WRITTEN_PAGE(page)) {
 			set_page_private(page, (unsigned long)NULL);

@@ -113,7 +112,7 @@ static void f2fs_write_end_io(struct bio *bio)
 			mapping_set_error(page->mapping, -EIO);
 			f2fs_stop_checkpoint(sbi, true);
 		}
-		dec_page_count(sbi, type);
+		dec_page_count(sbi, WB_DATA_TYPE(page));
 		clear_cold_data(page);
 		end_page_writeback(page);
 	}

@@ -389,7 +388,7 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 	fio->submitted = 1;
 
 	if (!is_read)
-		inc_page_count(sbi, WB_DATA_TYPE(bio_page));
+		inc_page_count(sbi, WB_DATA_TYPE(fio->page));
 
 	down_write(&io->io_rwsem);

@@ -403,7 +402,7 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 				fio->new_blkaddr & F2FS_IO_SIZE_MASK(sbi)) {
 			err = -EAGAIN;
 			if (!is_read)
-				dec_page_count(sbi, WB_DATA_TYPE(bio_page));
+				dec_page_count(sbi, WB_DATA_TYPE(fio->page));
 			goto out_fail;
 		}
 		io->bio = __bio_alloc(sbi, fio->new_blkaddr,

Jaegeuk Kim

author maintainer

2017-04-07 17:46 UTC #3 lore raw

Quoted 5 lines · Chao Yu

Hi Jaegeuk,
Quoted 1 line · Jaegeuk Kim
This patch fixes # of outstanding writeback pages by checking its original page.

Quoted 8 lines

Originally, we use bio_page to track encrypted page which is been writebacked, there are two cases:
1. background flush through .writepages: encrypted_page has no mapping info, will not be tracked.
2. synchronized GC through move_encrypted_block: encrypted_page is meta page, will be tracked.
So there is no problem with old code?

Consistency was fine, but why not tracking # of writebacked pages for encryption case?

Thanks,

Quoted 49 lines

Thanks,

Quoted 46 lines

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

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

fs/f2fs/data.c +3 -4

index 3d74c0ffa4c7..af3e849e96d2 100644
@@ -94,7 +94,6 @@ static void f2fs_write_end_io(struct bio *bio)
 
 	bio_for_each_segment_all(bvec, bio, i) {
 		struct page *page = bvec->bv_page;
-		enum count_type type = WB_DATA_TYPE(page);
 
 		if (IS_DUMMY_WRITTEN_PAGE(page)) {
 			set_page_private(page, (unsigned long)NULL);

@@ -113,7 +112,7 @@ static void f2fs_write_end_io(struct bio *bio)
 			mapping_set_error(page->mapping, -EIO);
 			f2fs_stop_checkpoint(sbi, true);
 		}
-		dec_page_count(sbi, type);
+		dec_page_count(sbi, WB_DATA_TYPE(page));
 		clear_cold_data(page);
 		end_page_writeback(page);
 	}

@@ -389,7 +388,7 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 	fio->submitted = 1;
 
 	if (!is_read)
-		inc_page_count(sbi, WB_DATA_TYPE(bio_page));
+		inc_page_count(sbi, WB_DATA_TYPE(fio->page));
 
 	down_write(&io->io_rwsem);

@@ -403,7 +402,7 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 				fio->new_blkaddr & F2FS_IO_SIZE_MASK(sbi)) {
 			err = -EAGAIN;
 			if (!is_read)
-				dec_page_count(sbi, WB_DATA_TYPE(bio_page));
+				dec_page_count(sbi, WB_DATA_TYPE(fio->page));
 			goto out_fail;
 		}
 		io->bio = __bio_alloc(sbi, fio->new_blkaddr,

Chao Yu

2017-04-10 11:14 UTC #4 lore raw

Quoted 13 lines · Jaegeuk Kim

Quoted 12 lines · Chao Yu
Hi Jaegeuk,
Quoted 1 line · Jaegeuk Kim
This patch fixes # of outstanding writeback pages by checking its original page.
Originally, we use bio_page to track encrypted page which is been writebacked, there are two cases:
1. background flush through .writepages: encrypted_page has no mapping info, will not be tracked.
2. synchronized GC through move_encrypted_block: encrypted_page is meta page, will be tracked.

Quoted 4 lines

Quoted 1 line
So there is no problem with old code?
Consistency was fine, but why not tracking # of writebacked pages for encryption case?

Hmm.. because checkpoint doesn't care about writebacked pages of normal file, no matter the file is encrypted or not. Right?

Thanks,

Quoted 55 lines

Thanks,

Quoted 39 lines

Thanks,

Quoted 36 lines

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

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

fs/f2fs/data.c +2 -3

index 3d74c0ffa4c7..af3e849e96d2 100644
@@ -94,7 +94,6 @@ static void f2fs_write_end_io(struct bio *bio)
 
 	bio_for_each_segment_all(bvec, bio, i) {
 		struct page *page = bvec->bv_page;
-		enum count_type type = WB_DATA_TYPE(page);
 
 		if (IS_DUMMY_WRITTEN_PAGE(page)) {
 			set_page_private(page, (unsigned long)NULL);

@@ -113,7 +112,7 @@ static void f2fs_write_end_io(struct bio *bio)
 			mapping_set_error(page->mapping, -EIO);
 			f2fs_stop_checkpoint(sbi, true);
 		}
-		dec_page_count(sbi, type);
+		dec_page_count(sbi, WB_DATA_TYPE(page));
 		clear_cold_data(page);
 		end_page_writeback(page);
 	}

@@ -389,7 +388,7 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 	fio->submitted = 1;
 
 	if (!is_read)
-		inc_page_count(sbi, WB_DATA_TYPE(bio_page));
+		inc_page_count(sbi, WB_DATA_TYPE(fio->page));
 
 	down_write(&io->io_rwsem);

Quoted 10 lines

fs/f2fs/data.c int f2fs_submit_page_mbio(struct f2fs_io_info *fio) · line 408

Quoted 10 lines

fs/f2fs/data.c +1 -1

@@ -403,7 +402,7 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 				fio->new_blkaddr & F2FS_IO_SIZE_MASK(sbi)) {
 			err = -EAGAIN;
 			if (!is_read)
-				dec_page_count(sbi, WB_DATA_TYPE(bio_page));
+				dec_page_count(sbi, WB_DATA_TYPE(fio->page));
 			goto out_fail;
 		}
 		io->bio = __bio_alloc(sbi, fio->new_blkaddr,