Re: [PATCH 13/23] btrfs: add the data transaction commit logic into may_commit_transaction

Josef Bacik
2020-07-07T15:01:44Z

On 7/7/20 10:39 AM, Nikolay Borisov wrote:
>
>
> On 30.06.20 г. 16:59 ч., Josef Bacik wrote:
>> Data space flushing currently unconditionally commits the transaction
>> twice in a row, and the last time it checks if there's enough pinned
>> extents to satisfy it's reservation before deciding to commit the
>> transaction for the 3rd and final time.
>>
>> Encode this logic into may_commit_transaction(). In the next patch we
>> will pass in U64_MAX for bytes_needed the first two times, and the final
>> time we will pass in the actual bytes we need so the normal logic will
>> apply.
>>
>> This patch exists soley to make the logical changes I will make to the
>> flushing state machine separate to make it easier to bisect any
>> performance related regressions.
>>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>> Tested-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>
> On a second pass through I'm now somewhat reluctant to merge this code.
> may_commit_transaction has grown more logic which pertain solely to
> metadata. As such I think we should separate that logic (i.e current
> may_commit_transaction) and any further adjustments we might want to
> make for data space info. For example all the delayed refs/trans
> reservation checks etc. make absolutely no sense for data space info.

Oooh I forgot another thing, the reason I did this is because I was trying to
maintain the behavior while converting, and then change the behavior in
subsequent patches. I'm doing this specifically because we may have performance
regressions, and I didn't want a bisect to show up at "Giant patch that changes
all the things". I want it to show up at the patch that actually changed the
behavior, so we have a better idea of where to look. So this intermediate step
is a bit wonky for sure, but the end result at the end of the series is less
wonky. But I'm still going to update that comment. Thanks,

Josef