[PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Takafumi Kubota]

This is actually inspired by Filipe's patch(55e3bd2e0c2e1).

When submit_extent_page() in __extent_writepage_io() fails,
Btrfs misses clearing a writeback bit of the failed page.
This causes the false under-writeback page.
Then, another sync task hangs in filemap_fdatawait_range(),
because it waits the false under-writeback page.

CPU0                            CPU1

__extent_writepage_io()
  ret = submit_extent_page() // fail

  if (ret)
    SetPageError(page)
    // miss clearing the writeback bit

                                sync()
                                  ...
                                  filemap_fdatawait_range()
                                    wait_on_page_writeback(page);
                                    // wait the false under-writeback page

Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
---
 fs/btrfs/extent_io.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1e67723..ef9793b 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
 					 bdev, &epd->bio, max_nr,
 					 end_bio_extent_writepage,
 					 0, 0, 0, false);
-		if (ret)
+		if (ret) {
 			SetPageError(page);
+			end_page_writeback(page);
+		}
 
 		cur = cur + iosize;
 		pg_offset += iosize;
-- 
1.9.3

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Liu Bo]

On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
> This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
> 
> When submit_extent_page() in __extent_writepage_io() fails,
> Btrfs misses clearing a writeback bit of the failed page.
> This causes the false under-writeback page.
> Then, another sync task hangs in filemap_fdatawait_range(),
> because it waits the false under-writeback page.
> 
> CPU0                            CPU1
> 
> __extent_writepage_io()
>   ret = submit_extent_page() // fail
> 
>   if (ret)
>     SetPageError(page)
>     // miss clearing the writeback bit
> 
>                                 sync()
>                                   ...
>                                   filemap_fdatawait_range()
>                                     wait_on_page_writeback(page);
>                                     // wait the false under-writeback page
> 
> Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
> ---
>  fs/btrfs/extent_io.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 1e67723..ef9793b 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
>  					 bdev, &epd->bio, max_nr,
>  					 end_bio_extent_writepage,
>  					 0, 0, 0, false);
> -		if (ret)
> +		if (ret) {
>  			SetPageError(page);
> +			end_page_writeback(page);
> +		}

OK...this could be complex as we don't know which part in
submit_extent_page gets the error, if the page has been added into bio
and bio_end would call end_page_writepage(page) as well, so whichever
comes later, the BUG() in end_page_writeback() would complain.

Looks like commit 55e3bd2e0c2e1 also has the same problem although I
gave it my reviewed-by.

Thanks,

-liubo

>  
>  		cur = cur + iosize;
>  		pg_offset += iosize;
> -- 
> 1.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[takafumi-sslab]

Thanks for your replying.

I understand this bug is more complicated than I expected.
I classify error cases under submit_extent_page() below

A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
I first assumed this case and sent the mail.
When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
In this case, bio_endio() is not called and the page's writeback bit 
remains.
So, there is a need to call end_page_writeback() in the error handling.

B: errors under submit_one_bio() of submit_extent_page()
Errors that occur under submit_one_bio() handles at bio_endio(), and 
bio_endio() would call end_page_writeback().

Therefore, as you mentioned in the last mail, simply adding 
end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can 
conflict in the case of B.
To avoid such conflict, one easy solution is adding PageWriteback() 
check too.

How do you think of this solution?

Sincerely,

On 2016/12/22 15:20, Liu Bo wrote:
> On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
>> This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
>>
>> When submit_extent_page() in __extent_writepage_io() fails,
>> Btrfs misses clearing a writeback bit of the failed page.
>> This causes the false under-writeback page.
>> Then, another sync task hangs in filemap_fdatawait_range(),
>> because it waits the false under-writeback page.
>>
>> CPU0                            CPU1
>>
>> __extent_writepage_io()
>>    ret = submit_extent_page() // fail
>>
>>    if (ret)
>>      SetPageError(page)
>>      // miss clearing the writeback bit
>>
>>                                  sync()
>>                                    ...
>>                                    filemap_fdatawait_range()
>>                                      wait_on_page_writeback(page);
>>                                      // wait the false under-writeback page
>>
>> Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
>> ---
>>   fs/btrfs/extent_io.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 1e67723..ef9793b 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
>>   					 bdev, &epd->bio, max_nr,
>>   					 end_bio_extent_writepage,
>>   					 0, 0, 0, false);
>> -		if (ret)
>> +		if (ret) {
>>   			SetPageError(page);
>> +			end_page_writeback(page);
>> +		}
> OK...this could be complex as we don't know which part in
> submit_extent_page gets the error, if the page has been added into bio
> and bio_end would call end_page_writepage(page) as well, so whichever
> comes later, the BUG() in end_page_writeback() would complain.
>
> Looks like commit 55e3bd2e0c2e1 also has the same problem although I
> gave it my reviewed-by.
>
> Thanks,
>
> -liubo
>
>>   
>>   		cur = cur + iosize;
>>   		pg_offset += iosize;
>> -- 
>> 1.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1012@sslab.ics.keio.jp

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Liu Bo]

On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
> Thanks for your replying.
> 
> I understand this bug is more complicated than I expected.
> I classify error cases under submit_extent_page() below
> 
> A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
> I first assumed this case and sent the mail.
> When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
> Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
> In this case, bio_endio() is not called and the page's writeback bit
> remains.
> So, there is a need to call end_page_writeback() in the error handling.
> 
> B: errors under submit_one_bio() of submit_extent_page()
> Errors that occur under submit_one_bio() handles at bio_endio(), and
> bio_endio() would call end_page_writeback().
> 
> Therefore, as you mentioned in the last mail, simply adding
> end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
> conflict in the case of B.
> To avoid such conflict, one easy solution is adding PageWriteback() check
> too.
> 
> How do you think of this solution?

(sorry for the late reply.)

I think its caller, "__extent_writepage", has covered the above case
by setting page writeback again.

Thanks,

-liubo
> 
> Sincerely,
> 
> On 2016/12/22 15:20, Liu Bo wrote:
> > On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
> > > This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
> > > 
> > > When submit_extent_page() in __extent_writepage_io() fails,
> > > Btrfs misses clearing a writeback bit of the failed page.
> > > This causes the false under-writeback page.
> > > Then, another sync task hangs in filemap_fdatawait_range(),
> > > because it waits the false under-writeback page.
> > > 
> > > CPU0                            CPU1
> > > 
> > > __extent_writepage_io()
> > >    ret = submit_extent_page() // fail
> > > 
> > >    if (ret)
> > >      SetPageError(page)
> > >      // miss clearing the writeback bit
> > > 
> > >                                  sync()
> > >                                    ...
> > >                                    filemap_fdatawait_range()
> > >                                      wait_on_page_writeback(page);
> > >                                      // wait the false under-writeback page
> > > 
> > > Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
> > > ---
> > >   fs/btrfs/extent_io.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > index 1e67723..ef9793b 100644
> > > --- a/fs/btrfs/extent_io.c
> > > +++ b/fs/btrfs/extent_io.c
> > > @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
> > >   					 bdev->bio, max_nr,
> > >   					 end_bio_extent_writepage,
> > >   					 0, 0, 0, false);
> > > -		if (ret)
> > > +		if (ret) {
> > >   			SetPageError(page);
> > > +			end_page_writeback(page);
> > > +		}
> > OK...this could be complex as we don't know which part in
> > submit_extent_page gets the error, if the page has been added into bio
> > and bio_end would call end_page_writepage(page) as well, so whichever
> > comes later, the BUG() in end_page_writeback() would complain.
> > 
> > Looks like commit 55e3bd2e0c2e1 also has the same problem although I
> > gave it my reviewed-by.
> > 
> > Thanks,
> > 
> > -liubo
> > 
> > >   		cur = cur + iosize;
> > >   		pg_offset += iosize;
> > > -- 
> > > 1.9.3
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Keio University
> System Software Laboratory
> Takafumi Kubota
> takafumi.kubota1012@sslab.ics.keio.jp
> 

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[takafumi-sslab]

Thanks for your reply.

I think you mentioned about the below if-block in __extent_writepage().

if (nr == 0) {
         /* make sure the mapping tag for page dirty gets cleared */
         set_page_writeback(page);
         end_page_writeback(page);
}

However, this if-block only works when nr is 0, and does not work in the 
case we indicated.
According to the below codes that we excerpt from 
__extent_writepage_io(), nr is incremented even if submit_extent_page() 
fails.
Therefore, we should safely clear the writeback bit in the error 
handling after the fail of submit_extent_page() call

     while (cur <= end) {
         ...
         ret = submit_extent_page(...);
         if (ret)
             SetPageError(page);

         cur = cur + iosize;
         pg_offset += iosize;
         nr++;
     }
done:
     *nr_ret = nr; // *nr_ret is nr of __extent_writepage()

Sincerely,

