Re: a old issue of ext4 on lts 3.10

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

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.

yes.
>>> 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

Thanks a lot for clear explanations, 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
>