[PATCH 4/4] f2fs: add blk plugging support in f2fs

[PATCH 4/4] f2fs: add blk plugging support in f2fs

[Namjae Jeon]

From: Namjae Jeon <namjae.jeon@samsung.com>

With f2fs having different writepages support for data, node and metapages.
It will not be covered under the generic blk plug support.
So,
1) modified the writepages for data pages.
2) Added blk plugging in node/metapages.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
---
 fs/f2fs/checkpoint.c |    3 +++
 fs/f2fs/data.c       |   14 +++++++++++++-
 fs/f2fs/node.c       |    4 +++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 448f728..127a904 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -129,10 +129,12 @@ long sync_meta_pages(struct f2fs_sb_info *sbi, enum page_type type,
 	struct writeback_control wbc = {
 		.for_reclaim = 0,
 	};
+	struct blk_plug plug;
 
 	trace_f2fs_sync_pages(sbi->sb, type, nr_to_write);
 	pagevec_init(&pvec, 0);
 
+	blk_start_plug(&plug);
 	while (index <= end) {
 		int i, nr_pages;
 		nr_pages = pagevec_lookup_tag(&pvec, mapping, &index,
@@ -158,6 +160,7 @@ long sync_meta_pages(struct f2fs_sb_info *sbi, enum page_type type,
 	if (nwritten)
 		f2fs_submit_bio(sbi, type, nr_to_write == LONG_MAX);
 
+	blk_finish_plug(&plug);
 	return nwritten;
 }
 
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 1d7dbb4..5203a5d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -558,6 +558,15 @@ redirty_out:
 
 #define MAX_DESIRED_PAGES_WP	4096
 
+static int __f2fs_writepage(struct page *page, struct writeback_control *wbc,
+			void *data)
+{
+	struct address_space *mapping = data;
+	int ret = mapping->a_ops->writepage(page, wbc);
+	mapping_set_error(mapping, ret);
+	return ret;
+}
+
 static int f2fs_write_data_pages(struct address_space *mapping,
 			    struct writeback_control *wbc)
 {
@@ -565,6 +574,7 @@ static int f2fs_write_data_pages(struct address_space *mapping,
 	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
 	int ret;
 	long excess_nrtw = 0, desired_nrtw;
+	struct blk_plug plug;
 
 	if (wbc->nr_to_write < MAX_DESIRED_PAGES_WP) {
 		desired_nrtw = MAX_DESIRED_PAGES_WP;
@@ -572,12 +582,14 @@ static int f2fs_write_data_pages(struct address_space *mapping,
 		wbc->nr_to_write = desired_nrtw;
 	}
 
+	blk_start_plug(&plug);
 	if (!S_ISDIR(inode->i_mode))
 		mutex_lock(&sbi->writepages);
-	ret = generic_writepages(mapping, wbc);
+	ret = write_cache_pages(mapping, wbc, __f2fs_writepage, mapping);
 	if (!S_ISDIR(inode->i_mode))
 		mutex_unlock(&sbi->writepages);
 	f2fs_submit_bio(sbi, DATA, (wbc->sync_mode == WB_SYNC_ALL));
+	blk_finish_plug(&plug);
 
 	remove_dirty_dir_inode(inode);
 
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 40ceda2..49a5806 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -999,9 +999,10 @@ int sync_node_pages(struct f2fs_sb_info *sbi, nid_t ino,
 	struct pagevec pvec;
 	int step = ino ? 2 : 0;
 	int nwritten = 0, wrote = 0;
-
+	struct blk_plug plug;
 	pagevec_init(&pvec, 0);
 
+	blk_start_plug(&plug);
 next_step:
 	index = 0;
 	end = LONG_MAX;
@@ -1090,6 +1091,7 @@ continue_unlock:
 
 	if (wrote)
 		f2fs_submit_bio(sbi, NODE, wbc->sync_mode == WB_SYNC_ALL);
+	blk_finish_plug(&plug);
 
 	return nwritten;
 }
-- 
1.7.9.5

Re: [PATCH 4/4] f2fs: add blk plugging support in f2fs

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

2013-01-12 (토), 14:42 +0900, Namjae Jeon:
> From: Namjae Jeon <namjae.jeon@samsung.com>
> 
> With f2fs having different writepages support for data, node and metapages.
> It will not be covered under the generic blk plug support.

Could you show any improvement points with this patch?

Currently, there is no reason to use blk plugging, since f2fs itself
gathers bios and then submit one big bio.

Thanks,

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] f2fs: add blk plugging support in f2fs

[Namjae Jeon]

2013/1/14, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> 2013-01-12 (토), 14:42 +0900, Namjae Jeon:
>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>
>> With f2fs having different writepages support for data, node and
>> metapages.
>> It will not be covered under the generic blk plug support.
>
> Could you show any improvement points with this patch?
>
> Currently, there is no reason to use blk plugging, since f2fs itself
> gathers bios and then submit one big bio.
>
> Thanks,
Hi Jaegeuk,

There is no performance difference after introducing the block
plugging in F2FS.
We introduced this to reduced block lock contention for f2fs also.

For every BIO request queuing part to the request queue: it needs to
acquire lock->
spin_lock_irq(q->queue_lock);

Even though the F2FS - already is handling the requests part very
well. But still we can make use of blk_plug to reduce the block lock
contention.

When we introduce block plugging to F2FS part - all the requests will
first be maintained on TASK basis and then pushed to the request
queue. So, we do not have contention for the “queue lock”.

If we consider the normal data write path: initial block plug is
already taken care by generic write_pages(for f2fs readpages block
plug is already taken care by default path of:
read_pages()->mpage_readpages())),
but we can optimize this write path also as we shared the in patch.
Similarly, we introduced block plug for the node_page and meta_pages.

Please share your opinion on this.

Thanks!
>
> --
> Jaegeuk Kim
> Samsung
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] f2fs: add blk plugging support in f2fs

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 2500 bytes --]

2013-01-14 (월), 20:10 +0900, Namjae Jeon:
> 2013/1/14, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> > 2013-01-12 (토), 14:42 +0900, Namjae Jeon:
> >> From: Namjae Jeon <namjae.jeon@samsung.com>
> >>
> >> With f2fs having different writepages support for data, node and
> >> metapages.
> >> It will not be covered under the generic blk plug support.
> >
> > Could you show any improvement points with this patch?
> >
> > Currently, there is no reason to use blk plugging, since f2fs itself
> > gathers bios and then submit one big bio.
> >
> > Thanks,
> Hi Jaegeuk,
> 
> There is no performance difference after introducing the block
> plugging in F2FS.

Well, this patch is not a bug fix, but an enhancement patch.
Therefore we need to come up with how exactly the blk plugging support
makes an effect on the performance.

> We introduced this to reduced block lock contention for f2fs also.

> For every BIO request queuing part to the request queue: it needs to
> acquire lock->
> spin_lock_irq(q->queue_lock);
> 
> Even though the F2FS - already is handling the requests part very
> well. But still we can make use of blk_plug to reduce the block lock
> contention.
> 
> When we introduce block plugging to F2FS part - all the requests will
> first be maintained on TASK basis and then pushed to the request
> queue. So, we do not have contention for the “queue lock”.
> 

What I concern is how much block lock contention would be serious.
Let's see the following operational differences.

1. Merging bios prior to grabing "queue lock"
 The f2fs merges consecutive IOs in the file system level before
submitting any bios, which is similar with the back merge by the
plugging mechanism in attempt_plug_merge(). Both of them need to acquire
no queue lock.

2. Merging policy with respect to tasks
 The f2fs merges IOs as much as possible regardless of tasks, while
blk-plugging is conducted on a basis of tasks. As we can understand
there are trade-offs, f2fs tries to maximize the write performance with
well-merged bios. 

As a result, if f2fs produces many consecutive but separated bios in
writepages(), it would be good to use blk-plugging. Since as you said,
f2fs would be able to avoid queue lock contention in the block layer by
merging them.
But, f2fs merges IOs and submit one bio, which means that there are not
much chances to merge bios by attempt_plug_merge().

How do you think?

Thanks,

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] f2fs: add blk plugging support in f2fs

[Namjae Jeon]

2013/1/15, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> 2013-01-14 (월), 20:10 +0900, Namjae Jeon:
>> 2013/1/14, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
>> > 2013-01-12 (토), 14:42 +0900, Namjae Jeon:
>> >> From: Namjae Jeon <namjae.jeon@samsung.com>
>> >>
>> >> With f2fs having different writepages support for data, node and
>> >> metapages.
>> >> It will not be covered under the generic blk plug support.
>> >
>> > Could you show any improvement points with this patch?
>> >
>> > Currently, there is no reason to use blk plugging, since f2fs itself
>> > gathers bios and then submit one big bio.
>> >
>> > Thanks,
>> Hi Jaegeuk,
>>
>> There is no performance difference after introducing the block
>> plugging in F2FS.
>
> Well, this patch is not a bug fix, but an enhancement patch.
> Therefore we need to come up with how exactly the blk plugging support
> makes an effect on the performance.
>
>> We introduced this to reduced block lock contention for f2fs also.
>
>> For every BIO request queuing part to the request queue: it needs to
>> acquire lock->
>> spin_lock_irq(q->queue_lock);
>>
>> Even though the F2FS - already is handling the requests part very
>> well. But still we can make use of blk_plug to reduce the block lock
>> contention.
>>
>> When we introduce block plugging to F2FS part - all the requests will
>> first be maintained on TASK basis and then pushed to the request
>> queue. So, we do not have contention for the “queue lock”.
>>
>
> What I concern is how much block lock contention would be serious.
> Let's see the following operational differences.
>
> 1. Merging bios prior to grabing "queue lock"
>  The f2fs merges consecutive IOs in the file system level before
> submitting any bios, which is similar with the back merge by the
> plugging mechanism in attempt_plug_merge(). Both of them need to acquire
> no queue lock.
>
> 2. Merging policy with respect to tasks
>  The f2fs merges IOs as much as possible regardless of tasks, while
> blk-plugging is conducted on a basis of tasks. As we can understand
> there are trade-offs, f2fs tries to maximize the write performance with
> well-merged bios.
>
> As a result, if f2fs produces many consecutive but separated bios in
> writepages(), it would be good to use blk-plugging. Since as you said,
> f2fs would be able to avoid queue lock contention in the block layer by
> merging them.
> But, f2fs merges IOs and submit one bio, which means that there are not
> much chances to merge bios by attempt_plug_merge().
>
> How do you think?
Hi Jaegeuk.

Yes, You're right. I agree block plug is not needed in f2fs. So plz
ignore this patch.
note => Regardless of the intent in the patch, it has already been
used in writepages (f2fs uses generic_writepages).
So to make the overall code consistent, either we should remove blk
plug from entire F2FS write part or change f2fs_write_data_pages to
include blk plug properly - like the code change for this part we
share in the patch.

Let me know your opinion.

Thanks.

>
> Thanks,
>
> --
> Jaegeuk Kim
> Samsung
>

Re: [PATCH 4/4] f2fs: add blk plugging support in f2fs

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 3264 bytes --]

