[PATCHv2 0/3] reiser4: another batch of fixes for 4.2.

[PATCHv2 0/3] reiser4: another batch of fixes for 4.2.

[Ivan Shapovalov]

These are the fixes for issues I've discovered during migration to 4.2.
Last one is a real issue causing a reproducible oops in my configuration,
first two are byproducts.

v2: The 3/3 is now a real fix instead of a blind duct-tape fixup.

Ivan Shapovalov (3):
  reiser4: remove last traces of JNODE_NEW in the debugging code.
  reiser4: call account_page_redirty() on re-dirtying pages before giving them to entd.
  reiser4: mark pages created during tail2extent conversion as dirty.

 fs/reiser4/entd.c                        | 1 +
 fs/reiser4/jnode.c                       | 3 +--
 fs/reiser4/plugin/file/tail_conversion.c | 1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.6.2

[PATCHv2 1/3] reiser4: remove last traces of JNODE_NEW in the debugging code.

[Ivan Shapovalov]

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/jnode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/reiser4/jnode.c b/fs/reiser4/jnode.c
index e0c9e0c..43af7e9 100644
--- a/fs/reiser4/jnode.c
+++ b/fs/reiser4/jnode.c
@@ -1846,7 +1846,7 @@ static void info_jnode(const char *prefix /* prefix to print */ ,
 	}
 
 	printk
