On Mon, Oct 07, 2024 at 11:49:51AM GMT, ross.philip...@oracle.com wrote:
> On 10/3/24 8:40 PM, Michael Chang via Grub-devel wrote:
> > On Thu, Oct 03, 2024 at 10:30:15AM GMT, ross.philip...@oracle.com wrote:
> > > On 10/3/24 12:23 AM, Michael Chang via Grub-devel wrote:
> > > > Previously, the buffer for LVM metadata parsing was set to twice the
> > > > size of the metadata area, which caused excessive memory use.
> > > >
> > > > This patch changes the allocation to read the actual raw metadata blocks
> > > > directly from the metadata area. Instead of using twice the entire
> > > > metadata area size, we now allocate just what’s needed for the raw
> > > > metadata.
> > > >
> > > > This change effectively reduces the buffer size while still working
> > > > correctly. In my test system on openSUSE, the buffer size for LVM
> > > > metadata dropped from 2,088,960 bytes to 1,119 bytes, which is a big
> > >
> > > Looking at the description here, I am not sure I understand these numbers.
> > > How did it go from around a 2M buffer to a roughly 1K one? If that is the
> > > case, they were allocating way more than 2x what was needed.
> >
> > We can determine the size of the LVM metadata area by running the command:
> >
> > pvs -o pv_name,pv_mda_size
> >
> > As a result, grub will allocate 2 * pv_mda_size, and since 1MiB is a
> > common default for pv_mda_size nowadays (it seems), this results in 2MiB
> > being allocated.
> >
> > However, most of the blocks in the metadata area are not useful to grub.
> > A lot of space gets wasted because it is occupied by a pre-allocated
> > circular buffer, which is used to track changes over time. To eliminate
> > this overhead, we can teach grub to use the raw metadata block to locate
> > the active metadata within the circular buffer. In the end, only 1,119
> > bytes are actually needed.
>
> Ahh ok that makes things a lot clearer (at least to me). Maybe some of this
> could go in the commit message.
I've sent v2 with added description we discussed here.
Thanks,
Michael
>
> Thanks
> Ross
>
> >
> > We've been using this patch since 2017, and I believe it is quite
> > stable. The circular buffer wrapping is also tested and covered. To give
> > some context on why we developed this patch, LVM used to allocate an
> > absurdly large mda_size on multipath disk setups [1], which caused boot
> > failures on PowerPC systems [2]. While this LVM issue may have been
> > fixed by now, I think it's still worth upstreaming this optimization.
> >
> > [1]
> > pvs -o pv_mda_size /dev/mapper/36...-part2
> > PMdaSize
> > 63.94m
> > [2]
> > Elapsed time since release of system processors: 190437 mins 52 secs
> > Welcome to GRUB!
> >
> > error: out of memory.
> > error: disk
> > `lvmid/efy0wc-Jr8x-yDG5-wEeI-k4ls-ql9Z-vKxAq0/UXMrG0-2C0A-1AaG-bzRv-
> > uSgG-lmK3-SAdAe5' not found.
> > Entering rescue mode...
> >
> > Thanks,
> > Michael
> >
> > >
> > > Thanks
> > > Ross
> > >
> > > > improvement.
> > > >
> > > > Signed-off-by: Michael Chang <mch...@suse.com>
> > > > ---
> > > > grub-core/disk/lvm.c | 79
> > > > ++++++++++++++++++++++++--------------------
> > > > 1 file changed, 43 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
> > > > index 0c32c95f9..ea260b47e 100644
> > > > --- a/grub-core/disk/lvm.c
> > > > +++ b/grub-core/disk/lvm.c
> > > > @@ -140,9 +140,11 @@ grub_lvm_detect (grub_disk_t disk,
> > > > grub_err_t err;
> > > > grub_uint64_t mda_offset, mda_size;
> > > > grub_size_t ptr;
> > > > + grub_uint64_t mda_raw_offset, mda_raw_size;
> > > > char buf[GRUB_LVM_LABEL_SIZE];
> > > > char vg_id[GRUB_LVM_ID_STRLEN+1];
> > > > char pv_id[GRUB_LVM_ID_STRLEN+1];
> > > > + char mdah_buf[sizeof (struct grub_lvm_mda_header) + sizeof (struct
> > > > grub_lvm_raw_locn)];
> > > > char *metadatabuf, *mda_end, *vgname;
> > > > const char *p, *q;
> > > > struct grub_lvm_label_header *lh = (struct grub_lvm_label_header
> > > > *) buf;
> > > > @@ -220,21 +222,15 @@ grub_lvm_detect (grub_disk_t disk,
> > > > dlocn++;
> > > > mda_offset = grub_le_to_cpu64 (dlocn->offset);
> > > > - mda_size = grub_le_to_cpu64 (dlocn->size);
> > > > /* It's possible to have multiple copies of metadata areas, we
> > > > just use the
> > > > first one. */
> > > > -
> > > > - /* Allocate buffer space for the circular worst-case scenario. */
> > > > - metadatabuf = grub_calloc (2, mda_size);
> > > > - if (! metadatabuf)
> > > > + err = grub_disk_read (disk, 0, mda_offset, sizeof (mdah_buf),
> > > > mdah_buf);
> > > > + if (err)
> > > > goto fail;
> > > > - err = grub_disk_read (disk, 0, mda_offset, mda_size, metadatabuf);
> > > > - if (err)
> > > > - goto fail2;
> > > > + mdah = (struct grub_lvm_mda_header *) mdah_buf;
> > > > - mdah = (struct grub_lvm_mda_header *) metadatabuf;
> > > > if ((grub_strncmp ((char *)mdah->magic, GRUB_LVM_FMTT_MAGIC,
> > > > sizeof (mdah->magic)))
> > > > || (grub_le_to_cpu32 (mdah->version) != GRUB_LVM_FMTT_VERSION))
> > > > @@ -244,42 +240,58 @@ grub_lvm_detect (grub_disk_t disk,
> > > > #ifdef GRUB_UTIL
> > > > grub_util_info ("unknown LVM metadata header");
> > > > #endif
> > > > - goto fail2;
> > > > + goto fail;
> > > > }
> > > > rlocn = mdah->raw_locns;
> > > > - if (grub_le_to_cpu64 (rlocn->offset) >= grub_le_to_cpu64 (mda_size))
> > > > +
> > > > + mda_size = grub_le_to_cpu64 (mdah->size);
> > > > + mda_raw_size = grub_le_to_cpu64 (rlocn->size);
> > > > + mda_raw_offset = grub_le_to_cpu64 (rlocn->offset);
> > > > +
> > > > + if (mda_raw_offset >= mda_size)
> > > > {
> > > > #ifdef GRUB_UTIL
> > > > grub_util_info ("metadata offset is beyond end of metadata
> > > > area");
> > > > #endif
> > > > - goto fail2;
> > > > + goto fail;
> > > > }
> > > > - if (grub_le_to_cpu64 (rlocn->offset) + grub_le_to_cpu64
> > > > (rlocn->size) >
> > > > - grub_le_to_cpu64 (mdah->size))
> > > > + metadatabuf = grub_malloc (mda_raw_size);
> > > > +
> > > > + if (! metadatabuf)
> > > > + goto fail;
> > > > +
> > > > + if (mda_raw_offset + mda_raw_size > mda_size)
> > > > {
> > > > - if (2 * mda_size < GRUB_LVM_MDA_HEADER_SIZE ||
> > > > - (grub_le_to_cpu64 (rlocn->offset) + grub_le_to_cpu64
> > > > (rlocn->size) -
> > > > - grub_le_to_cpu64 (mdah->size) > mda_size -
> > > > GRUB_LVM_MDA_HEADER_SIZE))
> > > > - {
> > > > -#ifdef GRUB_UTIL
> > > > - grub_util_info ("cannot copy metadata wrap in circular
> > > > buffer");
> > > > -#endif
> > > > - goto fail2;
> > > > - }
> > > > + err = grub_disk_read (disk, 0,
> > > > + mda_offset + mda_raw_offset,
> > > > + mda_size - mda_raw_offset,
> > > > + metadatabuf);
> > > > + if (err)
> > > > + goto fail2;
> > > > /* Metadata is circular. Copy the wrap in place. */
> > > > - grub_memcpy (metadatabuf + mda_size,
> > > > - metadatabuf + GRUB_LVM_MDA_HEADER_SIZE,
> > > > - grub_le_to_cpu64 (rlocn->offset) +
> > > > - grub_le_to_cpu64 (rlocn->size) -
> > > > - grub_le_to_cpu64 (mdah->size));
> > > > + err = grub_disk_read (disk, 0,
> > > > + mda_offset + GRUB_LVM_MDA_HEADER_SIZE,
> > > > + mda_raw_offset + mda_raw_size - mda_size,
> > > > + metadatabuf + mda_size - mda_raw_offset);
> > > > + if (err)
> > > > + goto fail2;
> > > > + }
> > > > + else
> > > > + {
> > > > + err = grub_disk_read (disk, 0,
> > > > + mda_offset + mda_raw_offset,
> > > > + mda_raw_size,
> > > > + metadatabuf);
> > > > + if (err)
> > > > + goto fail2;
> > > > }
> > > > - if (grub_add ((grub_size_t)metadatabuf,
> > > > - (grub_size_t)grub_le_to_cpu64 (rlocn->offset),
> > > > - &ptr))
> > > > + p = q = metadatabuf;
> > > > +
> > > > + if (grub_add ((grub_size_t)metadatabuf, (grub_size_t)mda_raw_size,
> > > > &ptr))
> > > > {
> > > > error_parsing_metadata:
> > > > #ifdef GRUB_UTIL
> > > > @@ -288,11 +300,6 @@ grub_lvm_detect (grub_disk_t disk,
> > > > goto fail2;
> > > > }
> > > > - p = q = (char *)ptr;
> > > > -
> > > > - if (grub_add (ptr, (grub_size_t) grub_le_to_cpu64 (rlocn->size),
> > > > &ptr))
> > > > - goto error_parsing_metadata;
> > > > -
> > > > mda_end = (char *)ptr;
> > > > while (*q != ' ' && q < mda_end)
> > >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://urldefense.com/v3/__https://lists.gnu.org/mailman/listinfo/grub-devel__;!!ACWV5N9M2RV99hQ!LaFHeJsQIeHfmuU6GDTX4C7R7Ee_p26-4Qmnjnkxgw8N5iHnkmtkuaCFgREsxdr-0O6-s4ENr1tp7tY4Z1oUtg$
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
