[PATCH 0/2] btrfs: cleanup related to the 1MiB reserved space

[PATCH 0/2] btrfs: cleanup related to the 1MiB reserved space

[Qu Wenruo]

The 1MiB reserved space is only introduced in v4.1 btrfs-progs, and
kernel has the same reserved space around the same time.

But there are two small nitpicks:

- Kernel never has a unified MACRO for this
  We just use SZ_1M, with extra comments on this.

  This makes later write-intent bitmap harder to implement, as the
  incoming write-intent bitmap will enlarge the reserved space to
  2MiB, and use the extra 1MiB for write-intent bitmap.
  (of course with extra RO compat flags)

  This will be addressed in the first patch, with a new
  BTRFS_DEFAULT_RESERVED macro.

  Later write-intent bitmap code will use BTRFS_DEFAULT_RESERVED as a
  beacon to ensure btrfs never touches the enlarged reserved space.

- No warning on such very old fses.
  Although kernel can handle such old fses without any problem,
  it's still better to output a warning, with a solution (just balance).

  For now, we only need a warning, but for the incoming write-intent
  bitmap, any dev extents inside the extra reserved space will be
  rejected.


Qu Wenruo (2):
  btrfs: introduce BTRFS_DEFAULT_RESERVED macro
  btrfs: warn about dev extents that are inside the reserved range

 fs/btrfs/ctree.h       |  7 +++++++
 fs/btrfs/extent-tree.c |  6 +++---
 fs/btrfs/super.c       | 10 +++++-----
 fs/btrfs/volumes.c     | 17 +++++++++++------
 4 files changed, 26 insertions(+), 14 deletions(-)

-- 
2.36.1

[PATCH 1/2] btrfs: introduce BTRFS_DEFAULT_RESERVED macro

[Qu Wenruo]

Since btrfs-progs v4.1, mkfs.btrfs will reserve the first 1MiB for the
primary super block (at offset 64KiB) and other legacy bootloaders which
may want to store their data there.

Kernel is doing the same behavior around the same time.

However in kernel we just use SZ_1M to express the reserved space, normally
with extra comments when using above SZ_1M.

Here we introduce a new macro, BTRFS_DEFAULT_RESERVED to replace such
SZ_1M usage.