> Yes, You're right. I agree block plug is not needed in f2fs. So plz
> ignore this patch.
> note => Regardless of the intent in the patch, it has already been
> used in writepages (f2fs uses generic_writepages).
> So to make the overall code consistent, either we should remove blk
> plug from entire F2FS write part or change f2fs_write_data_pages to
> include blk plug properly - like the code change for this part we
> share in the patch.

Agreed. :)
How about this patch?

From 09e11ac9734cd085f9a92c46098fb083091f5429 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk.kim@samsung.com>
Date: Tue, 15 Jan 2013 16:45:24 +0900
Subject: [PATCH] f2fs: remove the blk_plug usage in
f2fs_write_data_pages

From: Namjae Jeon <namjae.jeon@samsung.com>

Let's consider the usage of blk_plug in f2fs_write_data_pages().
We can come up with the two issues: lock contention and task awareness.

1. Merging bios prior to grabing "queue lock"
 The f2fs merges consecutive IOs in the file system level before
 submitting any bios, which is similar with the back merge by the
 plugging mechanism in attempt_plug_merge(). Both of them need to
acquire no queue lock.

2. Merging policy with respect to tasks
 The f2fs merges IOs as much as possible regardless of tasks, while
 blk-plugging is conducted on a basis of tasks. As we can understand
 there are trade-offs, f2fs tries to maximize the write performance with
 well-merged bios.