On 2017/01/31 5:09, Liu Bo wrote:
> On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
>> Thanks for your replying.
>>
>> I understand this bug is more complicated than I expected.
>> I classify error cases under submit_extent_page() below
>>
>> A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
>> I first assumed this case and sent the mail.
>> When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
>> Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
>> In this case, bio_endio() is not called and the page's writeback bit
>> remains.
>> So, there is a need to call end_page_writeback() in the error handling.
>>
>> B: errors under submit_one_bio() of submit_extent_page()
>> Errors that occur under submit_one_bio() handles at bio_endio(), and
>> bio_endio() would call end_page_writeback().
>>
>> Therefore, as you mentioned in the last mail, simply adding
>> end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
>> conflict in the case of B.
>> To avoid such conflict, one easy solution is adding PageWriteback() check
>> too.
>>
>> How do you think of this solution?
> (sorry for the late reply.)
>
> I think its caller, "__extent_writepage", has covered the above case
> by setting page writeback again.
>
> Thanks,
>
> -liubo
>> Sincerely,
>>
>> On 2016/12/22 15:20, Liu Bo wrote:
>>> On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
>>>> This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
>>>>
>>>> When submit_extent_page() in __extent_writepage_io() fails,
>>>> Btrfs misses clearing a writeback bit of the failed page.
>>>> This causes the false under-writeback page.
>>>> Then, another sync task hangs in filemap_fdatawait_range(),
>>>> because it waits the false under-writeback page.
>>>>
>>>> CPU0                            CPU1
>>>>
>>>> __extent_writepage_io()
>>>>     ret = submit_extent_page() // fail
>>>>
>>>>     if (ret)
>>>>       SetPageError(page)
>>>>       // miss clearing the writeback bit
>>>>
>>>>                                   sync()
>>>>                                     ...
>>>>                                     filemap_fdatawait_range()
>>>>                                       wait_on_page_writeback(page);
>>>>                                       // wait the false under-writeback page
>>>>
>>>> Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
>>>> ---
>>>>    fs/btrfs/extent_io.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>> index 1e67723..ef9793b 100644
>>>> --- a/fs/btrfs/extent_io.c
>>>> +++ b/fs/btrfs/extent_io.c
>>>> @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
>>>>    					 bdev->bio, max_nr,
>>>>    					 end_bio_extent_writepage,
>>>>    					 0, 0, 0, false);
>>>> -		if (ret)
>>>> +		if (ret) {
>>>>    			SetPageError(page);
>>>> +			end_page_writeback(page);
>>>> +		}
>>> OK...this could be complex as we don't know which part in
>>> submit_extent_page gets the error, if the page has been added into bio
>>> and bio_end would call end_page_writepage(page) as well, so whichever
>>> comes later, the BUG() in end_page_writeback() would complain.
>>>
>>> Looks like commit 55e3bd2e0c2e1 also has the same problem although I
>>> gave it my reviewed-by.
>>>
>>> Thanks,
>>>
>>> -liubo
>>>
>>>>    		cur = cur + iosize;
>>>>    		pg_offset += iosize;
>>>> -- 
>>>> 1.9.3
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> -- 
>> Keio University
>> System Software Laboratory
>> Takafumi Kubota
>> takafumi.kubota1012@sslab.ics.keio.jp
>>

-- 
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1012@sslab.ics.keio.jp

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Liu Bo]

On Wed, Feb 01, 2017 at 12:27:24PM +0900, takafumi-sslab wrote:
> Thanks for your reply.
> 
> I think you mentioned about the below if-block in __extent_writepage().
> 
> if (nr == 0) {
>         /* make sure the mapping tag for page dirty gets cleared */
>         set_page_writeback(page);
>         end_page_writeback(page);
> }

Right.

> 
> However, this if-block only works when nr is 0, and does not work in the
> case we indicated.
> According to the below codes that we excerpt from __extent_writepage_io(),
> nr is incremented even if submit_extent_page() fails.
> Therefore, we should safely clear the writeback bit in the error handling
> after the fail of submit_extent_page() call
> 
>     while (cur <= end) {
>         ...
>         ret = submit_extent_page(...);
>         if (ret)
>             SetPageError(page);
> 
>         cur = cur + iosize;
>         pg_offset += iosize;
>         nr++;
>     }
> done:
>     *nr_ret = nr; // *nr_ret is nr of __extent_writepage()

It'd be either nr == 0 or nr == 1,

right now (nr > 1) couldn't happen on btrfs because it assumes
blocksize == PAGE_SIZE, so we could only submit_extent_page once for
each page, and bio_ret is used to batch bio submit.

(But it could be changed after subpagesize block patchset, and there is
more work rather than just adding a end_page_writeback, e.g. writepage
endio also needs to be updated).

Thanks,

-liubo
> 
> Sincerely,
> 
> On 2017/01/31 5:09, Liu Bo wrote:
> > On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
> > > Thanks for your replying.
> > > 
> > > I understand this bug is more complicated than I expected.
> > > I classify error cases under submit_extent_page() below
> > > 
> > > A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
> > > I first assumed this case and sent the mail.
> > > When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
> > > Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
> > > In this case, bio_endio() is not called and the page's writeback bit
> > > remains.
> > > So, there is a need to call end_page_writeback() in the error handling.
> > > 
> > > B: errors under submit_one_bio() of submit_extent_page()
> > > Errors that occur under submit_one_bio() handles at bio_endio(), and
> > > bio_endio() would call end_page_writeback().
> > > 
> > > Therefore, as you mentioned in the last mail, simply adding
> > > end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
> > > conflict in the case of B.
> > > To avoid such conflict, one easy solution is adding PageWriteback() check
> > > too.
> > > 
> > > How do you think of this solution?
> > (sorry for the late reply.)
> > 
> > I think its caller, "__extent_writepage", has covered the above case
> > by setting page writeback again.
> > 
> > Thanks,
> > 
> > -liubo
> > > Sincerely,
> > > 
> > > On 2016/12/22 15:20, Liu Bo wrote:
> > > > On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
> > > > > This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
> > > > > 
> > > > > When submit_extent_page() in __extent_writepage_io() fails,
> > > > > Btrfs misses clearing a writeback bit of the failed page.
> > > > > This causes the false under-writeback page.
> > > > > Then, another sync task hangs in filemap_fdatawait_range(),
> > > > > because it waits the false under-writeback page.
> > > > > 
> > > > > CPU0                            CPU1
> > > > > 
> > > > > __extent_writepage_io()
> > > > >     ret = submit_extent_page() // fail
> > > > > 
> > > > >     if (ret)
> > > > >       SetPageError(page)
> > > > >       // miss clearing the writeback bit
> > > > > 
> > > > >                                   sync()
> > > > >                                     ...
> > > > >                                     filemap_fdatawait_range()
> > > > >                                       wait_on_page_writeback(page);
> > > > >                                       // wait the false under-writeback page
> > > > > 
> > > > > Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
> > > > > ---
> > > > >    fs/btrfs/extent_io.c | 4 +++-
> > > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > > > index 1e67723..ef9793b 100644
> > > > > --- a/fs/btrfs/extent_io.c
> > > > > +++ b/fs/btrfs/extent_io.c
> > > > > @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
> > > > >    					 bdev->bio, max_nr,
> > > > >    					 end_bio_extent_writepage,
> > > > >    					 0, 0, 0, false);
> > > > > -		if (ret)
> > > > > +		if (ret) {
> > > > >    			SetPageError(page);
> > > > > +			end_page_writeback(page);
> > > > > +		}
> > > > OK...this could be complex as we don't know which part in
> > > > submit_extent_page gets the error, if the page has been added into bio
> > > > and bio_end would call end_page_writepage(page) as well, so whichever
> > > > comes later, the BUG() in end_page_writeback() would complain.
> > > > 
> > > > Looks like commit 55e3bd2e0c2e1 also has the same problem although I
> > > > gave it my reviewed-by.
> > > > 
> > > > Thanks,
> > > > 
> > > > -liubo
> > > > 
> > > > >    		cur = cur + iosize;
> > > > >    		pg_offset += iosize;
> > > > > -- 
> > > > > 1.9.3
> > > > > 
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > -- 
> > > Keio University
> > > System Software Laboratory
> > > Takafumi Kubota
> > > takafumi.kubota1012@sslab.ics.keio.jp
> > > 
> 
> -- 
> Keio University
> System Software Laboratory
> Takafumi Kubota
> takafumi.kubota1012@sslab.ics.keio.jp
> 

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[takafumi-sslab]

> (But it could be changed after subpagesize block patchset, and there is
> more work rather than just adding a end_page_writeback, e.g. writepage
> endio also needs to be updated).

Ok... the discussion become complicated.
So, let me make this clear.

you think
a) this is a bug;
we need to clear the writeback bit in the error handling if the bit remains.

b) however, the way of fixing this bug has some concerns.　( and now we 
discuss the best solution )

Is my understanding correct?

Sincerely,

-takafumi
>
> Thanks,
>
> -liubo
>> Sincerely,
>>
>> On 2017/01/31 5:09, Liu Bo wrote:
>>> On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
>>>> Thanks for your replying.
>>>>
>>>> I understand this bug is more complicated than I expected.
>>>> I classify error cases under submit_extent_page() below
>>>>
>>>> A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
>>>> I first assumed this case and sent the mail.
>>>> When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
>>>> Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
>>>> In this case, bio_endio() is not called and the page's writeback bit
>>>> remains.
>>>> So, there is a need to call end_page_writeback() in the error handling.
>>>>
>>>> B: errors under submit_one_bio() of submit_extent_page()
>>>> Errors that occur under submit_one_bio() handles at bio_endio(), and
>>>> bio_endio() would call end_page_writeback().
>>>>
>>>> Therefore, as you mentioned in the last mail, simply adding
>>>> end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
>>>> conflict in the case of B.
>>>> To avoid such conflict, one easy solution is adding PageWriteback() check
>>>> too.
>>>>
>>>> How do you think of this solution?
>>> (sorry for the late reply.)
>>>
>>> I think its caller, "__extent_writepage", has covered the above case
>>> by setting page writeback again.
>>>
>>> Thanks,
>>>
>>> -liubo
>>>> Sincerely,
>>>>
>>>> On 2016/12/22 15:20, Liu Bo wrote:
>>>>> On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
>>>>>> This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
>>>>>>
>>>>>> When submit_extent_page() in __extent_writepage_io() fails,
>>>>>> Btrfs misses clearing a writeback bit of the failed page.
>>>>>> This causes the false under-writeback page.
>>>>>> Then, another sync task hangs in filemap_fdatawait_range(),
>>>>>> because it waits the false under-writeback page.
>>>>>>
>>>>>> CPU0                            CPU1
>>>>>>
>>>>>> __extent_writepage_io()
>>>>>>      ret = submit_extent_page() // fail
>>>>>>
>>>>>>      if (ret)
>>>>>>        SetPageError(page)
>>>>>>        // miss clearing the writeback bit
>>>>>>
>>>>>>                                    sync()
>>>>>>                                      ...
>>>>>>                                      filemap_fdatawait_range()
>>>>>>                                        wait_on_page_writeback(page);
>>>>>>                                        // wait the false under-writeback page
>>>>>>
>>>>>> Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
>>>>>> ---
>>>>>>     fs/btrfs/extent_io.c | 4 +++-
>>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>>> index 1e67723..ef9793b 100644
>>>>>> --- a/fs/btrfs/extent_io.c
>>>>>> +++ b/fs/btrfs/extent_io.c
>>>>>> @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
>>>>>>     					 bdev->bio, max_nr,
>>>>>>     					 end_bio_extent_writepage,
>>>>>>     					 0, 0, 0, false);
>>>>>> -		if (ret)
>>>>>> +		if (ret) {
>>>>>>     			SetPageError(page);
>>>>>> +			end_page_writeback(page);
>>>>>> +		}
>>>>> OK...this could be complex as we don't know which part in
>>>>> submit_extent_page gets the error, if the page has been added into bio
>>>>> and bio_end would call end_page_writepage(page) as well, so whichever
>>>>> comes later, the BUG() in end_page_writeback() would complain.
>>>>>
>>>>> Looks like commit 55e3bd2e0c2e1 also has the same problem although I
>>>>> gave it my reviewed-by.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -liubo
>>>>>
>>>>>>     		cur = cur + iosize;
>>>>>>     		pg_offset += iosize;
>>>>>> -- 
>>>>>> 1.9.3
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>> -- 
>>>> Keio University
>>>> System Software Laboratory
>>>> Takafumi Kubota
>>>> takafumi.kubota1012@sslab.ics.keio.jp
>>>>
>> -- 
>> Keio University
>> System Software Laboratory
>> Takafumi Kubota
>> takafumi.kubota1012@sslab.ics.keio.jp
>>

