Re: [PATCH 11/23] btrfs: check tickets after waiting on ordered extents

Josef Bacik
2020-02-03T15:57:45Z

On 2/3/20 8:10 AM, Nikolay Borisov wrote:
>
>
> On 1.02.20 г. 0:36 ч., Josef Bacik wrote:
>> Right now if the space is free'd up after the ordered extents complete
>> (which is likely since the reservations are held until they complete),
>> we would do extra delalloc flushing before we'd notice that we didn't
>> have any more tickets. Fix this by moving the tickets check after our
>> wait_ordered_extents check.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>
> This patch makes sense only for metadata. Is this your intention -
> tweaking the metadata change behavior? Correct me if I'm wrong but
>
> btrfs_start_delalloc_roots from previous patch will essentially call
> btrfs_run_delalloc_range for the roots which will create ordered extents in:
> btrfs_run_delalloc_range
> cow_file_range
> add_ordered_extents
>
> Following page writeout, from the endio routines we'll eventually do:
>
> finish_ordered_fn
> btrfs_finish_ordered_io
> insert_reserved_file_extent
> btrfs_alloc_reserved_file_extent
> create delayed ref <---- after the delayed extent is run this will
> free some data space. But this happens in transaction commit context and
> not when runnig ordered extents
> btrfs_remove_ordered_extent
> btrfs_delalloc_release_metadata <- this is only for metadata
> btrfs_inode_rsv_release
> __btrfs_block_rsv_release <-- frees metadata but not data?
>
>
> I'm looking those patches thinking every change should be pertinent to
> data space but apparently it's not. If so I think it will be best if you
> update the cover letter for V2 to mention which patches can go in
> independently or give more context why this particular patch is
> pertinent to data flush.
>

Specifically what I'm addressing here for data is this behavior

btrfs_start_delalloc_roots()
->check space_info->tickets, it's not empty
btrfs_finish_ordered_io()
-> we drop a previously uncompressed extent (ie larger one) for a newly
compressed extent, now space_info->tickets _is_ empty
loop again despite having no tickets to flush for.

Does this scenario happen all the time? Nope, but I noticed it was happening
sometimes with the data intensive enopsc tests xfstests so I threw it in there
because it made a tiny difference, plus it's actually correct.

It definitely affects the metadata case more for sure, but I only noticed it
when I forgot I had compression enabled for one of my xfstests runs. Thanks,

Joosef