As a result, if f2fs produces many consecutive but separated bios in
writepages(), it would be good to use blk-plugging since f2fs would be
able to avoid queue lock contention in the block layer by merging them.
But, f2fs merges IOs and submit one bio, which means that there are not
much chances to merge bios by attempt_plug_merge().

However, f2fs has already been used blk_plug by triggering
generic_writepages()
in f2fs_write_data_pages().
So to make the overall code consistency, I'd like to remove blk_plug
there.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/data.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 3aa5ce7..b1347fc 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -547,6 +547,15 @@ redirty_out:
 
 #define MAX_DESIRED_PAGES_WP	4096
 
+static int __f2fs_writepage(struct page *page, struct writeback_control
*wbc,
+			void *data)
+{
+	struct address_space *mapping = data;
+	int ret = mapping->a_ops->writepage(page, wbc);
+	mapping_set_error(mapping, ret);
+	return ret;
+}
+
 static int f2fs_write_data_pages(struct address_space *mapping,
 			    struct writeback_control *wbc)
 {
@@ -563,7 +572,7 @@ static int f2fs_write_data_pages(struct
address_space *mapping,
 
 	if (!S_ISDIR(inode->i_mode))
 		mutex_lock(&sbi->writepages);
-	ret = generic_writepages(mapping, wbc);
+	ret = write_cache_pages(mapping, wbc, __f2fs_writepage, mapping);
 	if (!S_ISDIR(inode->i_mode))
 		mutex_unlock(&sbi->writepages);
 	f2fs_submit_bio(sbi, DATA, (wbc->sync_mode == WB_SYNC_ALL));
-- 
1.8.0.1.250.gb7973fb

-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/4] f2fs: add blk plugging support in f2fs

[Namjae Jeon]

2013/1/15, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
>> Yes, You're right. I agree block plug is not needed in f2fs. So plz
>> ignore this patch.
>> note => Regardless of the intent in the patch, it has already been
>> used in writepages (f2fs uses generic_writepages).
>> So to make the overall code consistent, either we should remove blk
>> plug from entire F2FS write part or change f2fs_write_data_pages to
>> include blk plug properly - like the code change for this part we
>> share in the patch.
>
> Agreed. :)
> How about this patch?
Looks good to me!
Thanks a lot!