Re: a old issue of ext4 on lts 3.10

[Alex Shi <alex.shi@linaro.org>]

Hi Greg,

It was reported this commit could save few seconds sometime in
consequence writing on smart phone.

commit 7afe5aa59ed3da7b6161617e7f157c7c680dc41e
    ext4: convert write_begin methods to stable_page_writes semantics

> The patch helps because most of storage today doesn't require that the
> page isn't changed while IO is in flight. That is required only for
> data checksumming or copy-on-write semantics but ext4 does neither of
> those. So we don't have to wait for IO completion in ext4_write_begin()
> unless underlying storage requires it.
>
> 								Honza

Seems it is a very simple and useful patch for some stable kernel, like
lts 3.10. Would you like to pick it up?

Thanks
    Alex

On 05/18/2015 02:21 PM, Jan Kara wrote:
> On Thu 14-05-15 23:36:31, Dmitry Monakhov wrote:
>> Alex Shi <alex.shi@linaro.org> writes:
>>
>>> Hi Dmitry&Theodore,
>>>
>>> Someone said without the following patch on lts 3.10 kernel (which used
>>> as android base kernel). the write maybe very very slow, needs 1 or 2
>>> seconds to finish.
>> In fact this was an optimization.
>> wait_for_stable_page() is actually and optimized wait_on_page_writeback()
>>
>> see:
>> void wait_for_stable_page(struct page *page)
>> {
>>         struct address_space *mapping = page_mapping(page);
>>                 struct backing_dev_info *bdi =
>>                 mapping->backing_dev_info;
>>
>>         if (!bdi_cap_stable_pages_required(bdi))
>>                         return;
>>
>>         wait_on_page_writeback(page);
>> }
>> It is very unlikely the patch provokes such huge slowdown.
>> Can you please repeat your measurements and double check your evidence.
>   I think Alex meant that without the patch he is seeing long stalls.
> That is possible when we wait for writeback and the storage is busy.
> 
>>> I quick looked this patch, seems it's no harm for a normal fs function.
>>> but still don't know why it is helpful. So do you remember why you
>>> commit this change at that time?
>   The patch helps because most of storage today doesn't require that the
> page isn't changed while IO is in flight. That is required only for
> data checksumming or copy-on-write semantics but ext4 does neither of
> those. So we don't have to wait for IO completion in ext4_write_begin()
> unless underlying storage requires it.
> 
> 								Honza
> 
>>> ommit 7afe5aa59ed3da7b6161617e7f157c7c680dc41e
>>> Author: Dmitry Monakhov <dmonakhov@openvz.org>
>>> Date:   Wed Aug 28 14:30:47 2013 -0400
>>>
>>>     ext4: convert write_begin methods to stable_page_writes semantics
>>>
>>>     Use wait_for_stable_page() instead of wait_on_page_writeback()
>>>
>>>     Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
>>>     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>>>     Reviewed-by: Jan Kara <jack@suse.cz>
>>>
>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>> index fc4051e..47c8e46 100644
>>> --- a/fs/ext4/inode.c
>>> +++ b/fs/ext4/inode.c
>>> @@ -969,7 +969,8 @@ retry_journal:
>>>                 ext4_journal_stop(handle);
>>>                 goto retry_grab;
>>>         }
>>> -       wait_on_page_writeback(page);
>>> +       /* In case writeback began while the page was unlocked */
>>> +       wait_for_stable_page(page);
>>>
>>>         if (ext4_should_dioread_nolock(inode))
>>>                 ret = __block_write_begin(page, pos, len,
>>> ext4_get_block_write);
>>> @@ -2678,7 +2679,7 @@ retry_journal:
>>>                 goto retry_grab;
>>>         }
>>>         /* In case writeback began while the page was unlocked */
>>> -       wait_on_page_writeback(page);
>>> +       wait_for_stable_page(page);
>>>
>>>         ret = __block_write_begin(page, pos, len, ext4_da_get_block_prep);
>>>         if (ret < 0) {
>>> ~
>>>
>>> -- 
>>> Thanks
>>>     Alex
> 
>