Re: [PATCH 15/23] btrfs: use ticketing for data space reservations

[Josef Bacik <josef@toxicpanda.com>]

On 7/7/20 10:46 AM, Nikolay Borisov wrote:
> 
> 
> On 30.06.20 г. 16:59 ч., Josef Bacik wrote:
>> Now that we have all the infrastructure in place, use the ticketing
>> infrastructure to make data allocations.  This still maintains the exact
>> same flushing behavior, but now we're using tickets to get our
>> reservations satisfied.
>>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>> Tested-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/space-info.c | 125 ++++++++++++++++++++++--------------------
>>   1 file changed, 67 insertions(+), 58 deletions(-)
>>
>> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
>> index 799ee6090693..ee4747917b81 100644
>> --- a/fs/btrfs/space-info.c
>> +++ b/fs/btrfs/space-info.c
>> @@ -1068,6 +1068,54 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
>>   	} while (flush_state < states_nr);
>>   }
>>   
>> +static void priority_reclaim_data_space(struct btrfs_fs_info *fs_info,
>> +					struct btrfs_space_info *space_info,
>> +					struct reserve_ticket *ticket,
>> +					const enum btrfs_flush_state *states,
>> +					int states_nr)
>> +{
>> +	int flush_state = 0;
>> +	int commit_cycles = 2;
>> +
>> +	while (!space_info->full) {
>> +		flush_space(fs_info, space_info, U64_MAX, ALLOC_CHUNK_FORCE);
>> +		spin_lock(&space_info->lock);
>> +		if (ticket->bytes == 0) {
>> +			spin_unlock(&space_info->lock);
>> +			return;
>> +		}
>> +		spin_unlock(&space_info->lock);
>> +	}
>> +again:
>> +	while (flush_state < states_nr) {
>> +		u64 flush_bytes = U64_MAX;
>> +
>> +		if (!commit_cycles) {
>> +			if (states[flush_state] == FLUSH_DELALLOC_WAIT) {
>> +				flush_state++;
>> +				continue;
>> +			}
>> +			if (states[flush_state] == COMMIT_TRANS)
>> +				flush_bytes = ticket->bytes;
>> +		}
>> +
>> +		flush_space(fs_info, space_info, flush_bytes,
>> +			    states[flush_state]);
>> +		spin_lock(&space_info->lock);
>> +		if (ticket->bytes == 0) {
>> +			spin_unlock(&space_info->lock);
>> +			return;
>> +		}
>> +		spin_unlock(&space_info->lock);
>> +		flush_state++;
>> +	}
>> +	if (commit_cycles) {
>> +		commit_cycles--;
>> +		flush_state = 0;
>> +		goto again;
>> +	}
>> +}
>> +
>>   static void wait_reserve_ticket(struct btrfs_fs_info *fs_info,
>>   				struct btrfs_space_info *space_info,
>>   				struct reserve_ticket *ticket)
>> @@ -1134,6 +1182,15 @@ static int handle_reserve_ticket(struct btrfs_fs_info *fs_info,
>>   						evict_flush_states,
>>   						ARRAY_SIZE(evict_flush_states));
>>   		break;
>> +	case BTRFS_RESERVE_FLUSH_DATA:
>> +		priority_reclaim_data_space(fs_info, space_info, ticket,
>> +					data_flush_states,
>> +					ARRAY_SIZE(data_flush_states));
>> +		break;
>> +	case BTRFS_RESERVE_FLUSH_FREE_SPACE_INODE:
>> +		priority_reclaim_data_space(fs_info, space_info, ticket,
>> +					    NULL, 0);
>> +		break;
>>   	default:
>>   		ASSERT(0);
>>   		break;
>> @@ -1341,78 +1398,30 @@ int btrfs_reserve_data_bytes(struct btrfs_fs_info *fs_info, u64 bytes,
>>   			     enum btrfs_reserve_flush_enum flush)
>>   {
>>   	struct btrfs_space_info *data_sinfo = fs_info->data_sinfo;
>> -	const enum btrfs_flush_state *states = NULL;
>>   	u64 used;
>> -	int states_nr = 0;
>> -	int commit_cycles = 2;
>>   	int ret = -ENOSPC;
>>   
>>   	ASSERT(!current->journal_info || flush != BTRFS_RESERVE_FLUSH_DATA);
>>   
>> -	if (flush == BTRFS_RESERVE_FLUSH_DATA) {
>> -		states = data_flush_states;
>> -		states_nr = ARRAY_SIZE(data_flush_states);
>> -	}
>> -
>>   	spin_lock(&data_sinfo->lock);
>> -again:
>>   	used = btrfs_space_info_used(data_sinfo, true);
>>   
>>   	if (used + bytes > data_sinfo->total_bytes) {
>> -		u64 prev_total_bytes = data_sinfo->total_bytes;
>> -		int flush_state = 0;
>> +		struct reserve_ticket ticket;
>>   
>> +		init_waitqueue_head(&ticket.wait);
>> +		ticket.bytes = bytes;
>> +		ticket.error = 0;
>> +		list_add_tail(&ticket.list, &data_sinfo->priority_tickets);
> 
> nit: Shouldn't adding the ticket also be recorded in
> spac_info->reclaim_size?
> I see later that you are removing this code and relying on the existing
> logic in __reserve_metadata_bytes( renamed to reserve_bytes) which
> correctly modifies reclaim_size, but this just means this particular
> patch is slightly broken.

Yup I'll fix this up, thanks.  Thanks,

Josef