Re: [PATCH] Use full btrfs bootloader area

[Daniel Kiper]

On Tue, Nov 02, 2021 at 04:11:06PM +0800, Michael Chang via Grub-devel wrote:
> Up to now grub can only embed to the first 64 KiB before primary

s/grub/GRUB/

> superblock of btrfs, effectively limiting the size that could
> consequently pose restrictions to feature enablement like advancing zstd

s/advancing/advanced/?

> compression.
>
> This patch attempts to utilize full unused area reserved by btrfs for
> bootloader outlined in the document [1] where this paragraph quoted.
>
> "The first 1MiB on each device is unused with the exception of primary
> superblock that is on the offset 64KiB and spans 4KiB."
>
> Apart from that, adjacent sectors to superblock and first block group
> are not used for embedding in case of overflow and it's tracing.

What do you mean by "it's tracing" here?

> This patch has been tested to provide out of the box support to btrfs
> zstd compression with which grub has been installed to the partition.

s/grub/GRUB/

> [1] 
> https://btrfs.wiki.kernel.org/index.php/Manpage/btrfs(5)#BOOTLOADER_SUPPORT
>
> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>  grub-core/fs/btrfs.c | 80 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 67 insertions(+), 13 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 63203034d..dcd4635f1 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -2159,6 +2159,35 @@ grub_btrfs_label (grub_device_t device, char **label)
>  }
>
>  #ifdef GRUB_UTIL
> +
> +struct embed_region {
> +  unsigned int offset;

s/offset/start/

> +  unsigned int len;

s/len/secs/

> +};
> +
> +#define KB_TO_SECTOR(x) ((x) << 1)

I think this macro belongs to the include/grub/disk.h. Additionally, it
should be named, e.g., KiB_TO_GRUB_DISK_SECS and use GRUB_DISK_SECTOR_BITS.

> +/*
> + * 
> https://btrfs.wiki.kernel.org/index.php/Manpage/btrfs(5)#BOOTLOADER_SUPPORT
> + * The first 1MiB on each device is unused with the exception of primary
> + * superblock that is on the offset 64KiB and spans 4KiB.
> + */
> +
> +static const struct {
> +  struct embed_region available;
> +  struct embed_region used[6];
> +} area = {

s/area/btrfs_head/?

> +  .available = {0, KB_TO_SECTOR(1024)}, /* The first 1MiB */

Missing space between KB_TO_SECTOR and "(".

> +  .used = {
> +    {0, 1},                              /* boot.S */
> +    {KB_TO_SECTOR(64) - 1, 1},           /* overflow guard */

Ditto and below...

> +    {KB_TO_SECTOR(64), KB_TO_SECTOR(4)}, /* 4KiB superblock */
> +    {KB_TO_SECTOR(68), 1},               /* overflow guard */
> +    {KB_TO_SECTOR(1024) - 1, 1},         /* overflow guard */
> +    {0, 0}                               /* array terminator */
> +  }
> +};
> +
>  static grub_err_t
>  grub_btrfs_embed (grub_device_t device __attribute__ ((unused)),
>                 unsigned int *nsectors,
> @@ -2166,25 +2195,50 @@ grub_btrfs_embed (grub_device_t device __attribute__ 
> ((unused)),
>                 grub_embed_type_t embed_type,
>                 grub_disk_addr_t **sectors)
>  {
> -  unsigned i;
> +  unsigned int i, j;
> +  const struct embed_region *u;
> +  grub_disk_addr_t *map;
> +  unsigned int n = 0;
>
>    if (embed_type != GRUB_EMBED_PCBIOS)
>      return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
>                      "BtrFS currently supports only PC-BIOS embedding");
>
> -  if (64 * 2 - 1 < *nsectors)
> -    return grub_error (GRUB_ERR_OUT_OF_RANGE,
> -                    N_("your core.img is unusually large.  "
> -                       "It won't fit in the embedding area"));
> -
> -  *nsectors = 64 * 2 - 1;
> -  if (*nsectors > max_nsectors)
> -    *nsectors = max_nsectors;
> -  *sectors = grub_calloc (*nsectors, sizeof (**sectors));
> -  if (!*sectors)
> +  map = grub_calloc (area.available.len, sizeof (*map));
> +  if (!map)

if (map == NULL)

>      return grub_errno;
> -  for (i = 0; i < *nsectors; i++)
> -    (*sectors)[i] = i + 1;
> +
> +  for (u = area.used; u->len; ++u)
> +    {
> +      unsigned int end = u->offset + u->len;
> +
> +      if (end > area.available.len)
> +        end = area.available.len;
> +      for (i = u->offset; i < end; ++i)
> +        map[i] = 1;
> +    }
> +
> +  for (i = 0; i < area.available.len; ++i)
> +    if (map[i] == 0)
> +      n++;
> +
> +  if (n < *nsectors)
> +    {
> +      grub_free (map);
> +      return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +                      N_("your core.img is unusually large.  "
> +                         "It won't fit in the embedding area"));
> +    }
> +
> +  if (n > max_nsectors)
> +    n = max_nsectors;
> +
> +  for (i = 0, j = 0; i < area.available.len && j < n; ++i)
> +    if (map[i] == 0)
> +      map[j++] = area.available.offset + i;

I am not fully sure what is happening in this loop. In general I would
be happy if you add more comments before each step in the code above.

Daniel