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

Message

Qu Wenruo June 13, 2022, 7:06 a.m. UTC

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

Comments

David Sterba June 13, 2022, 6:20 p.m. UTC | #1

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.

Qu Wenruo June 13, 2022, 11:50 p.m. UTC | #2

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

David Sterba June 14, 2022, 1:56 p.m. UTC | #3

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.