Re: [PATCH v3 1/3] btrfs: Introduce per-profile available space facility

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: dsterba@suse.cz, Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org, Josef Bacik <josef@toxicpanda.com>
Subject: Re: [PATCH v3 1/3] btrfs: Introduce per-profile available space facility
Date: Tue, 7 Jan 2020 10:13:43 +0800	[thread overview]
Message-ID: <9c2308bb-c3ae-d502-4b27-f8dbedc25d1a@gmx.com> (raw)
In-Reply-To: <20200106143242.GG3929@twin.jikos.cz>

[-- Attachment #1.1: Type: text/plain, Size: 2526 bytes --]

On 2020/1/6 下午10:32, David Sterba wrote:
> On Mon, Jan 06, 2020 at 02:13:41PM +0800, Qu Wenruo wrote:
[...]
>> +static int calc_one_profile_avail(struct btrfs_fs_info *fs_info,
>> +				  enum btrfs_raid_types type)
>> +{
>> +	struct btrfs_device_info *devices_info = NULL;
>> +	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>> +	struct btrfs_device *device;
>> +	u64 allocated;
>> +	u64 result = 0;
>> +	int ret = 0;
>> +
>> +	ASSERT(type >= 0 && type < BTRFS_NR_RAID_TYPES);
>> +
>> +	/* Not enough devices, quick exit, just update the result */
>> +	if (fs_devices->rw_devices < btrfs_raid_array[type].devs_min)
>> +		goto out;
>> +
>> +	devices_info = kcalloc(fs_devices->rw_devices, sizeof(*devices_info),
>> +			       GFP_NOFS);
> 
> Calling kcalloc is another potential slowdown, for the statfs
> considerations.

That's also what we did in statfs() before, so it shouldn't cause extra
problem.
Furthermore, we didn't use calc_one_profile_avail() directly in statfs()
directly.

Although I get your point, and personally speaking the memory allocation
and extra in-memory device iteration should be pretty fast compared to
__btrfs_alloc_chunk().

Thus I don't think this memory allocation would cause extra trouble,
except the error handling mentioned below.

[...]
>> +			ret = calc_per_profile_avail(fs_info);
> 
> Adding new failure modes

Another solution I have tried is make calc_per_profile_avail() return
void, ignoring the ENOMEM error, and just set the affected profile to 0
available space.

But that method is just delaying ENOMEM, and would cause strange
pre-profile values until next successful update or mount cycle.

Any idea on which method is less worse?

[...]
>>  
>> --- a/fs/btrfs/volumes.h
>> +++ b/fs/btrfs/volumes.h
>> @@ -138,6 +138,13 @@ struct btrfs_device {
>>  	atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX];
>>  
>>  	struct extent_io_tree alloc_state;
>> +
>> +	/*
>> +	 * the "virtual" allocated space by virtual chunk allocator, which
>> +	 * is used to do accurate estimation on available space.
>> +	 * Doesn't affect real chunk allocator.
>> +	 */
>> +	u64 virtual_allocated;
> 
> I find it a bit confusing to use 'virtual', though I get what you mean.
> As this is per-device it accounts overall space, so the name should
> reflect somthing like that. I maybe have a more concrete suggestion once
> I read through the whole series.
> 
Looking forward that naming.

Thanks,
Qu

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]