On Fri, Sep 08, 2017 at 05:10:04PM +0900, Naohiro Aota wrote:
> On 2017年09月08日 03:25, David Sterba wrote:
> > On Fri, Sep 01, 2017 at 06:59:49PM +0800, Qu Wenruo wrote:
> >> On 2017年09月01日 16:58, Naohiro Aota wrote:
> >>> commit 524272607e88 ("btrfs: Handle delalloc error correctly to avoid
> >>> ordered extent hang") introduced btrfs_cleanup_ordered_extents() to
> >>> cleanup
> >>> submitted ordered extents. However, it does not clear the ordered bit
> >>> (Private2) of coresponding pages. Thus, the following BUG occurs from
> >>> free_pages_check_bad() (on btrfs/125 with nospace_cache).
> >>>
> >>> BUG: Bad page state in process btrfs pfn:3fa787
> >>> page:ffffdf2acfe9e1c0 count:0 mapcount:0 mapping: (null)
> >>> index:0xd
> >>> flags: 0x8000000000002008(uptodate|private_2)
> >>> raw: 8000000000002008 0000000000000000 000000000000000d 00000000ffffffff
> >>> raw: ffffdf2acf5c1b20 ffffb443802238b0 0000000000000000 0000000000000000
> >>> page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
> >>> bad because of flags: 0x2000(private_2)
> >>>
> >>> This patch clear the flag as same as other places calling
> >>> btrfs_dec_test_ordered_pending() for every page in the specified range.
> >>>
> >>> Signed-off-by: Naohiro Aota <naohiro.a...@wdc.com>
> >>> Fixes: 524272607e88 ("btrfs: Handle delalloc error correctly to avoid
> >>> ordered extent hang")
> >>> Cc: <sta...@vger.kernel.org> # 4.12
> >>> ---
> >>> fs/btrfs/inode.c | 12 ++++++++++++
> >>> 1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> >>> index 24bcd5cd9cf2..ae4c0a1bef38 100644
> >>> --- a/fs/btrfs/inode.c
> >>> +++ b/fs/btrfs/inode.c
> >>> @@ -135,6 +135,18 @@ static inline void
> >>> btrfs_cleanup_ordered_extents(struct inode *inode,
> >>> const u64 offset,
> >>> const u64 bytes)
> >>> {
> >>> + unsigned long index = offset >> PAGE_SHIFT;
> >>> + unsigned long end_index = (offset + bytes - 1) >> PAGE_SHIFT;
> >>> + struct page *page;
> >>> +
> >>> + while (index <= end_index) {
> >>> + page = find_get_page(inode->i_mapping, index);
> >>> + index++;
> >>> + if (!page)
> >>> + continue;
> >>> + ClearPagePrivate2(page);
> >>> + put_page(page);
> >>> + }
> >>
> >> At first glance, explicitly clearing Private2 flag here seems a little
> >> strange to me.
> >> However btrfs_invalidatepage() is also doing the same thing, I think
> >> it's fine.
> >>
> >> Reviewed-by: Qu Wenruo <quwenruo.bt...@gmx.com>
> >>
> >> BTW, Private2 flag is set by extent_clear_unlock_delalloc() with
> >> page_ops |= PAGE_SET_PRIVATE2, but we're clearing the page flag without
> >> any encapsulation, it may be better to use similar function to clear
> >> Private2 flag.
> >
> > I agree, the Private2 flag is given another meaning in btrfs, ie. the
> > writeback status, so this would be better wrapped in helpers that
> > reflect what is the private2 flag used for. The helpers might be
> > trivial, but their name will be a better documentation than the random
> > comments that we can be found next to its use.
> >
>
> Thanks for the reviews. I'll send a new patch to add the wrapping
> function. (or should I squash the patch with this change?)
Please send a separate patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
