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

From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: 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: Wed, 8 Jan 2020 16:04:41 +0100	[thread overview]
Message-ID: <20200108150441.GG3929@twin.jikos.cz> (raw)
In-Reply-To: <9c2308bb-c3ae-d502-4b27-f8dbedc25d1a@gmx.com>

On Tue, Jan 07, 2020 at 10:13:43AM +0800, Qu Wenruo wrote:
> >> +	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.

Right, current statfs also does allocation via
btrfs_calc_avail_data_space, so it's the same as before.

> [...]
> >> +			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?

Better to return the error than wrong values in this case. As the
numbers are sort of a cache and the mount cycle to get them fixed is not
very user friendly, we need some other way. As this is a global state, a
bit in fs_info::flags can be set and full recalculation attempted at
some point until it succeeds. This would leave the counters stale for
some time but I think still better than if they're suddenly 0.