Re: [PATCH v2] btrfs: Fix error handling in btrfs_cleanup_ordered_extents

[Nikolay Borisov <nborisov@suse.com>]

On 29.10.18 г. 7:53 ч., Qu Wenruo wrote:
> [snip]
>>> The cause sounds valid, however would you please explain more about how
>>> such cleaning on unrelated delalloc range happens?
>>
>> So in my case the following happened - 2 block groups were created as
>> delalloc ranges in the - 0-1m and 1m-128m. Their respective pages were
>> dirtied, so when page 0 - 0-4k when into writepage_delalloc,
>> find_lock_delalloc_range would return the range 0-1m. So the call to
>> fill_delalloc instantiates OE 0-1m and writeback continues as expected.
>>
>> Now, when the 2nd page from range 0-1m is again set for writeback and
>> find_lock_delalloc_range is called with delalloc_start ==  4096 it will
>> actually return the range 1m-128m.
> 
> IMHO this looks strange and may need extra investigation.
> 
> Normally I would expect it returns the range 0~1M or 4K~1M.

It cannot return 4k-1m since the writeback for the first page has
already dealt with this range. Also take a look in writepage_delalloc
how find_lock_delalloc_range is called : for 'start' it's passed the
page offset, calculated in __extent_writepage. And when
find_delalloc_range is called it just searches for an extent which ends
after passed start value. In find_delalloc_range  first    tree_search
is called which returns the 1m-128m range, then we go in the while(1)
loop on the first itertaion found is 0 so *end is populated with 128m ,
found is set to 1 and *start is set to 1m.

On the second iteration the check  if (found && (state->start !=
cur_start || (state->state & EXTENT_BOUNDARY)))

is triggered since the next extent found will have EXTENT_BOUNDARY since
it will be the next block group from relocation. EXTENT_BOUNDARY will be
set from relocate_file_extent_cluster' main loop:

          if (nr < cluster->nr &&

                    page_start + offset == cluster->boundary[nr]) {

                        set_extent_bits(&BTRFS_I(inode)->io_tree,

                                        page_start, page_end,

                                        EXTENT_BOUNDARY);

                        nr++;

                }
> 
> But that doesn't affect the fix patch anyway.
> 
> Thanks,
> Qu
> 
>> Then fill_delalloc is called with
>> locked_page belonging to the range which was already instantiated and
>> the start/end arguments pointing to 1m-128m if an error occurred in
>> run_delalloc_range in this case then  btrfs_cleanup_ordered_extents will
>> be called which will clear Private2 bit for pages belonging to 1m-128m
>> range and the OE will be cleared of all but the first page (since the
>> code wrongly assumed locked_page always belongs to the range currently
>> being instantiated).
>>
> 
>