[PATCH 2/2] mm: make do_writepages() use plugging

[PATCH 2/2] mm: make do_writepages() use plugging

[Amit Sahrawat]

This will cover all the invocations for writepages to be called with
plugging support.

Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
---
 mm/page-writeback.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 363ba70..2bea32c 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -1866,14 +1866,18 @@ EXPORT_SYMBOL(generic_writepages);
 
 int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
 {
+	struct blk_plug plug;
 	int ret;
 
 	if (wbc->nr_to_write <= 0)
 		return 0;
+
+	blk_start_plug(&plug);
 	if (mapping->a_ops->writepages)
 		ret = mapping->a_ops->writepages(mapping, wbc);
 	else
 		ret = generic_writepages(mapping, wbc);
+	blk_finish_plug(&plug);
 	return ret;
 }
 
-- 
1.7.2.3

Re: [PATCH 2/2] mm: make do_writepages() use plugging

[Peter Zijlstra]

On Fri, 2012-02-03 at 18:57 +0530, Amit Sahrawat wrote:
> This will cover all the invocations for writepages to be called with
> plugging support.

This changelog fails to explain why this is a good thing... I thought
the idea of the new plugging stuff was that we now don't need to
sprinkle plugs all over the kernel..

> Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
> ---
>  mm/page-writeback.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 363ba70..2bea32c 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1866,14 +1866,18 @@ EXPORT_SYMBOL(generic_writepages);
>  
>  int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>  {
> +	struct blk_plug plug;
>  	int ret;
>  
>  	if (wbc->nr_to_write <= 0)
>  		return 0;
> +
> +	blk_start_plug(&plug);
>  	if (mapping->a_ops->writepages)
>  		ret = mapping->a_ops->writepages(mapping, wbc);
>  	else
>  		ret = generic_writepages(mapping, wbc);
> +	blk_finish_plug(&plug);
>  	return ret;
>  }
>  

Re: [PATCH 2/2] mm: make do_writepages() use plugging

[Wu Fengguang]

On Fri, Feb 03, 2012 at 06:57:06PM +0530, Amit Sahrawat wrote:
> This will cover all the invocations for writepages to be called with
> plugging support.
 
Thanks.  I'll test it on the major filesystems. But would you
name a few filesystems that are expected to benefit from it?
It's not obvious because some FS ->writepages eventually calls
generic_writepages() which already does plugging.

Thanks,
Fengguang

> Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
> ---
>  mm/page-writeback.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 363ba70..2bea32c 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1866,14 +1866,18 @@ EXPORT_SYMBOL(generic_writepages);
>  
>  int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>  {
> +	struct blk_plug plug;
>  	int ret;
>  
>  	if (wbc->nr_to_write <= 0)
>  		return 0;
> +
> +	blk_start_plug(&plug);
>  	if (mapping->a_ops->writepages)
>  		ret = mapping->a_ops->writepages(mapping, wbc);
>  	else
>  		ret = generic_writepages(mapping, wbc);
> +	blk_finish_plug(&plug);
>  	return ret;
>  }
>  
> -- 
> 1.7.2.3

Re: [PATCH 2/2] mm: make do_writepages() use plugging

[Amit Sahrawat]

Hi Peter,
Thanks for pointing out.

While checking the plug support in Write code flow, I came across this
main point from which - we invoke
writepages(mapping->a_ops->writepages(mapping, wbc)) from almost all
the the filesystems.

By mistake I checked 2 different kernel versions for this code(and
missed that the current version already has put plug in
mpage_writepages) ... so may be this patch is not worth considering.

Regards,
Amit Sahrawat


On Fri, Feb 3, 2012 at 7:02 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Fri, 2012-02-03 at 18:57 +0530, Amit Sahrawat wrote:
>> This will cover all the invocations for writepages to be called with
>> plugging support.
>
> This changelog fails to explain why this is a good thing... I thought
> the idea of the new plugging stuff was that we now don't need to
> sprinkle plugs all over the kernel..
>
>> Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
>> ---
>>  mm/page-writeback.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>> index 363ba70..2bea32c 100644
>> --- a/mm/page-writeback.c
>> +++ b/mm/page-writeback.c
>> @@ -1866,14 +1866,18 @@ EXPORT_SYMBOL(generic_writepages);
>>
>>  int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>>  {
>> +     struct blk_plug plug;
>>       int ret;
>>
>>       if (wbc->nr_to_write <= 0)
>>               return 0;
>> +
>> +     blk_start_plug(&plug);
>>       if (mapping->a_ops->writepages)
>>               ret = mapping->a_ops->writepages(mapping, wbc);
>>       else
>>               ret = generic_writepages(mapping, wbc);
>> +     blk_finish_plug(&plug);
>>       return ret;
>>  }
>>
>
>
>