-- 
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1012@sslab.ics.keio.jp

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Liu Bo]

On Sat, Feb 04, 2017 at 09:42:17PM +0900, takafumi-sslab wrote:
> 
> > (But it could be changed after subpagesize block patchset, and there is
> > more work rather than just adding a end_page_writeback, e.g. writepage
> > endio also needs to be updated).
> 
> Ok... the discussion become complicated.
> So, let me make this clear.
> 
> you think
> a) this is a bug;
> we need to clear the writeback bit in the error handling if the bit remains.
> 
> b) however, the way of fixing this bug has some concerns.　( and now we
> discuss the best solution )
> 
> Is my understanding correct?

Sorry for making you confused even more, to clarify it, I don't think
the bug could exist in the current btrfs because blocksize is equal to
PAGE_SIZE so that @nr in __extent_writepage could only be 0 or 1.

a) __extent_writepage has handled the case when nr == 0.
b) when nr == 1, the page is marked with writeback bit and added into a
   bio, thus we have bio_end to deal with page bits.

So I don't think the patch is necessary for now.

But as I said, the fact (nr == 0 or 1) would be changed if the
subpagesize blocksize is supported.

Thanks,

-liubo

> 
> Sincerely,
> 
> -takafumi
> > 
> > Thanks,
> > 
> > -liubo
> > > Sincerely,
> > > 
> > > On 2017/01/31 5:09, Liu Bo wrote:
> > > > On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
> > > > > Thanks for your replying.
> > > > > 
> > > > > I understand this bug is more complicated than I expected.
> > > > > I classify error cases under submit_extent_page() below
> > > > > 
> > > > > A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
> > > > > I first assumed this case and sent the mail.
> > > > > When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
> > > > > Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
> > > > > In this case, bio_endio() is not called and the page's writeback bit
> > > > > remains.
> > > > > So, there is a need to call end_page_writeback() in the error handling.
> > > > > 
> > > > > B: errors under submit_one_bio() of submit_extent_page()
> > > > > Errors that occur under submit_one_bio() handles at bio_endio(), and
> > > > > bio_endio() would call end_page_writeback().
> > > > > 
> > > > > Therefore, as you mentioned in the last mail, simply adding
> > > > > end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
> > > > > conflict in the case of B.
> > > > > To avoid such conflict, one easy solution is adding PageWriteback() check
> > > > > too.
> > > > > 
> > > > > How do you think of this solution?
> > > > (sorry for the late reply.)
> > > > 
> > > > I think its caller, "__extent_writepage", has covered the above case
> > > > by setting page writeback again.
> > > > 
> > > > Thanks,
> > > > 
> > > > -liubo
> > > > > Sincerely,
> > > > > 
> > > > > On 2016/12/22 15:20, Liu Bo wrote:
> > > > > > On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
> > > > > > > This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
> > > > > > > 
> > > > > > > When submit_extent_page() in __extent_writepage_io() fails,
> > > > > > > Btrfs misses clearing a writeback bit of the failed page.
> > > > > > > This causes the false under-writeback page.
> > > > > > > Then, another sync task hangs in filemap_fdatawait_range(),
> > > > > > > because it waits the false under-writeback page.
> > > > > > > 
> > > > > > > CPU0                            CPU1
> > > > > > > 
> > > > > > > __extent_writepage_io()
> > > > > > >      ret = submit_extent_page() // fail
> > > > > > > 
> > > > > > >      if (ret)
> > > > > > >        SetPageError(page)
> > > > > > >        // miss clearing the writeback bit
> > > > > > > 
> > > > > > >                                    sync()
> > > > > > >                                      ...
> > > > > > >                                      filemap_fdatawait_range()
> > > > > > >                                        wait_on_page_writeback(page);
> > > > > > >                                        // wait the false under-writeback page
> > > > > > > 
> > > > > > > Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
> > > > > > > ---
> > > > > > >     fs/btrfs/extent_io.c | 4 +++-
> > > > > > >     1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > > > > > index 1e67723..ef9793b 100644
> > > > > > > --- a/fs/btrfs/extent_io.c
> > > > > > > +++ b/fs/btrfs/extent_io.c
> > > > > > > @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
> > > > > > >     					 bdev->bio, max_nr,
> > > > > > >     					 end_bio_extent_writepage,
> > > > > > >     					 0, 0, 0, false);
> > > > > > > -		if (ret)
> > > > > > > +		if (ret) {
> > > > > > >     			SetPageError(page);
> > > > > > > +			end_page_writeback(page);
> > > > > > > +		}
> > > > > > OK...this could be complex as we don't know which part in
> > > > > > submit_extent_page gets the error, if the page has been added into bio
> > > > > > and bio_end would call end_page_writepage(page) as well, so whichever
> > > > > > comes later, the BUG() in end_page_writeback() would complain.
> > > > > > 
> > > > > > Looks like commit 55e3bd2e0c2e1 also has the same problem although I
> > > > > > gave it my reviewed-by.
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > -liubo
> > > > > > 
> > > > > > >     		cur = cur + iosize;
> > > > > > >     		pg_offset += iosize;
> > > > > > > -- 
> > > > > > > 1.9.3
> > > > > > > 
> > > > > > > --
> > > > > > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > > > > > > the body of a message to majordomo@vger.kernel.org
> > > > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > -- 
> > > > > Keio University
> > > > > System Software Laboratory
> > > > > Takafumi Kubota
> > > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > > 
> > > -- 
> > > Keio University
> > > System Software Laboratory
> > > Takafumi Kubota
> > > takafumi.kubota1012@sslab.ics.keio.jp
> > > 
> 
> -- 
> Keio University
> System Software Laboratory
> Takafumi Kubota
> takafumi.kubota1012@sslab.ics.keio.jp
> 

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[takafumi-sslab]

On 2017/02/06 12:35, Liu Bo wrote:
> a) __extent_writepage has handled the case when nr == 0.

Yes, I agree this.

> b) when nr == 1, the page is marked with writeback bit and added into a
>     bio, thus we have bio_end to deal with page bits.

However, I don't think this is always correct,
because, as I said before as the Error A, when (bio_ret == NULL or 
*bio_ret == NULL) and btrfs_bio_alloc() fails, the page is not added 
into a bio.
Thus, bio_end does not deal with the page's bit, and the writeback bit 
remains with nr == 1.

To confirm this, I inject a fault to mimic the fail of btrfs_bio_alloc().
I can reproduce the hang at the kernel ver4.10-rc7 that commit id is 
d5adbfcd5f7bcc6fa58a41c5c5ada0e5c826ce2c
and  Ftrace log messages are like below

a.out-2867  : __extent_writepage: call __extent_writepage_io: 
page:0xffffea000006d200(writeback:0), nr:0, logged at just previous to 
line 3523 of extent_io.c
a.out-2867  : __extent_writepage_io: call submit_extent_page: 
page:0xffffea000006d200(writeback:1), nr:0, bio_ret:0xffffc90003bd7d40, 
*bio_ret: (null), logged at just previous to line 3443 of extent_io.c
a.out-2867  : submit_extent_page.isra.53: Mimic Error A: 
btrfs_bio_alloc() fails and submit_extent_page() return -ENOMEM, logged 
at just previous to line 2807 of extent_io.c
a.out-2867  : __extent_writepage_io: after call submit_extent_page: 
page:0xffffea000006d200(writeback:1), nr:0, bio_ret:0xffffc90003bd7d40, 
*bio_ret: (null), logged at just next to line 3447 of extent_io.c
a.out-2867  : __extent_writepage_io: nr == 1: 
page:0xffffea000006d200(writeback:1), nr:1, bio_ret:0xffffc90003de7d40, 
logged at just next to line 3456 of extent_io.c //nr == 1 and the 
writeback bit remains
a.out-2867  : __extent_writepage: after call __extent_writepage_io: 
page:0xffffea000006d200(writeback:1), nr:1, logged at just next to line 
3524 of extent_io.c // __extent_writepage does not handle, because nr == 1
...
sync-2868   : __filemap_fdatawait_range: wait_on_page_writeback: 
page:0xffffea000006d200, logged at just previous to line 404 of 
mm/filemap.c // Then, sync hangs

Sincerely,

-takafumi
>
> So I don't think the patch is necessary for now.
>
> But as I said, the fact (nr == 0 or 1) would be changed if the
> subpagesize blocksize is supported.
>
> Thanks,
>
> -liubo
>
>> Sincerely,
>>
>> -takafumi
>>> Thanks,
>>>
>>> -liubo
>>>> Sincerely,
>>>>
>>>> On 2017/01/31 5:09, Liu Bo wrote:
>>>>> On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
>>>>>> Thanks for your replying.
>>>>>>
>>>>>> I understand this bug is more complicated than I expected.
>>>>>> I classify error cases under submit_extent_page() below
>>>>>>
>>>>>> A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
>>>>>> I first assumed this case and sent the mail.
>>>>>> When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
>>>>>> Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
>>>>>> In this case, bio_endio() is not called and the page's writeback bit
>>>>>> remains.
>>>>>> So, there is a need to call end_page_writeback() in the error handling.
>>>>>>
>>>>>> B: errors under submit_one_bio() of submit_extent_page()
>>>>>> Errors that occur under submit_one_bio() handles at bio_endio(), and
>>>>>> bio_endio() would call end_page_writeback().
>>>>>>
>>>>>> Therefore, as you mentioned in the last mail, simply adding
>>>>>> end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
>>>>>> conflict in the case of B.
>>>>>> To avoid such conflict, one easy solution is adding PageWriteback() check
>>>>>> too.
>>>>>>
>>>>>> How do you think of this solution?
>>>>> (sorry for the late reply.)
>>>>>
>>>>> I think its caller, "__extent_writepage", has covered the above case
>>>>> by setting page writeback again.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -liubo
>>>>>> Sincerely,
>>>>>>
>>>>>> On 2016/12/22 15:20, Liu Bo wrote:
>>>>>>> On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
>>>>>>>> This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
>>>>>>>>
>>>>>>>> When submit_extent_page() in __extent_writepage_io() fails,
>>>>>>>> Btrfs misses clearing a writeback bit of the failed page.
>>>>>>>> This causes the false under-writeback page.
>>>>>>>> Then, another sync task hangs in filemap_fdatawait_range(),
>>>>>>>> because it waits the false under-writeback page.
>>>>>>>>
>>>>>>>> CPU0                            CPU1
>>>>>>>>
>>>>>>>> __extent_writepage_io()
>>>>>>>>       ret = submit_extent_page() // fail
>>>>>>>>
>>>>>>>>       if (ret)
>>>>>>>>         SetPageError(page)
>>>>>>>>         // miss clearing the writeback bit
>>>>>>>>
>>>>>>>>                                     sync()
>>>>>>>>                                       ...
>>>>>>>>                                       filemap_fdatawait_range()
>>>>>>>>                                         wait_on_page_writeback(page);
>>>>>>>>                                         // wait the false under-writeback page
>>>>>>>>
>>>>>>>> Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
>>>>>>>> ---
>>>>>>>>      fs/btrfs/extent_io.c | 4 +++-
>>>>>>>>      1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>>>>> index 1e67723..ef9793b 100644
>>>>>>>> --- a/fs/btrfs/extent_io.c
>>>>>>>> +++ b/fs/btrfs/extent_io.c
>>>>>>>> @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
>>>>>>>>      					 bdev->bio, max_nr,
>>>>>>>>      					 end_bio_extent_writepage,
>>>>>>>>      					 0, 0, 0, false);
>>>>>>>> -		if (ret)
>>>>>>>> +		if (ret) {
>>>>>>>>      			SetPageError(page);
>>>>>>>> +			end_page_writeback(page);
>>>>>>>> +		}
>>>>>>> OK...this could be complex as we don't know which part in
>>>>>>> submit_extent_page gets the error, if the page has been added into bio
>>>>>>> and bio_end would call end_page_writepage(page) as well, so whichever
>>>>>>> comes later, the BUG() in end_page_writeback() would complain.
>>>>>>>
>>>>>>> Looks like commit 55e3bd2e0c2e1 also has the same problem although I
>>>>>>> gave it my reviewed-by.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> -liubo
>>>>>>>
>>>>>>>>      		cur = cur + iosize;
>>>>>>>>      		pg_offset += iosize;
>>>>>>>> -- 
>>>>>>>> 1.9.3
>>>>>>>>
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>> -- 
>>>>>> Keio University
>>>>>> System Software Laboratory
>>>>>> Takafumi Kubota
>>>>>> takafumi.kubota1012@sslab.ics.keio.jp
>>>>>>
>>>> -- 
>>>> Keio University
>>>> System Software Laboratory
>>>> Takafumi Kubota
>>>> takafumi.kubota1012@sslab.ics.keio.jp
>>>>
>> -- 
>> Keio University
>> System Software Laboratory
>> Takafumi Kubota
>> takafumi.kubota1012@sslab.ics.keio.jp
>>

-- 
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1012@sslab.ics.keio.jp

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Liu Bo]

On Mon, Feb 06, 2017 at 02:50:18PM +0900, takafumi-sslab wrote:
> 
> 
> On 2017/02/06 12:35, Liu Bo wrote:
> > a) __extent_writepage has handled the case when nr == 0.
> 
> Yes, I agree this.
> 
> > b) when nr == 1, the page is marked with writeback bit and added into a
> >     bio, thus we have bio_end to deal with page bits.
> 
> However, I don't think this is always correct,
> because, as I said before as the Error A, when (bio_ret == NULL or *bio_ret
> == NULL) and btrfs_bio_alloc() fails, the page is not added into a bio.
> Thus, bio_end does not deal with the page's bit, and the writeback bit
> remains with nr == 1.
> 
> To confirm this, I inject a fault to mimic the fail of btrfs_bio_alloc().
> I can reproduce the hang at the kernel ver4.10-rc7 that commit id is
> d5adbfcd5f7bcc6fa58a41c5c5ada0e5c826ce2c
> and  Ftrace log messages are like below
> 
> a.out-2867  : __extent_writepage: call __extent_writepage_io:
> page:0xffffea000006d200(writeback:0), nr:0, logged at just previous to line
> 3523 of extent_io.c
> a.out-2867  : __extent_writepage_io: call submit_extent_page:
> page:0xffffea000006d200(writeback:1), nr:0, bio_ret:0xffffc90003bd7d40,
> *bio_ret: (null), logged at just previous to line 3443 of extent_io.c
> a.out-2867  : submit_extent_page.isra.53: Mimic Error A: btrfs_bio_alloc()
> fails and submit_extent_page() return -ENOMEM, logged at just previous to
> line 2807 of extent_io.c
> a.out-2867  : __extent_writepage_io: after call submit_extent_page:
> page:0xffffea000006d200(writeback:1), nr:0, bio_ret:0xffffc90003bd7d40,
> *bio_ret: (null), logged at just next to line 3447 of extent_io.c
> a.out-2867  : __extent_writepage_io: nr == 1:
> page:0xffffea000006d200(writeback:1), nr:1, bio_ret:0xffffc90003de7d40,
> logged at just next to line 3456 of extent_io.c //nr == 1 and the writeback
> bit remains
> a.out-2867  : __extent_writepage: after call __extent_writepage_io:
> page:0xffffea000006d200(writeback:1), nr:1, logged at just next to line 3524
> of extent_io.c // __extent_writepage does not handle, because nr == 1
> ...
> sync-2868   : __filemap_fdatawait_range: wait_on_page_writeback:
> page:0xffffea000006d200, logged at just previous to line 404 of mm/filemap.c
> // Then, sync hangs

Thanks for the efforts to prove it, nice.  Obviously I missed that @nr
will even be bumped up by one if ENOMEM, so yes, we need to clear the
writeback bit.

But shouldn't @nr means nr_submitted_successfully?  If a submit fails,
it makes no senses to keep submitting subsequent parts.

Anyway, for now I think the proposed fix is good to solve the problem, so you
may add

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo
> 
> Sincerely,
> 
> -takafumi
> > 
> > So I don't think the patch is necessary for now.
> > 
> > But as I said, the fact (nr == 0 or 1) would be changed if the
> > subpagesize blocksize is supported.
> > 
> > Thanks,
> > 
> > -liubo
> > 
> > > Sincerely,
> > > 
> > > -takafumi
> > > > Thanks,
> > > > 
> > > > -liubo
> > > > > Sincerely,
> > > > > 
> > > > > On 2017/01/31 5:09, Liu Bo wrote:
> > > > > > On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
> > > > > > > Thanks for your replying.
> > > > > > > 
> > > > > > > I understand this bug is more complicated than I expected.
> > > > > > > I classify error cases under submit_extent_page() below
> > > > > > > 
> > > > > > > A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
> > > > > > > I first assumed this case and sent the mail.
> > > > > > > When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
> > > > > > > Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
> > > > > > > In this case, bio_endio() is not called and the page's writeback bit
> > > > > > > remains.
> > > > > > > So, there is a need to call end_page_writeback() in the error handling.
> > > > > > > 
> > > > > > > B: errors under submit_one_bio() of submit_extent_page()
> > > > > > > Errors that occur under submit_one_bio() handles at bio_endio(), and
> > > > > > > bio_endio() would call end_page_writeback().
> > > > > > > 
> > > > > > > Therefore, as you mentioned in the last mail, simply adding
> > > > > > > end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
> > > > > > > conflict in the case of B.
> > > > > > > To avoid such conflict, one easy solution is adding PageWriteback() check
> > > > > > > too.
> > > > > > > 
> > > > > > > How do you think of this solution?
> > > > > > (sorry for the late reply.)
> > > > > > 
> > > > > > I think its caller, "__extent_writepage", has covered the above case
> > > > > > by setting page writeback again.
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > -liubo
> > > > > > > Sincerely,
> > > > > > > 
> > > > > > > On 2016/12/22 15:20, Liu Bo wrote:
> > > > > > > > On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
> > > > > > > > > This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
> > > > > > > > > 
> > > > > > > > > When submit_extent_page() in __extent_writepage_io() fails,
> > > > > > > > > Btrfs misses clearing a writeback bit of the failed page.
> > > > > > > > > This causes the false under-writeback page.
> > > > > > > > > Then, another sync task hangs in filemap_fdatawait_range(),
> > > > > > > > > because it waits the false under-writeback page.
> > > > > > > > > 
> > > > > > > > > CPU0                            CPU1
> > > > > > > > > 
> > > > > > > > > __extent_writepage_io()
> > > > > > > > >       ret = submit_extent_page() // fail
> > > > > > > > > 
> > > > > > > > >       if (ret)
> > > > > > > > >         SetPageError(page)
> > > > > > > > >         // miss clearing the writeback bit
> > > > > > > > > 
> > > > > > > > >                                     sync()
> > > > > > > > >                                       ...
> > > > > > > > >                                       filemap_fdatawait_range()
> > > > > > > > >                                         wait_on_page_writeback(page);
> > > > > > > > >                                         // wait the false under-writeback page
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
> > > > > > > > > ---
> > > > > > > > >      fs/btrfs/extent_io.c | 4 +++-
> > > > > > > > >      1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > > > > > > > index 1e67723..ef9793b 100644
> > > > > > > > > --- a/fs/btrfs/extent_io.c
> > > > > > > > > +++ b/fs/btrfs/extent_io.c
> > > > > > > > > @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
> > > > > > > > >      					 bdev->bio, max_nr,
> > > > > > > > >      					 end_bio_extent_writepage,
> > > > > > > > >      					 0, 0, 0, false);
> > > > > > > > > -		if (ret)
> > > > > > > > > +		if (ret) {
> > > > > > > > >      			SetPageError(page);
> > > > > > > > > +			end_page_writeback(page);
> > > > > > > > > +		}
> > > > > > > > OK...this could be complex as we don't know which part in
> > > > > > > > submit_extent_page gets the error, if the page has been added into bio
> > > > > > > > and bio_end would call end_page_writepage(page) as well, so whichever
> > > > > > > > comes later, the BUG() in end_page_writeback() would complain.
> > > > > > > > 
> > > > > > > > Looks like commit 55e3bd2e0c2e1 also has the same problem although I
> > > > > > > > gave it my reviewed-by.
> > > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > 
> > > > > > > > -liubo
> > > > > > > > 
> > > > > > > > >      		cur = cur + iosize;
> > > > > > > > >      		pg_offset += iosize;
> > > > > > > > > -- 
> > > > > > > > > 1.9.3
> > > > > > > > > 
> > > > > > > > > --
> > > > > > > > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > > > > > > > > the body of a message to majordomo@vger.kernel.org
> > > > > > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > > > -- 
> > > > > > > Keio University
> > > > > > > System Software Laboratory
> > > > > > > Takafumi Kubota
> > > > > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > > > > 
> > > > > -- 
> > > > > Keio University
> > > > > System Software Laboratory
> > > > > Takafumi Kubota
> > > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > > 
> > > -- 
> > > Keio University
> > > System Software Laboratory
> > > Takafumi Kubota
> > > takafumi.kubota1012@sslab.ics.keio.jp
> > > 
> 
> -- 
> Keio University
> System Software Laboratory
> Takafumi Kubota
> takafumi.kubota1012@sslab.ics.keio.jp
> 

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Liu Bo]

On Mon, Feb 06, 2017 at 08:26:54AM -0800, Liu Bo wrote:
> On Mon, Feb 06, 2017 at 02:50:18PM +0900, takafumi-sslab wrote:
> > 
> > 
> > On 2017/02/06 12:35, Liu Bo wrote:
> > > a) __extent_writepage has handled the case when nr == 0.
> > 
> > Yes, I agree this.
> > 
> > > b) when nr == 1, the page is marked with writeback bit and added into a
> > >     bio, thus we have bio_end to deal with page bits.
> > 
> > However, I don't think this is always correct,
> > because, as I said before as the Error A, when (bio_ret == NULL or *bio_ret
> > == NULL) and btrfs_bio_alloc() fails, the page is not added into a bio.
> > Thus, bio_end does not deal with the page's bit, and the writeback bit
> > remains with nr == 1.
> > 
> > To confirm this, I inject a fault to mimic the fail of btrfs_bio_alloc().
> > I can reproduce the hang at the kernel ver4.10-rc7 that commit id is
> > d5adbfcd5f7bcc6fa58a41c5c5ada0e5c826ce2c
> > and  Ftrace log messages are like below
> > 
> > a.out-2867  : __extent_writepage: call __extent_writepage_io:
> > page:0xffffea000006d200(writeback:0), nr:0, logged at just previous to line
> > 3523 of extent_io.c
> > a.out-2867  : __extent_writepage_io: call submit_extent_page:
> > page:0xffffea000006d200(writeback:1), nr:0, bio_ret:0xffffc90003bd7d40,
> > *bio_ret: (null), logged at just previous to line 3443 of extent_io.c
> > a.out-2867  : submit_extent_page.isra.53: Mimic Error A: btrfs_bio_alloc()
> > fails and submit_extent_page() return -ENOMEM, logged at just previous to
> > line 2807 of extent_io.c
> > a.out-2867  : __extent_writepage_io: after call submit_extent_page:
> > page:0xffffea000006d200(writeback:1), nr:0, bio_ret:0xffffc90003bd7d40,
> > *bio_ret: (null), logged at just next to line 3447 of extent_io.c
> > a.out-2867  : __extent_writepage_io: nr == 1:
> > page:0xffffea000006d200(writeback:1), nr:1, bio_ret:0xffffc90003de7d40,
> > logged at just next to line 3456 of extent_io.c //nr == 1 and the writeback
> > bit remains
> > a.out-2867  : __extent_writepage: after call __extent_writepage_io:
> > page:0xffffea000006d200(writeback:1), nr:1, logged at just next to line 3524
> > of extent_io.c // __extent_writepage does not handle, because nr == 1
> > ...
> > sync-2868   : __filemap_fdatawait_range: wait_on_page_writeback:
> > page:0xffffea000006d200, logged at just previous to line 404 of mm/filemap.c
> > // Then, sync hangs
> 
> Thanks for the efforts to prove it, nice.  Obviously I missed that @nr
> will even be bumped up by one if ENOMEM, so yes, we need to clear the
> writeback bit.
> 
> But shouldn't @nr means nr_submitted_successfully?  If a submit fails,
> it makes no senses to keep submitting subsequent parts.
> 
> Anyway, for now I think the proposed fix is good to solve the problem, so you
> may add

One thing to add, we still need to check whether page has writeback bit before
end_page_writeback.

Thanks,

-liubo

> 
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> 
> Thanks,
> 
> -liubo
> > 
> > Sincerely,
> > 
> > -takafumi
> > > 
> > > So I don't think the patch is necessary for now.
> > > 
> > > But as I said, the fact (nr == 0 or 1) would be changed if the
> > > subpagesize blocksize is supported.
> > > 
> > > Thanks,
> > > 
> > > -liubo
> > > 
> > > > Sincerely,
> > > > 
> > > > -takafumi
> > > > > Thanks,
> > > > > 
> > > > > -liubo
> > > > > > Sincerely,
> > > > > > 
> > > > > > On 2017/01/31 5:09, Liu Bo wrote:
> > > > > > > On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
> > > > > > > > Thanks for your replying.
> > > > > > > > 
> > > > > > > > I understand this bug is more complicated than I expected.
> > > > > > > > I classify error cases under submit_extent_page() below
> > > > > > > > 
> > > > > > > > A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
> > > > > > > > I first assumed this case and sent the mail.
> > > > > > > > When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
> > > > > > > > Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
> > > > > > > > In this case, bio_endio() is not called and the page's writeback bit
> > > > > > > > remains.
> > > > > > > > So, there is a need to call end_page_writeback() in the error handling.
> > > > > > > > 
> > > > > > > > B: errors under submit_one_bio() of submit_extent_page()
> > > > > > > > Errors that occur under submit_one_bio() handles at bio_endio(), and
> > > > > > > > bio_endio() would call end_page_writeback().
> > > > > > > > 
> > > > > > > > Therefore, as you mentioned in the last mail, simply adding
> > > > > > > > end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
> > > > > > > > conflict in the case of B.
> > > > > > > > To avoid such conflict, one easy solution is adding PageWriteback() check
> > > > > > > > too.
> > > > > > > > 
> > > > > > > > How do you think of this solution?
> > > > > > > (sorry for the late reply.)
> > > > > > > 
> > > > > > > I think its caller, "__extent_writepage", has covered the above case
> > > > > > > by setting page writeback again.
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > -liubo
> > > > > > > > Sincerely,
> > > > > > > > 
> > > > > > > > On 2016/12/22 15:20, Liu Bo wrote:
> > > > > > > > > On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
> > > > > > > > > > This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
> > > > > > > > > > 
> > > > > > > > > > When submit_extent_page() in __extent_writepage_io() fails,
> > > > > > > > > > Btrfs misses clearing a writeback bit of the failed page.
> > > > > > > > > > This causes the false under-writeback page.
> > > > > > > > > > Then, another sync task hangs in filemap_fdatawait_range(),
> > > > > > > > > > because it waits the false under-writeback page.
> > > > > > > > > > 
> > > > > > > > > > CPU0                            CPU1
> > > > > > > > > > 
> > > > > > > > > > __extent_writepage_io()
> > > > > > > > > >       ret = submit_extent_page() // fail
> > > > > > > > > > 
> > > > > > > > > >       if (ret)
> > > > > > > > > >         SetPageError(page)
> > > > > > > > > >         // miss clearing the writeback bit
> > > > > > > > > > 
> > > > > > > > > >                                     sync()
> > > > > > > > > >                                       ...
> > > > > > > > > >                                       filemap_fdatawait_range()
> > > > > > > > > >                                         wait_on_page_writeback(page);
> > > > > > > > > >                                         // wait the false under-writeback page
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
> > > > > > > > > > ---
> > > > > > > > > >      fs/btrfs/extent_io.c | 4 +++-
> > > > > > > > > >      1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > > > > > > > > index 1e67723..ef9793b 100644
> > > > > > > > > > --- a/fs/btrfs/extent_io.c
> > > > > > > > > > +++ b/fs/btrfs/extent_io.c
> > > > > > > > > > @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
> > > > > > > > > >      					 bdev->bio, max_nr,
> > > > > > > > > >      					 end_bio_extent_writepage,
> > > > > > > > > >      					 0, 0, 0, false);
> > > > > > > > > > -		if (ret)
> > > > > > > > > > +		if (ret) {
> > > > > > > > > >      			SetPageError(page);
> > > > > > > > > > +			end_page_writeback(page);
> > > > > > > > > > +		}
> > > > > > > > > OK...this could be complex as we don't know which part in
> > > > > > > > > submit_extent_page gets the error, if the page has been added into bio
> > > > > > > > > and bio_end would call end_page_writepage(page) as well, so whichever
> > > > > > > > > comes later, the BUG() in end_page_writeback() would complain.
> > > > > > > > > 
> > > > > > > > > Looks like commit 55e3bd2e0c2e1 also has the same problem although I
> > > > > > > > > gave it my reviewed-by.
> > > > > > > > > 
> > > > > > > > > Thanks,
> > > > > > > > > 
> > > > > > > > > -liubo
> > > > > > > > > 
> > > > > > > > > >      		cur = cur + iosize;
> > > > > > > > > >      		pg_offset += iosize;
> > > > > > > > > > -- 
> > > > > > > > > > 1.9.3
> > > > > > > > > > 
> > > > > > > > > > --
> > > > > > > > > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > > > > > > > > > the body of a message to majordomo@vger.kernel.org
> > > > > > > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > > > > -- 
> > > > > > > > Keio University
> > > > > > > > System Software Laboratory
> > > > > > > > Takafumi Kubota
> > > > > > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > > > > > 
> > > > > > -- 
> > > > > > Keio University
> > > > > > System Software Laboratory
> > > > > > Takafumi Kubota
> > > > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > > > 
> > > > -- 
> > > > Keio University
> > > > System Software Laboratory
> > > > Takafumi Kubota
> > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > 
> > 
> > -- 
> > Keio University
> > System Software Laboratory
> > Takafumi Kubota
> > takafumi.kubota1012@sslab.ics.keio.jp
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[takafumi-sslab]

On 2017/02/07 1:34, Liu Bo wrote:

>
> One thing to add, we still need to check whether page has writeback bit before
> end_page_writeback.

Ok, I add PageWriteback check before end_page_writeback.

>>>>>>>>>>> Looks like commit 55e3bd2e0c2e1 also has the same problem although I
>>>>>>>>>>> gave it my reviewed-by.

I also add PageWriteback check in write_one_eb.

Finally, the diff becomes like below.
Is it Ok ?

---

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 4ac383a..aa1908a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3445,8 +3445,11 @@ static noinline_for_stack int 
__extent_writepage_io(struct inode *inode,
  					 bdev, &epd->bio, max_nr,
  					 end_bio_extent_writepage,
  					 0, 0, 0, false);
-		if (ret)
+		if (ret) {
  			SetPageError(page);
+			if (PageWriteback(page))
+				end_page_writeback(page);
+		}

  		cur = cur + iosize;
  		pg_offset += iosize;
@@ -3767,7 +3770,8 @@ static noinline_for_stack int write_one_eb(struct 
extent_buffer *eb,
  		epd->bio_flags = bio_flags;
  		if (ret) {
  			set_btree_ioerr(p);
-			end_page_writeback(p);
+			if (PageWriteback(p))
+				end_page_writeback(p);
  			if (atomic_sub_and_test(num_pages - i, &eb->io_pages))
  				end_extent_buffer_writeback(eb);
  			ret = -EIO;

---


Sincerely,

-takafumi


>
> Thanks,
>
> -liubo
>
>>
>> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
>>
>> Thanks,
>>
>> -liubo
>>>
>>> Sincerely,
>>>
>>> -takafumi
>>>>
>>>> So I don't think the patch is necessary for now.
>>>>
>>>> But as I said, the fact (nr == 0 or 1) would be changed if the
>>>> subpagesize blocksize is supported.
>>>>
>>>> Thanks,
>>>>
>>>> -liubo
>>>>
>>>>> Sincerely,
>>>>>
>>>>> -takafumi
>>>>>> Thanks,
>>>>>>
>>>>>> -liubo
>>>>>>> Sincerely,
>>>>>>>
>>>>>>> On 2017/01/31 5:09, Liu Bo wrote:
>>>>>>>> On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
>>>>>>>>> Thanks for your replying.
>>>>>>>>>
>>>>>>>>> I understand this bug is more complicated than I expected.
>>>>>>>>> I classify error cases under submit_extent_page() below
>>>>>>>>>
>>>>>>>>> A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
>>>>>>>>> I first assumed this case and sent the mail.
>>>>>>>>> When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
>>>>>>>>> Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
>>>>>>>>> In this case, bio_endio() is not called and the page's writeback bit
>>>>>>>>> remains.
>>>>>>>>> So, there is a need to call end_page_writeback() in the error handling.
>>>>>>>>>
>>>>>>>>> B: errors under submit_one_bio() of submit_extent_page()
>>>>>>>>> Errors that occur under submit_one_bio() handles at bio_endio(), and
>>>>>>>>> bio_endio() would call end_page_writeback().
>>>>>>>>>
>>>>>>>>> Therefore, as you mentioned in the last mail, simply adding
>>>>>>>>> end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
>>>>>>>>> conflict in the case of B.
>>>>>>>>> To avoid such conflict, one easy solution is adding PageWriteback() check
>>>>>>>>> too.
>>>>>>>>>
>>>>>>>>> How do you think of this solution?
>>>>>>>> (sorry for the late reply.)
>>>>>>>>
>>>>>>>> I think its caller, "__extent_writepage", has covered the above case
>>>>>>>> by setting page writeback again.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> -liubo
>>>>>>>>> Sincerely,
>>>>>>>>>
>>>>>>>>> On 2016/12/22 15:20, Liu Bo wrote:
>>>>>>>>>> On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
>>>>>>>>>>> This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
>>>>>>>>>>>
>>>>>>>>>>> When submit_extent_page() in __extent_writepage_io() fails,
>>>>>>>>>>> Btrfs misses clearing a writeback bit of the failed page.
>>>>>>>>>>> This causes the false under-writeback page.
>>>>>>>>>>> Then, another sync task hangs in filemap_fdatawait_range(),
>>>>>>>>>>> because it waits the false under-writeback page.
>>>>>>>>>>>
>>>>>>>>>>> CPU0                            CPU1
>>>>>>>>>>>
>>>>>>>>>>> __extent_writepage_io()
>>>>>>>>>>>       ret = submit_extent_page() // fail
>>>>>>>>>>>
>>>>>>>>>>>       if (ret)
>>>>>>>>>>>         SetPageError(page)
>>>>>>>>>>>         // miss clearing the writeback bit
>>>>>>>>>>>
>>>>>>>>>>>                                     sync()
>>>>>>>>>>>                                       ...
>>>>>>>>>>>                                       filemap_fdatawait_range()
>>>>>>>>>>>                                         wait_on_page_writeback(page);
>>>>>>>>>>>                                         // wait the false under-writeback page
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
>>>>>>>>>>> ---
>>>>>>>>>>>      fs/btrfs/extent_io.c | 4 +++-
>>>>>>>>>>>      1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>>>>>>>> index 1e67723..ef9793b 100644
>>>>>>>>>>> --- a/fs/btrfs/extent_io.c
>>>>>>>>>>> +++ b/fs/btrfs/extent_io.c
>>>>>>>>>>> @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
>>>>>>>>>>>      					 bdev->bio, max_nr,
>>>>>>>>>>>      					 end_bio_extent_writepage,
>>>>>>>>>>>      					 0, 0, 0, false);
>>>>>>>>>>> -		if (ret)
>>>>>>>>>>> +		if (ret) {
>>>>>>>>>>>      			SetPageError(page);
>>>>>>>>>>> +			end_page_writeback(page);
>>>>>>>>>>> +		}
>>>>>>>>>> OK...this could be complex as we don't know which part in
>>>>>>>>>> submit_extent_page gets the error, if the page has been added into bio
>>>>>>>>>> and bio_end would call end_page_writepage(page) as well, so whichever
>>>>>>>>>> comes later, the BUG() in end_page_writeback() would complain.
>>>>>>>>>>
>>>>>>>>>> Looks like commit 55e3bd2e0c2e1 also has the same problem although I
>>>>>>>>>> gave it my reviewed-by.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> -liubo
>>>>>>>>>>
>>>>>>>>>>>      		cur = cur + iosize;
>>>>>>>>>>>      		pg_offset += iosize;
>>>>>>>>>>> --
>>>>>>>>>>> 1.9.3
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>> --
>>>>>>>>> Keio University
>>>>>>>>> System Software Laboratory
>>>>>>>>> Takafumi Kubota
>>>>>>>>> takafumi.kubota1012@sslab.ics.keio.jp
>>>>>>>>>
>>>>>>> --
>>>>>>> Keio University
>>>>>>> System Software Laboratory
>>>>>>> Takafumi Kubota
>>>>>>> takafumi.kubota1012@sslab.ics.keio.jp
>>>>>>>
>>>>> --
>>>>> Keio University
>>>>> System Software Laboratory
>>>>> Takafumi Kubota
>>>>> takafumi.kubota1012@sslab.ics.keio.jp
>>>>>
>>>
>>> --
>>> Keio University
>>> System Software Laboratory
>>> Takafumi Kubota
>>> takafumi.kubota1012@sslab.ics.keio.jp
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1012@sslab.ics.keio.jp

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Liu Bo]

