Re: [PATCHv2 3/3] reiser4: mark pages created during tail2extent conversion as dirty.

[Edward Shishkin <edward.shishkin@gmail.com>]

On 11/12/2015 12:31 PM, Ivan Shapovalov wrote:
> On 2015-11-12 at 11:00 +0100, Edward Shishkin wrote:
>> On 11/11/2015 11:30 AM, Edward Shishkin wrote:
>>>
>>> On 11/09/2015 01:18 PM, Edward Shishkin wrote:
>>>> On 10/25/2015 01:02 AM, Ivan Shapovalov wrote:
>>>>> This is responsible for an oops in v4.2 in
>>>>> write_jnodes_to_disk_extent() -> set_page_writeback().
>>>
>>> As to me, this patch doesn't prevent the oops,
>>> so someone else forgets to set pages dirty...
>>
>> False alarm: I applied to the old stuff..
>> Everything woks without the hack in
>> write_jnodes_to_disk_extent().
>> Good work! Merged to reiser4-for-4.2.3.
> Hi Edward,
>
> Glad to hear that. I was already halfway recovering my debug patches
> for collecting stacktraces and related information and was going to ask
> you to reproduce :)
>
>>>>> The pages needs to be marked dirty before marked writeback.
>>>>>
>>>>>   From a similar problem in f2fs:
>>>>> "The cgroup attaches inode->i_wb via mark_inode_dirty and when
>>>>> set_page_writeback is called, __inc_wb_stat() updates i_wb's
>>>>> stat.
>>>>>
>>>>> So, we need to explicitly call set_page_dirty-
>>>>>> __mark_inode_dirty in
>>>>> prior to any writebacking pages."
>>>>>
>>>>> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
>>>>> ---
>>>>>    fs/reiser4/plugin/file/tail_conversion.c | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/fs/reiser4/plugin/file/tail_conversion.c
>>>>> b/fs/reiser4/plugin/file/tail_conversion.c
>>>>> index 7542c03..c856b73 100644
>>>>> --- a/fs/reiser4/plugin/file/tail_conversion.c
>>>>> +++ b/fs/reiser4/plugin/file/tail_conversion.c
>>>>> @@ -175,6 +175,7 @@ static int replace(struct inode *inode,
>>>>> struct
>>>>> page **pages, unsigned nr_pages,
>>>>>                                    i_mapping));
>>>>>            if (result)
>>>>>                break;
>>>>> +        set_page_dirty_notag(pages[i]);
>>>>
>>>> So, at this point the page is dirty but not uptodate.
>>>> I am confused with this. Why not to set the page
>>>> uptodate right before setting it dirty? At this point
>>>> everything has been copied already, so the page is
>>>> in fact uptodate. Could you please try this?
> Have you made this correction in the merged patch?

Yes I have.

> Anyway, could you
> please explain why it should be done one way and not another? What will
> happen if the ordering is left as is?
>
> Is it about marking page uptodate _before unlocking_, so that no one will see it dirty && !uptodate?

Just let's set all the flags in timely fashion.
Otherwise it would mean a bad style.

Thanks,
Edward.