Messages in this thread | | | Subject | Re: [PATCH] Btrfs: add another missing end_page_writeback on submit_extent_page failure | From | takafumi-sslab <> | Date | Sat, 4 Feb 2017 21:42:17 +0900 |
| |
> (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
| |