Re: [PATCH] Use full btrfs bootloader area

[Michael Chang]

On Wed, Dec 01, 2021 at 05:16:56PM +0100, Daniel Kiper wrote:
> 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?

Maybe can put it this way "useful in tracing the problem of overflow by
logging it's access to adjacent sectors and don't overwrite it".

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

Thanks for review. I will add comment on those loops and agree to do all
the fixups you suggested in next version.

Thanks,
Michael

> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel