| Message ID | 51b17f7480724d528e709a920acd026acff447ea.1655103954.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: cleanup related to the 1MiB reserved space | expand |
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>
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>
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.
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
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.
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
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(-)