On Tue, Feb 07, 2017 at 08:09:53PM +0900, takafumi-sslab wrote:
> 
> On 2017/02/07 1:34, Liu Bo wrote:
> 
> > 
> > One thing to add, we still need to check whether page has writeback bit before
> > end_page_writeback.
> 
> Ok, I add PageWriteback check before end_page_writeback.
> 
> > > > > > > > > > > > Looks like commit 55e3bd2e0c2e1 also has the same problem although I
> > > > > > > > > > > > gave it my reviewed-by.
> 
> I also add PageWriteback check in write_one_eb.
> 
> Finally, the diff becomes like below.
> Is it Ok ?
> 
> ---
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 4ac383a..aa1908a 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3445,8 +3445,11 @@ static noinline_for_stack int
> __extent_writepage_io(struct inode *inode,
>  					 bdev, &epd->bio, max_nr,
>  					 end_bio_extent_writepage,
>  					 0, 0, 0, false);
> -		if (ret)
> +		if (ret) {
>  			SetPageError(page);
> +			if (PageWriteback(page))
> +				end_page_writeback(page);
> +		}
> 
>  		cur = cur + iosize;
>  		pg_offset += iosize;
> @@ -3767,7 +3770,8 @@ static noinline_for_stack int write_one_eb(struct
> extent_buffer *eb,
>  		epd->bio_flags = bio_flags;
>  		if (ret) {
>  			set_btree_ioerr(p);
> -			end_page_writeback(p);
> +			if (PageWriteback(p))
> +				end_page_writeback(p);
>  			if (atomic_sub_and_test(num_pages - i, &eb->io_pages))
>  				end_extent_buffer_writeback(eb);
>  			ret = -EIO;
> 
> ---
> 

Looks good, could you please make a comment for the if statement in your
commit log so that others could know why we put it?

Since you've got a reproducer, baking it into a fstests case is also
welcome.

Thanks,

-liubo

> 
> Sincerely,
> 
> -takafumi
> 
> 
> > 
> > Thanks,
> > 
> > -liubo
> > 
> > > 
> > > Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> > > 
> > > Thanks,
> > > 
> > > -liubo
> > > > 
> > > > Sincerely,
> > > > 
> > > > -takafumi
> > > > > 
> > > > > So I don't think the patch is necessary for now.
> > > > > 
> > > > > But as I said, the fact (nr == 0 or 1) would be changed if the
> > > > > subpagesize blocksize is supported.
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > -liubo
> > > > > 
> > > > > > Sincerely,
> > > > > > 
> > > > > > -takafumi
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > -liubo
> > > > > > > > Sincerely,
> > > > > > > > 
> > > > > > > > On 2017/01/31 5:09, Liu Bo wrote:
> > > > > > > > > On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
> > > > > > > > > > Thanks for your replying.
> > > > > > > > > > 
> > > > > > > > > > I understand this bug is more complicated than I expected.
> > > > > > > > > > I classify error cases under submit_extent_page() below
> > > > > > > > > > 
> > > > > > > > > > A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
> > > > > > > > > > I first assumed this case and sent the mail.
> > > > > > > > > > When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
> > > > > > > > > > Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
> > > > > > > > > > In this case, bio_endio() is not called and the page's writeback bit
> > > > > > > > > > remains.
> > > > > > > > > > So, there is a need to call end_page_writeback() in the error handling.
> > > > > > > > > > 
> > > > > > > > > > B: errors under submit_one_bio() of submit_extent_page()
> > > > > > > > > > Errors that occur under submit_one_bio() handles at bio_endio(), and
> > > > > > > > > > bio_endio() would call end_page_writeback().
> > > > > > > > > > 
> > > > > > > > > > Therefore, as you mentioned in the last mail, simply adding
> > > > > > > > > > end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
> > > > > > > > > > conflict in the case of B.
> > > > > > > > > > To avoid such conflict, one easy solution is adding PageWriteback() check
> > > > > > > > > > too.
> > > > > > > > > > 
> > > > > > > > > > How do you think of this solution?
> > > > > > > > > (sorry for the late reply.)
> > > > > > > > > 
> > > > > > > > > I think its caller, "__extent_writepage", has covered the above case
> > > > > > > > > by setting page writeback again.
> > > > > > > > > 
> > > > > > > > > Thanks,
> > > > > > > > > 
> > > > > > > > > -liubo
> > > > > > > > > > Sincerely,
> > > > > > > > > > 
> > > > > > > > > > On 2016/12/22 15:20, Liu Bo wrote:
> > > > > > > > > > > On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
> > > > > > > > > > > > This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
> > > > > > > > > > > > 
> > > > > > > > > > > > When submit_extent_page() in __extent_writepage_io() fails,
> > > > > > > > > > > > Btrfs misses clearing a writeback bit of the failed page.
> > > > > > > > > > > > This causes the false under-writeback page.
> > > > > > > > > > > > Then, another sync task hangs in filemap_fdatawait_range(),
> > > > > > > > > > > > because it waits the false under-writeback page.
> > > > > > > > > > > > 
> > > > > > > > > > > > CPU0                            CPU1
> > > > > > > > > > > > 
> > > > > > > > > > > > __extent_writepage_io()
> > > > > > > > > > > >       ret = submit_extent_page() // fail
> > > > > > > > > > > > 
> > > > > > > > > > > >       if (ret)
> > > > > > > > > > > >         SetPageError(page)
> > > > > > > > > > > >         // miss clearing the writeback bit
> > > > > > > > > > > > 
> > > > > > > > > > > >                                     sync()
> > > > > > > > > > > >                                       ...
> > > > > > > > > > > >                                       filemap_fdatawait_range()
> > > > > > > > > > > >                                         wait_on_page_writeback(page);
> > > > > > > > > > > >                                         // wait the false under-writeback page
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
> > > > > > > > > > > > ---
> > > > > > > > > > > >      fs/btrfs/extent_io.c | 4 +++-
> > > > > > > > > > > >      1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > > > > > > > > > > index 1e67723..ef9793b 100644
> > > > > > > > > > > > --- a/fs/btrfs/extent_io.c
> > > > > > > > > > > > +++ b/fs/btrfs/extent_io.c
> > > > > > > > > > > > @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
> > > > > > > > > > > >      					 bdev->bio, max_nr,
> > > > > > > > > > > >      					 end_bio_extent_writepage,
> > > > > > > > > > > >      					 0, 0, 0, false);
> > > > > > > > > > > > -		if (ret)
> > > > > > > > > > > > +		if (ret) {
> > > > > > > > > > > >      			SetPageError(page);
> > > > > > > > > > > > +			end_page_writeback(page);
> > > > > > > > > > > > +		}
> > > > > > > > > > > OK...this could be complex as we don't know which part in
> > > > > > > > > > > submit_extent_page gets the error, if the page has been added into bio
> > > > > > > > > > > and bio_end would call end_page_writepage(page) as well, so whichever
> > > > > > > > > > > comes later, the BUG() in end_page_writeback() would complain.
> > > > > > > > > > > 
> > > > > > > > > > > Looks like commit 55e3bd2e0c2e1 also has the same problem although I
> > > > > > > > > > > gave it my reviewed-by.
> > > > > > > > > > > 
> > > > > > > > > > > Thanks,
> > > > > > > > > > > 
> > > > > > > > > > > -liubo
> > > > > > > > > > > 
> > > > > > > > > > > >      		cur = cur + iosize;
> > > > > > > > > > > >      		pg_offset += iosize;
> > > > > > > > > > > > --
> > > > > > > > > > > > 1.9.3
> > > > > > > > > > > > 
> > > > > > > > > > > > --
> > > > > > > > > > > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > > > > > > > > > > > the body of a message to majordomo@vger.kernel.org
> > > > > > > > > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > > > > > > --
> > > > > > > > > > Keio University
> > > > > > > > > > System Software Laboratory
> > > > > > > > > > Takafumi Kubota
> > > > > > > > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > > > > > > > 
> > > > > > > > --
> > > > > > > > Keio University
> > > > > > > > System Software Laboratory
> > > > > > > > Takafumi Kubota
> > > > > > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > > > > > 
> > > > > > --
> > > > > > Keio University
> > > > > > System Software Laboratory
> > > > > > Takafumi Kubota
> > > > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > > > 
> > > > 
> > > > --
> > > > Keio University
> > > > System Software Laboratory
> > > > Takafumi Kubota
> > > > takafumi.kubota1012@sslab.ics.keio.jp
> > > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Keio University
> System Software Laboratory
> Takafumi Kubota
> takafumi.kubota1012@sslab.ics.keio.jp

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[David Sterba]

On Tue, Feb 07, 2017 at 12:14:51PM -0800, Liu Bo wrote:
> > +				end_page_writeback(page);
> > +		}
> > 
> >  		cur = cur + iosize;
> >  		pg_offset += iosize;
> > @@ -3767,7 +3770,8 @@ static noinline_for_stack int write_one_eb(struct
> > extent_buffer *eb,
> >  		epd->bio_flags = bio_flags;
> >  		if (ret) {
> >  			set_btree_ioerr(p);
> > -			end_page_writeback(p);
> > +			if (PageWriteback(p))
> > +				end_page_writeback(p);
> >  			if (atomic_sub_and_test(num_pages - i, &eb->io_pages))
> >  				end_extent_buffer_writeback(eb);
> >  			ret = -EIO;
> > 
> > ---
> > 
> 
> Looks good, could you please make a comment for the if statement in your
> commit log so that others could know why we put it?

Thank you both. Please resend v2 so I can add it to 4.11 queue.
> 
> Since you've got a reproducer, baking it into a fstests case is also
> welcome.

AFAICS the reproducer needs a kernel patch so the memory allocation
fails reliably, this is not suitable for fstests. We don't have an easy
way to inject allocation failures easily, but some reduced steps to
reprroduce could be added to the changelog.

Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[takafumi-sslab]

I am sorry for forggeting to write the reproducing steps.

I injects the ftrace's logging code and the fault to the Linux kerenl 
v4.10-rc7.
The diff is too long for pasting here.
So, I put the repository of the kernel here.
https://github.com/tk1012/linux-for-reproduce-btrfs-failure.git

And the steps to reproduce are:
	
	$ git clone https://github.com/tk1012/linux-for-reproduce-btrfs-failure.git
	
	$ make menuconfig # enable these configs
	- CONFIG_FAILSLAB
	- CONFIG_FAULT_INJECTION_DEBUG_FS
	- CONFIG_FTRACE
	
	$ make && make install

Then, I run the below reproducer.
And, you can see the ftrace log like:
	
	$ cat /sys/kernel/debug/tracing/trace > ~/ftrace_log
	
FYI, you can use the kernel config here.
https://github.com/tk1012/linux-for-reproduce-btrfs-failure/blob/master/minconfig

----------

#!/bin/bash

set -xe

echo 128000 > /sys/kernel/debug/tracing/buffer_size_kb

echo N > /sys/kernel/debug/failk9/task-filter
echo N > /sys/kernel/debug/failk9/ignore-gfp-wait
echo 100 > /sys/kernel/debug/failk9/probability
echo 1 > /sys/kernel/debug/failk9/interval

directory=/mnt/btrfs
mkdir -p ${directory}

truncate -s 100G btrfs.img
mkfs.btrfs btrfs.img
mount btrfs.img ${directory}

for i in `seq 1 1 100`; do
  echo 1 > /sys/kernel/debug/failk9/times
  bash -c "echo 1 > /proc/self/make-it-fail && dd if=/dev/zero 
of=${directory}/text_txt bs=1M count=10 2> /dev/null"
  sync # never return
done

umount ${directory}


----------



Sincerely,
-takafumi

On 2017/02/06 12:35, Liu Bo wrote:
> On Sat, Feb 04, 2017 at 09:42:17PM +0900, takafumi-sslab wrote:
>>
>>> (But it could be changed after subpagesize block patchset, and there is
>>> more work rather than just adding a end_page_writeback, e.g. writepage
>>> endio also needs to be updated).
>>
>> Ok... the discussion become complicated.
>> So, let me make this clear.
>>
>> you think
>> a) this is a bug;
>> we need to clear the writeback bit in the error handling if the bit remains.
>>
>> b) however, the way of fixing this bug has some concerns.　( and now we
>> discuss the best solution )
>>
>> Is my understanding correct?
>
> Sorry for making you confused even more, to clarify it, I don't think
> the bug could exist in the current btrfs because blocksize is equal to
> PAGE_SIZE so that @nr in __extent_writepage could only be 0 or 1.
>
> a) __extent_writepage has handled the case when nr == 0.
> b) when nr == 1, the page is marked with writeback bit and added into a
>    bio, thus we have bio_end to deal with page bits.
>
> So I don't think the patch is necessary for now.
>
> But as I said, the fact (nr == 0 or 1) would be changed if the
> subpagesize blocksize is supported.
>
> Thanks,
>
> -liubo
>
>>
>> Sincerely,
>>
>> -takafumi
>>>
>>> Thanks,
>>>
>>> -liubo
>>>> Sincerely,
>>>>
>>>> On 2017/01/31 5:09, Liu Bo wrote:
>>>>> On Fri, Jan 13, 2017 at 03:12:31PM +0900, takafumi-sslab wrote:
>>>>>> Thanks for your replying.
>>>>>>
>>>>>> I understand this bug is more complicated than I expected.
>>>>>> I classify error cases under submit_extent_page() below
>>>>>>
>>>>>> A: ENOMEM error at btrfs_bio_alloc() in submit_extent_page()
>>>>>> I first assumed this case and sent the mail.
>>>>>> When bio_ret is NULL, submit_extent_page() calls btrfs_bio_alloc().
>>>>>> Then, btrfs_bio_alloc() may fail and submit_extent_page() returns -ENOMEM.
>>>>>> In this case, bio_endio() is not called and the page's writeback bit
>>>>>> remains.
>>>>>> So, there is a need to call end_page_writeback() in the error handling.
>>>>>>
>>>>>> B: errors under submit_one_bio() of submit_extent_page()
>>>>>> Errors that occur under submit_one_bio() handles at bio_endio(), and
>>>>>> bio_endio() would call end_page_writeback().
>>>>>>
>>>>>> Therefore, as you mentioned in the last mail, simply adding
>>>>>> end_page_writeback() like my last email and commit 55e3bd2e0c2e1 can
>>>>>> conflict in the case of B.
>>>>>> To avoid such conflict, one easy solution is adding PageWriteback() check
>>>>>> too.
>>>>>>
>>>>>> How do you think of this solution?
>>>>> (sorry for the late reply.)
>>>>>
>>>>> I think its caller, "__extent_writepage", has covered the above case
>>>>> by setting page writeback again.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -liubo
>>>>>> Sincerely,
>>>>>>
>>>>>> On 2016/12/22 15:20, Liu Bo wrote:
>>>>>>> On Fri, Dec 16, 2016 at 03:41:50PM +0900, Takafumi Kubota wrote:
>>>>>>>> This is actually inspired by Filipe's patch(55e3bd2e0c2e1).
>>>>>>>>
>>>>>>>> When submit_extent_page() in __extent_writepage_io() fails,
>>>>>>>> Btrfs misses clearing a writeback bit of the failed page.
>>>>>>>> This causes the false under-writeback page.
>>>>>>>> Then, another sync task hangs in filemap_fdatawait_range(),
>>>>>>>> because it waits the false under-writeback page.
>>>>>>>>
>>>>>>>> CPU0                            CPU1
>>>>>>>>
>>>>>>>> __extent_writepage_io()
>>>>>>>>      ret = submit_extent_page() // fail
>>>>>>>>
>>>>>>>>      if (ret)
>>>>>>>>        SetPageError(page)
>>>>>>>>        // miss clearing the writeback bit
>>>>>>>>
>>>>>>>>                                    sync()
>>>>>>>>                                      ...
>>>>>>>>                                      filemap_fdatawait_range()
>>>>>>>>                                        wait_on_page_writeback(page);
>>>>>>>>                                        // wait the false under-writeback page
>>>>>>>>
>>>>>>>> Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
>>>>>>>> ---
>>>>>>>>     fs/btrfs/extent_io.c | 4 +++-
>>>>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>>>>> index 1e67723..ef9793b 100644
>>>>>>>> --- a/fs/btrfs/extent_io.c
>>>>>>>> +++ b/fs/btrfs/extent_io.c
>>>>>>>> @@ -3443,8 +3443,10 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
>>>>>>>>     					 bdev->bio, max_nr,
>>>>>>>>     					 end_bio_extent_writepage,
>>>>>>>>     					 0, 0, 0, false);
>>>>>>>> -		if (ret)
>>>>>>>> +		if (ret) {
>>>>>>>>     			SetPageError(page);
>>>>>>>> +			end_page_writeback(page);
>>>>>>>> +		}
>>>>>>> OK...this could be complex as we don't know which part in
>>>>>>> submit_extent_page gets the error, if the page has been added into bio
>>>>>>> and bio_end would call end_page_writepage(page) as well, so whichever
>>>>>>> comes later, the BUG() in end_page_writeback() would complain.
>>>>>>>
>>>>>>> Looks like commit 55e3bd2e0c2e1 also has the same problem although I
>>>>>>> gave it my reviewed-by.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> -liubo
>>>>>>>
>>>>>>>>     		cur = cur + iosize;
>>>>>>>>     		pg_offset += iosize;
>>>>>>>> --
>>>>>>>> 1.9.3
>>>>>>>>
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>> --
>>>>>> Keio University
>>>>>> System Software Laboratory
>>>>>> Takafumi Kubota
>>>>>> takafumi.kubota1012@sslab.ics.keio.jp
>>>>>>
>>>> --
>>>> Keio University
>>>> System Software Laboratory
>>>> Takafumi Kubota
>>>> takafumi.kubota1012@sslab.ics.keio.jp
>>>>
>>
>> --
>> Keio University
>> System Software Laboratory
>> Takafumi Kubota
>> takafumi.kubota1012@sslab.ics.keio.jp
>>

