Re: [PATCH 2/2] btrfs: warn about dev extents that are inside the reserved range

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/6/14 03:05, Boris Burkov wrote:
> On Mon, Jun 13, 2022 at 03:06:35PM +0800, Qu Wenruo wrote:
>> Btrfs has reserved the first 1MiB for the primary super block (at 64KiB
>> offset) and legacy programs like older bootloaders.
>>
>> This behavior is only introduced since v4.1 btrfs-progs release,
>> although kernel can ensure we never touch the reserved range of super
>> blocks, it's better to inform the end users, and a balance will resolve
>> the problem.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/volumes.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 051d124679d1..b39f4030d2ba 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -7989,6 +7989,16 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>>   		goto out;
>>   	}
>>
>> +	/*
>> +	 * Very old mkfs.btrfs (before v4.1) will not respect the reserved
>> +	 * space. Although kernel can handle it without problem, better to
>> +	 * warn the users.
>> +	 */
>> +	if (physical_offset < BTRFS_DEFAULT_RESERVED)
>> +		btrfs_warn(fs_info,
>> +"devid %llu physical %llu len %llu is inside the reserved space, balance is needed to solve this problem.",
>
> If I saw this warning, I wouldn't know what balance to run, and it's
> not obvious what to search for online either (if it's even documented).
> I think a more explicit instruction like "btrfs balance start XXXX"
> would be helpful.

Firstly, the balance command needs extra filters, thus the command can
be pretty long, like:

# btrfs balance start -mdrange=0..1048576 -ddrange=0..1048576
-srange0..1048576 <mnt>

I'm not sure if this is a good idea to put all these into the already
long message.

>
> If it's something we're ok with in general, then maybe a URL for a wiki
> page that explains the issue and the workaround would be the most
> useful.

URL can be helpful but not always. Imagine a poor sysadmin in a noisy
server room, seeing a URL in dmesg, and has to type the full URL into
their phone, if the server has very limited network access.

In fact, this error message for now will be super rare already.

The main usage of this message is for the incoming feature, which will
allow btrfs to reserve extra space for its internal usage.

In that case, we will allow btrfstune to set the reservation (even it's
already used by some dev extent), and btrfstune would give a commandline
how to do the balance.

I guess I'd put all these preparation patches into the incoming on-disk
format change patchset to make it clear.

Thanks,
Qu

>
>> +			   devid, physical_offset, physical_len);
>> +
>>   	for (i = 0; i < map->num_stripes; i++) {
>>   		if (map->stripes[i].dev->devid == devid &&
>>   		    map->stripes[i].physical == physical_offset) {
>> --
>> 2.36.1
>>