-	    ("%s: %p: state: %lx: [%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s], level: %i,"
+	    ("%s: %p: state: %lx: [%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s], level: %i,"
 	     " block: %s, d_count: %d, x_count: %d, "
 	     "pg: %p, atom: %p, lock: %i:%i, type: %s, ", prefix, node,
 	     node->state,
@@ -1863,7 +1863,6 @@ static void info_jnode(const char *prefix /* prefix to print */ ,
 	     jnode_state_name(node, JNODE_RIP),
 	     jnode_state_name(node, JNODE_MISSED_IN_CAPTURE),
 	     jnode_state_name(node, JNODE_WRITEBACK),
-	     jnode_state_name(node, JNODE_NEW),
 	     jnode_state_name(node, JNODE_DKSET),
 	     jnode_state_name(node, JNODE_REPACK),
 	     jnode_state_name(node, JNODE_CLUSTER_PAGE),
-- 
2.6.2

Re: [PATCHv2 1/3] reiser4: remove last traces of JNODE_NEW in the debugging code.

[Edward Shishkin]

OK

On 10/25/2015 01:02 AM, Ivan Shapovalov wrote:
> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> ---
>   fs/reiser4/jnode.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/reiser4/jnode.c b/fs/reiser4/jnode.c
> index e0c9e0c..43af7e9 100644
> --- a/fs/reiser4/jnode.c
> +++ b/fs/reiser4/jnode.c
> @@ -1846,7 +1846,7 @@ static void info_jnode(const char *prefix /* prefix to print */ ,
>   	}
>   
>   	printk
> -	    ("%s: %p: state: %lx: [%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s], level: %i,"
> +	    ("%s: %p: state: %lx: [%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s], level: %i,"
>   	     " block: %s, d_count: %d, x_count: %d, "
>   	     "pg: %p, atom: %p, lock: %i:%i, type: %s, ", prefix, node,
>   	     node->state,
> @@ -1863,7 +1863,6 @@ static void info_jnode(const char *prefix /* prefix to print */ ,
>   	     jnode_state_name(node, JNODE_RIP),
>   	     jnode_state_name(node, JNODE_MISSED_IN_CAPTURE),
>   	     jnode_state_name(node, JNODE_WRITEBACK),
> -	     jnode_state_name(node, JNODE_NEW),
>   	     jnode_state_name(node, JNODE_DKSET),
>   	     jnode_state_name(node, JNODE_REPACK),
>   	     jnode_state_name(node, JNODE_CLUSTER_PAGE),

[PATCHv2 2/3] reiser4: call account_page_redirty() on re-dirtying pages before giving them to entd.

[Ivan Shapovalov]

I guess we want that to rollback effects of account_page_dirtied()
in set_page_dirty_notag().

Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
---
 fs/reiser4/entd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/reiser4/entd.c b/fs/reiser4/entd.c
index bff4bfa..c254e71 100644
--- a/fs/reiser4/entd.c
+++ b/fs/reiser4/entd.c
@@ -303,6 +303,7 @@ int write_page_by_ent(struct page *page, struct writeback_control *wbc)
 	 * write it - it will remain dirty
 	 */
 	set_page_dirty_notag(page);
+	account_page_redirty(page);
 
 	/*
 	 * pin inode in memory, unlock page, entd_flush will iput. We can not
-- 
2.6.2

Re: [PATCHv2 2/3] reiser4: call account_page_redirty() on re-dirtying pages before giving them to entd.

[Edward Shishkin]

OK.

On 10/25/2015 01:02 AM, Ivan Shapovalov wrote:
> I guess we want that to rollback effects of account_page_dirtied()
> in set_page_dirty_notag().
>
> Signed-off-by: Ivan Shapovalov <intelfx100@gmail.com>
> ---
>   fs/reiser4/entd.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/fs/reiser4/entd.c b/fs/reiser4/entd.c
> index bff4bfa..c254e71 100644
> --- a/fs/reiser4/entd.c
> +++ b/fs/reiser4/entd.c
> @@ -303,6 +303,7 @@ int write_page_by_ent(struct page *page, struct writeback_control *wbc)
>   	 * write it - it will remain dirty
>   	 */
>   	set_page_dirty_notag(page);
> +	account_page_redirty(page);
>   
>   	/*
>   	 * pin inode in memory, unlock page, entd_flush will iput. We can not

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

[Ivan Shapovalov]

This is responsible for an oops in v4.2 in
write_jnodes_to_disk_extent() -> set_page_writeback().

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]);
 		unlock_page(pages[i]);
 		result = find_or_create_extent(pages[i]);
 		if (result) {
-- 
2.6.2

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

[Edward Shishkin]

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().
>
> 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?

Thanks,
Edward.


>   		unlock_page(pages[i]);
>   		result = find_or_create_extent(pages[i]);
>   		if (result) {

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

[Edward Shishkin]

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

Thanks,
Edward.


>>
>> 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?
>
> Thanks,
> Edward.
>
>
>>           unlock_page(pages[i]);
>>           result = find_or_create_extent(pages[i]);
>>           if (result) {
>

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

[Edward Shishkin]

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.

Thanks,
Edward.

>
>
>>>
>>> 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?
>>
>> Thanks,
>> Edward.
>>
>>
>>>           unlock_page(pages[i]);
>>>           result = find_or_create_extent(pages[i]);
>>>           if (result) {
>>
>

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

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

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?
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?

Thanks,
-- 
Ivan Shapovalov / intelfx /

> > > >           unlock_page(pages[i]);
> > > >           result = find_or_create_extent(pages[i]);
> > > >           if (result) {

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

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

[Edward Shishkin]

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.

Re: [PATCHv2 0/3] reiser4: another batch of fixes for 4.2.

[doiggl]

On Sun, 25 Oct 2015 02:02:17 +0300, Ivan Shapovalov <intelfx100@gmail.com>
wrote:
> These are the fixes for issues I've discovered during migration to 4.2.
> Last one is a real issue causing a reproducible oops in my
configuration,
> first two are byproducts.
> 
> v2: The 3/3 is now a real fix instead of a blind duct-tape fixup.
> 
> Ivan Shapovalov (3):
>   reiser4: remove last traces of JNODE_NEW in the debugging code.
>   reiser4: call account_page_redirty() on re-dirtying pages before
giving
>   them to entd.
>   reiser4: mark pages created during tail2extent conversion as dirty.
> 
>  fs/reiser4/entd.c                        | 1 +
>  fs/reiser4/jnode.c                       | 3 +--
>  fs/reiser4/plugin/file/tail_conversion.c | 1 +
>  3 files changed, 3 insertions(+), 2 deletions(-)

#

Question.
Are these included in reiser4-for-4.3.0.patch  ?
If not, can they be included in next patchset ?

--Glenn