This will make later enlarged per-dev reservation easier to implement.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h       |  7 +++++++
 fs/btrfs/extent-tree.c |  6 +++---
 fs/btrfs/super.c       | 10 +++++-----
 fs/btrfs/volumes.c     |  7 +------
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index f7afdfd0bae7..62028e7d5799 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -229,6 +229,13 @@ struct btrfs_root_backup {
 #define BTRFS_SUPER_INFO_OFFSET			SZ_64K
 #define BTRFS_SUPER_INFO_SIZE			4096
 
+/*
+ * The default reserved space for each device.
+ * This range covers the primary superblock, and some other legacy programs like
+ * older bootloader may want to store their data there.
+ */
+#define BTRFS_DEFAULT_RESERVED			(SZ_1M)
+
 /*
  * the super block basically lists the main trees of the FS
  * it currently lacks any block count etc etc
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 8e9f0a99b292..e384dd483eaa 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5976,7 +5976,7 @@ int btrfs_error_unpin_extent_range(struct btrfs_fs_info *fs_info,
  */
 static int btrfs_trim_free_extents(struct btrfs_device *device, u64 *trimmed)
 {
-	u64 start = SZ_1M, len = 0, end = 0;
+	u64 start = BTRFS_DEFAULT_RESERVED, len = 0, end = 0;
 	int ret;
 
 	*trimmed = 0;
@@ -6020,8 +6020,8 @@ static int btrfs_trim_free_extents(struct btrfs_device *device, u64 *trimmed)
 			break;
 		}
 
-		/* Ensure we skip the reserved area in the first 1M */
-		start = max_t(u64, start, SZ_1M);
+		/* Ensure we skip the reserved area of each device. */
+		start = max_t(u64, start, BTRFS_DEFAULT_RESERVED);
 
 		/*
 		 * If find_first_clear_extent_bit find a range that spans the
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 719dda57dc7a..c2d051e887be 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2273,16 +2273,16 @@ static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info,
 
 		/*
 		 * In order to avoid overwriting the superblock on the drive,
-		 * btrfs starts at an offset of at least 1MB when doing chunk
-		 * allocation.
+		 * btrfs has some reserved space at the beginning of each
+		 * device.
 		 *
 		 * This ensures we have at least min_stripe_size free space
-		 * after excluding 1MB.
+		 * after excluding that reserved space.
 		 */
-		if (avail_space <= SZ_1M + min_stripe_size)
+		if (avail_space <= BTRFS_DEFAULT_RESERVED + min_stripe_size)
 			continue;
 
-		avail_space -= SZ_1M;
+		avail_space -= BTRFS_DEFAULT_RESERVED;
 
 		devices_info[i].dev = device;
 		devices_info[i].max_avail = avail_space;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 12a6150ee19d..051d124679d1 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1396,12 +1396,7 @@ static u64 dev_extent_search_start(struct btrfs_device *device, u64 start)
 {
 	switch (device->fs_devices->chunk_alloc_policy) {
 	case BTRFS_CHUNK_ALLOC_REGULAR:
-		/*
-		 * We don't want to overwrite the superblock on the drive nor
-		 * any area used by the boot loader (grub for example), so we
-		 * make sure to start at an offset of at least 1MB.
-		 */
-		return max_t(u64, start, SZ_1M);
+		return max_t(u64, start, BTRFS_DEFAULT_RESERVED);
 	case BTRFS_CHUNK_ALLOC_ZONED:
 		/*
 		 * We don't care about the starting region like regular
-- 
2.36.1

Re: [PATCH 1/2] btrfs: introduce BTRFS_DEFAULT_RESERVED macro

[Nikolay Borisov]

On 13.06.22 г. 10:06 ч., Qu Wenruo wrote:
> Since btrfs-progs v4.1, mkfs.btrfs will reserve the first 1MiB for the
> primary super block (at offset 64KiB) and other legacy bootloaders which
> may want to store their data there.
> 
> Kernel is doing the same behavior around the same time.
> 
> However in kernel we just use SZ_1M to express the reserved space, normally
> with extra comments when using above SZ_1M.
> 
> Here we introduce a new macro, BTRFS_DEFAULT_RESERVED to replace such
> SZ_1M usage.
> 
> This will make later enlarged per-dev reservation easier to implement.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/ctree.h       |  7 +++++++
>   fs/btrfs/extent-tree.c |  6 +++---
>   fs/btrfs/super.c       | 10 +++++-----
>   fs/btrfs/volumes.c     |  7 +------
>   4 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index f7afdfd0bae7..62028e7d5799 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -229,6 +229,13 @@ struct btrfs_root_backup {
>   #define BTRFS_SUPER_INFO_OFFSET			SZ_64K
>   #define BTRFS_SUPER_INFO_SIZE			4096
>   
> +/*
> + * The default reserved space for each device.
> + * This range covers the primary superblock, and some other legacy programs like
> + * older bootloader may want to store their data there.
> + */
> +#define BTRFS_DEFAULT_RESERVED			(SZ_1M)
> +

The name of this macros is too generic and uninformative. How about 
BTRFS_BOOT_RESERVED or simply BTRFS_RESERVED_SPACE, because 
BTRFS_DEFAULT_RESERVED implies  there is something else, apart from 
"default" and there won't be ...

<snip>

Re: [PATCH 1/2] btrfs: introduce BTRFS_DEFAULT_RESERVED macro

[Qu Wenruo]

On 2022/6/13 17:13, Nikolay Borisov wrote:
>
>
> On 13.06.22 г. 10:06 ч., Qu Wenruo wrote:
>> Since btrfs-progs v4.1, mkfs.btrfs will reserve the first 1MiB for the
>> primary super block (at offset 64KiB) and other legacy bootloaders which
>> may want to store their data there.
>>
>> Kernel is doing the same behavior around the same time.
>>
>> However in kernel we just use SZ_1M to express the reserved space,
>> normally
>> with extra comments when using above SZ_1M.
>>
>> Here we introduce a new macro, BTRFS_DEFAULT_RESERVED to replace such
>> SZ_1M usage.
>>
>> This will make later enlarged per-dev reservation easier to implement.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/ctree.h       |  7 +++++++
>>   fs/btrfs/extent-tree.c |  6 +++---
>>   fs/btrfs/super.c       | 10 +++++-----
>>   fs/btrfs/volumes.c     |  7 +------
>>   4 files changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index f7afdfd0bae7..62028e7d5799 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -229,6 +229,13 @@ struct btrfs_root_backup {
>>   #define BTRFS_SUPER_INFO_OFFSET            SZ_64K
>>   #define BTRFS_SUPER_INFO_SIZE            4096
>> +/*
>> + * The default reserved space for each device.
>> + * This range covers the primary superblock, and some other legacy
>> programs like
>> + * older bootloader may want to store their data there.
>> + */
>> +#define BTRFS_DEFAULT_RESERVED            (SZ_1M)
>> +
>
> The name of this macros is too generic and uninformative. How about
> BTRFS_BOOT_RESERVED or simply BTRFS_RESERVED_SPACE, because
> BTRFS_DEFAULT_RESERVED implies  there is something else, apart from
> "default" and there won't be ...

BTRFS_RESERVED_SPACE sounds good. The "BOOT_RESERVED" part will
definitely lose its meaning in the long run, since now there is no
modern bootloader really doing that.
(Either go full ESP, like systemd-boot, or extra reserved partition like
GRUB2, or use unpartitioned space after MBR like the legacy GRUB1)

But "DEFAULT" is here because later we will enlarge the reserved space
(for write-intent map, and will introduce a new super block member to
indicate exact how many bytes are reserved), and I don't want to add
"DEFAULT" when we introduce that new reserved behavior.

Any clue on the naming part?

Thanks,
Qu

>
> <snip>

Re: [PATCH 1/2] btrfs: introduce BTRFS_DEFAULT_RESERVED macro

[David Sterba]

On Mon, Jun 13, 2022 at 05:36:19PM +0800, Qu Wenruo wrote:
> >> index f7afdfd0bae7..62028e7d5799 100644
> >> --- a/fs/btrfs/ctree.h
> >> +++ b/fs/btrfs/ctree.h
> >> @@ -229,6 +229,13 @@ struct btrfs_root_backup {
> >>   #define BTRFS_SUPER_INFO_OFFSET            SZ_64K
> >>   #define BTRFS_SUPER_INFO_SIZE            4096
> >> +/*
> >> + * The default reserved space for each device.
> >> + * This range covers the primary superblock, and some other legacy
> >> programs like
> >> + * older bootloader may want to store their data there.
> >> + */
> >> +#define BTRFS_DEFAULT_RESERVED            (SZ_1M)
> >> +
> >
> > The name of this macros is too generic and uninformative. How about
> > BTRFS_BOOT_RESERVED or simply BTRFS_RESERVED_SPACE, because
> > BTRFS_DEFAULT_RESERVED implies  there is something else, apart from
> > "default" and there won't be ...

I've renamed it to BTRFS_DEVICE_RANGE_RESERVED

> BTRFS_RESERVED_SPACE sounds good. The "BOOT_RESERVED" part will
> definitely lose its meaning in the long run, since now there is no
> modern bootloader really doing that.
> (Either go full ESP, like systemd-boot, or extra reserved partition like
> GRUB2, or use unpartitioned space after MBR like the legacy GRUB1)

Yeah the bootloader is there as a known example, but what will happen
with bootloaders in the long run we can't predict, so the reserved space
will stay.

> But "DEFAULT" is here because later we will enlarge the reserved space
> (for write-intent map, and will introduce a new super block member to
> indicate exact how many bytes are reserved), and I don't want to add
> "DEFAULT" when we introduce that new reserved behavior.

I've dropped the default because right now it's not configurable and if
it eventually be then it can be renamed, but the write intent bitmap is
a work in progress so existing patches should not mention it. As said
before, naming the constant with a single point explaining the meaning
is a good cleanup on itself.

Re: [PATCH 1/2] btrfs: introduce BTRFS_DEFAULT_RESERVED macro

[Qu Wenruo]

On 2022/6/16 23:20, David Sterba wrote:
> On Mon, Jun 13, 2022 at 05:36:19PM +0800, Qu Wenruo wrote:
>>>> index f7afdfd0bae7..62028e7d5799 100644
>>>> --- a/fs/btrfs/ctree.h
>>>> +++ b/fs/btrfs/ctree.h
>>>> @@ -229,6 +229,13 @@ struct btrfs_root_backup {
>>>>    #define BTRFS_SUPER_INFO_OFFSET            SZ_64K
>>>>    #define BTRFS_SUPER_INFO_SIZE            4096
>>>> +/*
>>>> + * The default reserved space for each device.
>>>> + * This range covers the primary superblock, and some other legacy
>>>> programs like
>>>> + * older bootloader may want to store their data there.
>>>> + */
>>>> +#define BTRFS_DEFAULT_RESERVED            (SZ_1M)
>>>> +
>>>
>>> The name of this macros is too generic and uninformative. How about
>>> BTRFS_BOOT_RESERVED or simply BTRFS_RESERVED_SPACE, because
>>> BTRFS_DEFAULT_RESERVED implies  there is something else, apart from
>>> "default" and there won't be ...
>
> I've renamed it to BTRFS_DEVICE_RANGE_RESERVED
>
>> BTRFS_RESERVED_SPACE sounds good. The "BOOT_RESERVED" part will
>> definitely lose its meaning in the long run, since now there is no
>> modern bootloader really doing that.
>> (Either go full ESP, like systemd-boot, or extra reserved partition like
>> GRUB2, or use unpartitioned space after MBR like the legacy GRUB1)
>
> Yeah the bootloader is there as a known example, but what will happen
> with bootloaders in the long run we can't predict, so the reserved space
> will stay.
>
>> But "DEFAULT" is here because later we will enlarge the reserved space
>> (for write-intent map, and will introduce a new super block member to
>> indicate exact how many bytes are reserved), and I don't want to add
>> "DEFAULT" when we introduce that new reserved behavior.
>
> I've dropped the default because right now it's not configurable and if
> it eventually be then it can be renamed, but the write intent bitmap is
> a work in progress so existing patches should not mention it. As said
> before, naming the constant with a single point explaining the meaning
> is a good cleanup on itself.

OK, I would rebase the branch and using thew new naming (and rename it
if needed) after finishing the write-intent bitmaps feature (at least
for the bitmaps update part)

Thanks,
Qu

Re: [PATCH 1/2] btrfs: introduce BTRFS_DEFAULT_RESERVED macro

[David Sterba]

On Fri, Jun 17, 2022 at 09:47:49AM +0800, Qu Wenruo wrote:
> > I've dropped the default because right now it's not configurable and if
> > it eventually be then it can be renamed, but the write intent bitmap is
> > a work in progress so existing patches should not mention it. As said
> > before, naming the constant with a single point explaining the meaning
> > is a good cleanup on itself.
> 
> OK, I would rebase the branch and using thew new naming (and rename it
> if needed) after finishing the write-intent bitmaps feature (at least
> for the bitmaps update part)

Patches are now in misc-next.

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

[Qu Wenruo]

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.",
+			   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

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

[Boris Burkov]

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.

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.

> +			   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
> 

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

[Qu Wenruo]

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
>>

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

[Boris Burkov]

On Tue, Jun 14, 2022 at 03:48:06PM +0800, Qu Wenruo wrote:
> 
> 
> 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.

I don't see how the poor sysadmin would be any better off with "you need
to do a balance" vs "you need to do a balance: <URL>" or "you need to do
a balance using mdrange and ddrange to move the affected extents" etc..

My high level point is that you clearly have something in mind that the
person needs to do in the unlikely event they hit this, but I have no
idea how they are supposed to figure it out. Send a mail to our mailing
list and hope you notice it?

> 
> 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
> > > 

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

[Qu Wenruo]

On 2022/6/14 23:30, Boris Burkov wrote:
> On Tue, Jun 14, 2022 at 03:48:06PM +0800, Qu Wenruo wrote:
>>
>>
>> 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.
>
> I don't see how the poor sysadmin would be any better off with "you need
> to do a balance" vs "you need to do a balance: <URL>" or "you need to do
> a balance using mdrange and ddrange to move the affected extents" etc..
>
> My high level point is that you clearly have something in mind that the
> person needs to do in the unlikely event they hit this, but I have no
> idea how they are supposed to figure it out. Send a mail to our mailing
> list and hope you notice it?

I guess you miss the point here.

First, this is really rare case, it need older mkfs.btrfs and never
balanced the fs.

Second, the warning message itself is fine, kernel is 100% fine handling
it. The warning message can be ignored as long as there is no usage of
legacy bootloader.

>
>>
>> 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.

In fact, that would be where the detailed balance command line to be shown.

Btrfs check and btrfstune would output the detailed command line to do that.

Thanks,
Qu
>>
>> 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
>>>>

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

[David Sterba]

On Wed, Jun 15, 2022 at 06:12:29AM +0800, Qu Wenruo wrote:
> On 2022/6/14 23:30, Boris Burkov wrote:
> > On Tue, Jun 14, 2022 at 03:48:06PM +0800, Qu Wenruo wrote:
> >> 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.
> >
> > I don't see how the poor sysadmin would be any better off with "you need
> > to do a balance" vs "you need to do a balance: <URL>" or "you need to do
> > a balance using mdrange and ddrange to move the affected extents" etc..
> >
> > My high level point is that you clearly have something in mind that the
> > person needs to do in the unlikely event they hit this, but I have no
> > idea how they are supposed to figure it out. Send a mail to our mailing
> > list and hope you notice it?
> 
> I guess you miss the point here.
> 
> First, this is really rare case, it need older mkfs.btrfs and never
> balanced the fs.
> 
> Second, the warning message itself is fine, kernel is 100% fine handling
> it. The warning message can be ignored as long as there is no usage of
> legacy bootloader.
> 
> >
> >>
> >> 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.
> 
> In fact, that would be where the detailed balance command line to be shown.
> 
> Btrfs check and btrfstune would output the detailed command line to do that.

I don't think this is a good place either. There's a WIP page
file:///home/ds/x/btrfs-progs/Documentation/_build/html/trouble-index.html

that should be the starting point to explain errors or error messages in
greater detail than what can be fit to one line. There is/was a similar
page on wiki but not was used or lacked details.

Re: [PATCH 0/2] btrfs: cleanup related to the 1MiB reserved space

[David Sterba]

On Mon, Jun 13, 2022 at 03:06:33PM +0800, Qu Wenruo wrote:
> The 1MiB reserved space is only introduced in v4.1 btrfs-progs, and
> kernel has the same reserved space around the same time.

Kernel had this since the beginnning, or do you have pointer for the
commits? I know that progs wrongly allocated some of the tree roots
under 1M and this got fixed, but otherwise kernel never used the 1st
megabyte for allocations.

> But there are two small nitpicks:
> 
> - Kernel never has a unified MACRO for this
>   We just use SZ_1M, with extra comments on this.
> 
>   This makes later write-intent bitmap harder to implement, as the
>   incoming write-intent bitmap will enlarge the reserved space to
>   2MiB, and use the extra 1MiB for write-intent bitmap.
>   (of course with extra RO compat flags)
> 
>   This will be addressed in the first patch, with a new
>   BTRFS_DEFAULT_RESERVED macro.

Cleaning up the raw 1M value and the comments makes sense, but I'm not
sure about making it dynamic. We used to have mount option and mkfs
parameter alloc_offset and it got removed.

>   Later write-intent bitmap code will use BTRFS_DEFAULT_RESERVED as a
>   beacon to ensure btrfs never touches the enlarged reserved space.

Ok, I'll wait with further comments until I see the patches.

Re: [PATCH 0/2] btrfs: cleanup related to the 1MiB reserved space

[Qu Wenruo]

On 2022/6/14 02:20, David Sterba wrote:
> On Mon, Jun 13, 2022 at 03:06:33PM +0800, Qu Wenruo wrote:
>> The 1MiB reserved space is only introduced in v4.1 btrfs-progs, and
>> kernel has the same reserved space around the same time.
>
> Kernel had this since the beginnning, or do you have pointer for the
> commits? I know that progs wrongly allocated some of the tree roots
> under 1M and this got fixed, but otherwise kernel never used the 1st
> megabyte for allocations.

My bad, kernel part is indeed doing the reservation from the very beginning.

>
>> But there are two small nitpicks:
>>
>> - Kernel never has a unified MACRO for this
>>    We just use SZ_1M, with extra comments on this.
>>
>>    This makes later write-intent bitmap harder to implement, as the
>>    incoming write-intent bitmap will enlarge the reserved space to
>>    2MiB, and use the extra 1MiB for write-intent bitmap.
>>    (of course with extra RO compat flags)
>>
>>    This will be addressed in the first patch, with a new
>>    BTRFS_DEFAULT_RESERVED macro.
>
> Cleaning up the raw 1M value and the comments makes sense, but I'm not
> sure about making it dynamic. We used to have mount option and mkfs
> parameter alloc_offset and it got removed.

The dynamic part will be done at mkfs time, and will introduce new RO
compat flags for it.

So no mount option to change that.

>
>>    Later write-intent bitmap code will use BTRFS_DEFAULT_RESERVED as a
>>    beacon to ensure btrfs never touches the enlarged reserved space.
>
> Ok, I'll wait with further comments until I see the patches.

So do I need to include those two patches in the incoming write-intent
series?

Thanks,
Qu

Re: [PATCH 0/2] btrfs: cleanup related to the 1MiB reserved space

[David Sterba]

On Tue, Jun 14, 2022 at 07:50:25AM +0800, Qu Wenruo wrote:
> My bad, kernel part is indeed doing the reservation from the very beginning.
> 
> >
> >> But there are two small nitpicks:
> >>
> >> - Kernel never has a unified MACRO for this
> >>    We just use SZ_1M, with extra comments on this.
> >>
> >>    This makes later write-intent bitmap harder to implement, as the
> >>    incoming write-intent bitmap will enlarge the reserved space to
> >>    2MiB, and use the extra 1MiB for write-intent bitmap.
> >>    (of course with extra RO compat flags)
> >>
> >>    This will be addressed in the first patch, with a new
> >>    BTRFS_DEFAULT_RESERVED macro.
> >
> > Cleaning up the raw 1M value and the comments makes sense, but I'm not
> > sure about making it dynamic. We used to have mount option and mkfs
> > parameter alloc_offset and it got removed.
> 
> The dynamic part will be done at mkfs time, and will introduce new RO
> compat flags for it.
> 
> So no mount option to change that.
> 
> >
> >>    Later write-intent bitmap code will use BTRFS_DEFAULT_RESERVED as a
> >>    beacon to ensure btrfs never touches the enlarged reserved space.
> >
> > Ok, I'll wait with further comments until I see the patches.
> 
> So do I need to include those two patches in the incoming write-intent
> series?

For the preview series yes, so it can be applied and tested. The
preparatory work or cleanups do not need to be perfect or finalized for
that reason (though we might want to take them independently).

You've sent several series tackling the raid56 problems but we don't
have the final solution yet, so you don't need to spend too much time on
the less important bits.

The ro-compat bit can be a new one or we can reuse the raid56 incompat
bit that's currently not used for anything, but I can't say which way to
go until I see the patches.