grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] lvm: allocate metadata buffer from raw contents


From: ross . philipson
Subject: Re: [PATCH] lvm: allocate metadata buffer from raw contents
Date: Mon, 7 Oct 2024 11:49:51 -0700
User-agent: Mozilla Thunderbird

On 10/3/24 8:40 PM, Michael Chang via Grub-devel wrote:
On Thu, Oct 03, 2024 at 10:30:15AM GMT, ross.philipson@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.

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 <mchang@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$




reply via email to

[Prev in Thread] Current Thread [Next in Thread]