On Thu, Jan 21, 2021 at 02:51:46PM +0800, Qu Wenruo wrote:

On 2021/1/21 下午2:32, Qu Wenruo wrote:

quoted

On 2021/1/20 上午5:41, Josef Bacik wrote:

quoted

On 1/16/21 2:15 AM, Qu Wenruo wrote:

quoted

When __process_pages_contig() get called for
extent_clear_unlock_delalloc(), if we hit the locked page, only Private2
bit is updated, but dirty/writeback/error bits are all skipped.

There are several call sites call extent_clear_unlock_delalloc() with
@locked_page and PAGE_CLEAR_DIRTY/PAGE_SET_WRITEBACK/PAGE_END_WRITEBACK

- cow_file_range()
- run_delalloc_nocow()
- cow_file_range_async()
   All for their error handling branches.

For those call sites, since we skip the locked page for
dirty/error/writeback bit update, the locked page will still have its
dirty bit remaining.

Thankfully, since all those call sites can only be hit with various
serious errors, it's pretty hard to hit and shouldn't affect regular
btrfs operations.

But still, we shouldn't leave the locked_page with its
dirty/error/writeback bits untouched.

Fix this by only skipping lock/unlock page operations for locked_page.

Signed-off-by: Qu Wenruo <redacted>

Except this is handled by the callers.  We clear_page_dirty_for_io() the
page before calling btrfs_run_delalloc_range(), so we don't need the
PAGE_CLEAR_DIRTY, it's already cleared.  The SetPageError() is handled
in the error path for locked_page, as is the
set_writeback/end_writeback.  Now I don't think this patch causes
problems specifically, but the changelog is at least wrong, and I'd
rather we'd skip the handling of the locked_page here and leave it in
the proper error handling.  If you need to do this for some other reason
that I haven't gotten to yet then you need to make that clear in the
changelog, because as of right now I don't see why this is needed.  
Thanks,

This is mostly to co-operate with a later patch on
__process_pages_contig(), where we need to make sure page locked by
__process_pages_contig() is only unlocked by __process_pages_contig() too.

The exception is after cow_file_inline(), we call
__process_pages_contig() on the locked page, making it to clear page
writeback and unlock it.

To be more clear, we call extent_clear_unlock_delalloc() with 
locked_page == NULL, to allow __process_pages_contig() to unlock the 
locked page (while the locked page isn't locked by 
__process_pages_contig()).

For subpage data, we need writers accounting for subpage, but that 
accounting only happens in __process_pages_contig(), thus we don't want 
pages without the accounting to be unlocked by __process_pages_contig().

I can do extra page unlock/clear_dirty/end_writeback just for that 
exception, but it would definitely needs more comments.

This is patch 1 and other depend on the changed behaviour so if it's
just updated changelog and comments, and Josef is ok with the result, I
can take it but otherwise this could delay the series once the rest is
reworked.