-- 
Keio University
System Software Laboratory
Takafumi Kubota
takafumi.kubota1012@sslab.ics.keio.jp

[PATCH v2] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Takafumi Kubota]

If btrfs_bio_alloc fails in submit_extent_page, submit_extent_page returns
without clearing the writeback bit of the failed page.

__extent_writepage_io, that is a caller of submit_extent_page,
does not clear the remaining writeback bit anywhere.
As a result, this will cause the hang at filemap_fdatawait_range,
because it waits the writeback bit to be cleared from the failed page.
So, we have to call end_page_writeback to clear the writeback bit.

For reproducing the hang, we inject a fault like

   if (should_failtest()) { // I define should_failtest()
        bio = NULL;
   }
   else {
        bio = btrfs_bio_alloc(...);
   }

in submit_extent_page.

We should also check whether page has the bit before end_page_writeback,
to avoid the conflict against the other end_page_writeback in bio_endio.
Thus, we add PageWriteback checks not only in __extent_writepage_io,
but also in write_one_eb too, because it misses the check.

Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Cc: David Sterba <dsterba@suse.cz>
---
 fs/btrfs/extent_io.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 4ac383a..aa1908a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3445,8 +3445,11 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
 					 bdev, &epd->bio, max_nr,
 					 end_bio_extent_writepage,
 					 0, 0, 0, false);
-		if (ret)
+		if (ret) {
 			SetPageError(page);
+			if (PageWriteback(page))
+				end_page_writeback(page);
+		}
 
 		cur = cur + iosize;
 		pg_offset += iosize;
@@ -3767,7 +3770,8 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
 		epd->bio_flags = bio_flags;
 		if (ret) {
 			set_btree_ioerr(p);
-			end_page_writeback(p);
+			if (PageWriteback(p))
+				end_page_writeback(p);
 			if (atomic_sub_and_test(num_pages - i, &eb->io_pages))
 				end_extent_buffer_writeback(eb);
 			ret = -EIO;
-- 
1.9.3

[PATCH v2] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[Takafumi Kubota]

If btrfs_bio_alloc fails in submit_extent_page, submit_extent_page returns
without clearing the writeback bit of the failed page.

__extent_writepage_io, that is a caller of submit_extent_page,
does not clear the remaining writeback bit anywhere.
As a result, this will cause the hang at filemap_fdatawait_range,
because it waits the writeback bit to be cleared from the failed page.
So, we have to call end_page_writeback to clear the writeback bit.

For reproducing the hang, we inject a fault like

   if (should_failtest()) { // I define should_failtest()
        bio = NULL;
   }
   else {
        bio = btrfs_bio_alloc(...);
   }

in submit_extent_page.

We should also check whether page has the bit before end_page_writeback,
to avoid the conflict against the other end_page_writeback in bio_endio.
Thus, we add PageWriteback checks not only in __extent_writepage_io,
but also in write_one_eb too, because it misses the check.

Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
Cc: David Sterba <dsterba@suse.cz>
---
 fs/btrfs/extent_io.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 4ac383a..aa1908a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3445,8 +3445,11 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
 					 bdev, &epd->bio, max_nr,
 					 end_bio_extent_writepage,
 					 0, 0, 0, false);
-		if (ret)
+		if (ret) {
 			SetPageError(page);
+			if (PageWriteback(page))
+				end_page_writeback(page);
+		}
 
 		cur = cur + iosize;
 		pg_offset += iosize;
@@ -3767,7 +3770,8 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb,
 		epd->bio_flags = bio_flags;
 		if (ret) {
 			set_btree_ioerr(p);
-			end_page_writeback(p);
+			if (PageWriteback(p))
+				end_page_writeback(p);
 			if (atomic_sub_and_test(num_pages - i, &eb->io_pages))
 				end_extent_buffer_writeback(eb);
 			ret = -EIO;
-- 
1.9.3

Re: [PATCH v2] Btrfs: add another missing end_page_writeback on submit_extent_page failure

[David Sterba]

On Thu, Feb 09, 2017 at 05:24:33PM +0900, Takafumi Kubota wrote:
> If btrfs_bio_alloc fails in submit_extent_page, submit_extent_page returns
> without clearing the writeback bit of the failed page.
> 
> __extent_writepage_io, that is a caller of submit_extent_page,
> does not clear the remaining writeback bit anywhere.
> As a result, this will cause the hang at filemap_fdatawait_range,
> because it waits the writeback bit to be cleared from the failed page.
> So, we have to call end_page_writeback to clear the writeback bit.
> 
> For reproducing the hang, we inject a fault like
> 
>    if (should_failtest()) { // I define should_failtest()
>         bio = NULL;
>    }
>    else {
>         bio = btrfs_bio_alloc(...);
>    }
> 
> in submit_extent_page.
> 
> We should also check whether page has the bit before end_page_writeback,
> to avoid the conflict against the other end_page_writeback in bio_endio.
> Thus, we add PageWriteback checks not only in __extent_writepage_io,
> but also in write_one_eb too, because it misses the check.
> 
> Signed-off-by: Takafumi Kubota <takafumi.kubota1012@sslab.ics.keio.ac.jp>
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> Cc: David Sterba <dsterba@suse.cz>

Added to 4.11 queue, thanks.