On Mon, 2011-09-26 at 17:45 +0800, Christoph Hellwig wrote:
> On Mon, Sep 26, 2011 at 03:38:47PM +0800, Shaohua Li wrote:
> > On Mon, 2011-09-26 at 15:30 +0800, Christoph Hellwig wrote:
> > > On Mon, Sep 26, 2011 at 10:30:26AM +0800, Shaohua Li wrote:
> > > > Some filesystems implement .writepages. We don't have blk plug
> > > > in such filesystems for .writepages.
> > >
> > > Please add the plugging in the actual ->writepages instances.
> > there are several filesystems have ->writepages. Can you share an hint
> > why we don't add plugging in the do_writepages?
>
> Because do_writepages is completely generic code with no knowledge of
> block I/O. I really don't want to have block plugging be intimately
> tied into core VM/writeback code all over. We've been there with
> ->sync_page, and it was a major pain - nevermind the additional (small)
> overhead for the not block based filesystems.
I searched a little bit, looks only ext4 need it. here is the patch.
Add block plug for ext4 .writepages. Though ext4 .writepages
already handles request merge very well, block plug is still
helpful to reduce block lock contention.
Signed-off-by: Shaohua Li <[email protected]>
---
fs/ext4/inode.c | 3 +++
1 file changed, 3 insertions(+)
Index: linux/fs/ext4/inode.c
===================================================================
--- linux.orig/fs/ext4/inode.c 2011-09-28 10:13:50.000000000 +0800
+++ linux/fs/ext4/inode.c 2011-09-28 10:23:31.000000000 +0800
@@ -2046,6 +2046,7 @@ static int ext4_da_writepages(struct add
struct ext4_sb_info *sbi = EXT4_SB(mapping->host->i_sb);
pgoff_t done_index = 0;
pgoff_t end;
+ struct blk_plug plug;
trace_ext4_da_writepages(inode, wbc);
@@ -2124,6 +2125,7 @@ retry:
if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages)
tag_pages_for_writeback(mapping, index, end);
+ blk_start_plug(&plug);
while (!ret && wbc->nr_to_write > 0) {
/*
@@ -2188,6 +2190,7 @@ retry:
*/
break;
}
+ blk_finish_plug(&plug);
if (!io_done && !cycled) {
cycled = 1;
index = 0;
On Wed, 2011-09-28 at 11:09 +0800, Shaohua Li wrote:
> On Mon, 2011-09-26 at 17:45 +0800, Christoph Hellwig wrote:
> > On Mon, Sep 26, 2011 at 03:38:47PM +0800, Shaohua Li wrote:
> > > On Mon, 2011-09-26 at 15:30 +0800, Christoph Hellwig wrote:
> > > > On Mon, Sep 26, 2011 at 10:30:26AM +0800, Shaohua Li wrote:
> > > > > Some filesystems implement .writepages. We don't have blk plug
> > > > > in such filesystems for .writepages.
> > > >
> > > > Please add the plugging in the actual ->writepages instances.
> > > there are several filesystems have ->writepages. Can you share an hint
> > > why we don't add plugging in the do_writepages?
> >
> > Because do_writepages is completely generic code with no knowledge of
> > block I/O. I really don't want to have block plugging be intimately
> > tied into core VM/writeback code all over. We've been there with
> > ->sync_page, and it was a major pain - nevermind the additional (small)
> > overhead for the not block based filesystems.
> I searched a little bit, looks only ext4 need it. here is the patch.
>
>
> Add block plug for ext4 .writepages. Though ext4 .writepages
> already handles request merge very well, block plug is still
> helpful to reduce block lock contention.
>
ping
> Signed-off-by: Shaohua Li <[email protected]>
>
> ---
> fs/ext4/inode.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> Index: linux/fs/ext4/inode.c
> ===================================================================
> --- linux.orig/fs/ext4/inode.c 2011-09-28 10:13:50.000000000 +0800
> +++ linux/fs/ext4/inode.c 2011-09-28 10:23:31.000000000 +0800
> @@ -2046,6 +2046,7 @@ static int ext4_da_writepages(struct add
> struct ext4_sb_info *sbi = EXT4_SB(mapping->host->i_sb);
> pgoff_t done_index = 0;
> pgoff_t end;
> + struct blk_plug plug;
>
> trace_ext4_da_writepages(inode, wbc);
>
> @@ -2124,6 +2125,7 @@ retry:
> if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages)
> tag_pages_for_writeback(mapping, index, end);
>
> + blk_start_plug(&plug);
> while (!ret && wbc->nr_to_write > 0) {
>
> /*
> @@ -2188,6 +2190,7 @@ retry:
> */
> break;
> }
> + blk_finish_plug(&plug);
> if (!io_done && !cycled) {
> cycled = 1;
> index = 0;
>
On Wed, Sep 28, 2011 at 11:09:40AM +0800, Shaohua Li wrote:
> I searched a little bit, looks only ext4 need it. here is the patch.
>
>
> Add block plug for ext4 .writepages. Though ext4 .writepages
> already handles request merge very well, block plug is still
> helpful to reduce block lock contention.
>
> Signed-off-by: Shaohua Li <[email protected]>
Thanks, applied.
- Ted
2011/10/11 Ted Ts'o <[email protected]>:
> On Wed, Sep 28, 2011 at 11:09:40AM +0800, Shaohua Li wrote:
>> I searched a little bit, looks only ext4 need it. here is the patch.
>>
>>
>> Add block plug for ext4 .writepages. Though ext4 .writepages
>> already handles request merge very well, block plug is still
>> helpful to reduce block lock contention.
>>
>> Signed-off-by: Shaohua Li <[email protected]>
You found good point.
If there are other filesystem that didn't use generic_writepages,
these are needed as this patch.
Reviewed-by: NamJae Jeon <[email protected]>
>
> Thanks, applied.
>
> - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Tue, Oct 11, 2011 at 1:51 AM, Ted Ts'o <[email protected]> wrote:
> On Wed, Sep 28, 2011 at 11:09:40AM +0800, Shaohua Li wrote:
>> I searched a little bit, looks only ext4 need it. here is the patch.
>>
>>
>> Add block plug for ext4 .writepages. Though ext4 .writepages
>> already handles request merge very well, block plug is still
>> helpful to reduce block lock contention.
>>
>> Signed-off-by: Shaohua Li <[email protected]>
Does it require to add blk_finish_plug(&plug) when error case?
+ blk_start_plug(&plug);
while (!ret && wbc->nr_to_write > 0) {
...
/* start a new transaction*/
handle = ext4_journal_start(inode, needed_blocks);
if (IS_ERR(handle)) {
ret = PTR_ERR(handle);
ext4_msg(inode->i_sb, KERN_CRIT, "%s: jbd2_start: "
"%ld pages, ino %lu; err %d", __func__,
wbc->nr_to_write, inode->i_ino, ret);
+ blk_finish_plug(&plug);
goto out_writepages;
}
Thank you,
Kyungmin Park-
On Tue, 2011-10-11 at 18:40 +0800, Kyungmin Park wrote:
> On Tue, Oct 11, 2011 at 1:51 AM, Ted Ts'o <[email protected]> wrote:
> > On Wed, Sep 28, 2011 at 11:09:40AM +0800, Shaohua Li wrote:
> >> I searched a little bit, looks only ext4 need it. here is the patch.
> >>
> >>
> >> Add block plug for ext4 .writepages. Though ext4 .writepages
> >> already handles request merge very well, block plug is still
> >> helpful to reduce block lock contention.
> >>
> >> Signed-off-by: Shaohua Li <[email protected]>
>
> Does it require to add blk_finish_plug(&plug) when error case?
oops, we need.
Subject: ext4: calling blk_finish_plug in error path
Forgot calling blk_finish_plug in error code path. Thanks Kyungmin Park pointed
it out.
Signed-off-by: Shaohua Li <[email protected]>
---
fs/ext4/inode.c | 1 +
1 file changed, 1 insertion(+)
Index: linux/fs/ext4/inode.c
===================================================================
--- linux.orig/fs/ext4/inode.c 2011-10-12 08:46:32.000000000 +0800
+++ linux/fs/ext4/inode.c 2011-10-12 08:48:10.000000000 +0800
@@ -2144,6 +2144,7 @@ retry:
ext4_msg(inode->i_sb, KERN_CRIT, "%s: jbd2_start: "
"%ld pages, ino %lu; err %d", __func__,
wbc->nr_to_write, inode->i_ino, ret);
+ blk_finish_plug(&plug);
goto out_writepages;
}