Re: [PATCH 2/2] mm: make do_writepages() use plugging

[Christoph Hellwig]

On Fri, Feb 03, 2012 at 09:38:23PM +0800, Wu Fengguang wrote:
> On Fri, Feb 03, 2012 at 06:57:06PM +0530, Amit Sahrawat wrote:
> > This will cover all the invocations for writepages to be called with
> > plugging support.
>  
> Thanks.  I'll test it on the major filesystems. But would you
> name a few filesystems that are expected to benefit from it?
> It's not obvious because some FS ->writepages eventually calls
> generic_writepages() which already does plugging.

Ant that's exactly where it should stay instead of beeing sprinkled all
over the VM code.

NAK to the patch.

Re: [PATCH 2/2] mm: make do_writepages() use plugging

[Amit Sahrawat]

Is there a case for introducing blk_plug in write_one_page() can't
seem to find that support in the code flow
write_one_page()->mpage_writepage()

Regards,
Amit Sahrawat


On Fri, Feb 3, 2012 at 7:31 PM, Amit Sahrawat <amit.sahrawat83@gmail.com> wrote:
> Hi Peter,
> Thanks for pointing out.
>
> While checking the plug support in Write code flow, I came across this
> main point from which - we invoke
> writepages(mapping->a_ops->writepages(mapping, wbc)) from almost all
> the the filesystems.
>
> By mistake I checked 2 different kernel versions for this code(and
> missed that the current version already has put plug in
> mpage_writepages) ... so may be this patch is not worth considering.
>
> Regards,
> Amit Sahrawat
>
>
> On Fri, Feb 3, 2012 at 7:02 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
>> On Fri, 2012-02-03 at 18:57 +0530, Amit Sahrawat wrote:
>>> This will cover all the invocations for writepages to be called with
>>> plugging support.
>>
>> This changelog fails to explain why this is a good thing... I thought
>> the idea of the new plugging stuff was that we now don't need to
>> sprinkle plugs all over the kernel..
>>
>>> Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
>>> ---
>>>  mm/page-writeback.c |    4 ++++
>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
>>> index 363ba70..2bea32c 100644
>>> --- a/mm/page-writeback.c
>>> +++ b/mm/page-writeback.c
>>> @@ -1866,14 +1866,18 @@ EXPORT_SYMBOL(generic_writepages);
>>>
>>>  int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
>>>  {
>>> +     struct blk_plug plug;
>>>       int ret;
>>>
>>>       if (wbc->nr_to_write <= 0)
>>>               return 0;
>>> +
>>> +     blk_start_plug(&plug);
>>>       if (mapping->a_ops->writepages)
>>>               ret = mapping->a_ops->writepages(mapping, wbc);
>>>       else
>>>               ret = generic_writepages(mapping, wbc);
>>> +     blk_finish_plug(&plug);
>>>       return ret;
>>>  }
>>>
>>
>>
>>

Re: [PATCH 2/2] mm: make do_writepages() use plugging

[Chris Mason]

On Fri, Feb 03, 2012 at 09:08:34AM -0500, Christoph Hellwig wrote:
> On Fri, Feb 03, 2012 at 09:38:23PM +0800, Wu Fengguang wrote:
> > On Fri, Feb 03, 2012 at 06:57:06PM +0530, Amit Sahrawat wrote:
> > > This will cover all the invocations for writepages to be called with
> > > plugging support.
> >  
> > Thanks.  I'll test it on the major filesystems. But would you
> > name a few filesystems that are expected to benefit from it?
> > It's not obvious because some FS ->writepages eventually calls
> > generic_writepages() which already does plugging.
> 
> Ant that's exactly where it should stay instead of beeing sprinkled all
> over the VM code.
> 
> NAK to the patch.

We've actually had problems with plugging in the higher layers.
writepages is queueing up lots and lots and lots of pages for IO, and
especially on SSDs we want these IOs sent sooner rather than later.

I'm not sure yet how to make the plugs aware of the
please-feed-me-right-away demands of lower storage, but I'd rather not
add more high level plugs that the filesystems can't control.

-chris