On Thu, Jan 21, 2021 at 02:51:46PM +0800, Qu Wenruo wrote: > > > On 2021/1/21 下午2:32, Qu Wenruo wrote: > > > > > > On 2021/1/20 上午5:41, Josef Bacik wrote: > >> On 1/16/21 2:15 AM, Qu Wenruo wrote: > >>> 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 <w...@suse.com> > >> > >> 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.
