Re: [PATCH 1/2] btrfs: clear ordered flag on cleaning up ordered extents

[David Sterba <dsterba@suse.cz>]

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.aota@wdc.com>
> >>> Fixes: 524272607e88 ("btrfs: Handle delalloc error correctly to avoid ordered extent hang")
> >>> Cc: <stable@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.btrfs@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.