On Tue, Nov 05, 2019 at 09:33:00AM +0000, Michael Chang wrote:
> The lvm cache logical volume is the logical volume consisting of the
> original and the cache pool logical volume. The original is usually on a
> larger and slower storage device while the cache pool is on a smaller
> and faster one. The performance of the original volume can be improved
> by storing the frequently used data on the cache pool to utilize the
> greater performance of faster device.
>
> The default cache mode "writethrough" ensures that any data written will
> be stored both in the cache and on the origin LV, therefore grub can go
> straight to read the original lv as no data loss is guarenteed.
>
> The second cache mode is "writeback", which delays writing from the
> cache pool back to the origin LV to have increased performance. The
> drawback is potential data loss if losing the associated cache device.
>
> During the boot time grub reads the LVM offline i.e. LVM volumes are not
> activated and mounted, IMHO it should be fine to read directly from
> original lv since all cached data should have been flushed back in the
> process of taking it offline.
s/, IMHO/. Hence,/?
> Signed-off-by: Michael Chang <mch...@suse.com>
> ---
>
> Changes since v2:
> * Move struct cache_lv definition to the beginning of file
> * Add handling when grub_(zalloc|malloc|strdup) etc failed
>
> grub-core/disk/lvm.c | 149
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 149 insertions(+)
>
> diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
> index 7b265c780..e296b49e3 100644
> --- a/grub-core/disk/lvm.c
> +++ b/grub-core/disk/lvm.c
> @@ -33,6 +33,14 @@
>
> GRUB_MOD_LICENSE ("GPLv3+");
>
> +struct cache_lv
> +{
> + struct grub_diskfilter_lv *lv;
> + char *cache_pool;
> + char *origin;
> + struct cache_lv *next;
> +};
> +
>
> /* Go the string STR and return the number after STR. *P will point
> at the number. In case STR is not found, *P will be NULL and the
> @@ -242,6 +250,8 @@ grub_lvm_detect (grub_disk_t disk,
>
> if (! vg)
> {
> + struct cache_lv *cache_lvs = NULL;
> +
> /* First time we see this volume group. We've to create the
> whole volume group structure. */
> vg = grub_malloc (sizeof (*vg));
> @@ -671,6 +681,79 @@ grub_lvm_detect (grub_disk_t disk,
> seg->nodes[seg->node_count - 1].name = tmp;
> }
> }
> + else if (grub_memcmp (p, "cache\"",
> + sizeof ("cache\"") - 1) == 0)
> + {
> + struct cache_lv *cache;
> +
> + char *p2, *p3;
> + grub_ssize_t sz;
> +
> + if (!(cache = grub_zalloc (sizeof (*cache))))
> + goto cache_lv_fail;
cache = grub_zalloc (sizeof (*cache));
if (!cache)
goto cache_lv_fail;
...and below please...
> + if (!(cache->lv = grub_zalloc (sizeof (*cache->lv))))
> + goto cache_lv_fail;
> + grub_memcpy (cache->lv, lv, sizeof (*cache->lv));
> +
> + if (lv->fullname)
> + if (!(cache->lv->fullname = grub_strdup (lv->fullname)))
> + goto cache_lv_fail;
> + if (lv->idname)
> + if (!(cache->lv->idname = grub_strdup (lv->idname)))
> + goto cache_lv_fail;
> + if (lv->name)
> + if (!(cache->lv->name = grub_strdup (lv->name)))
> + goto cache_lv_fail;
> +
> + skip_lv = 1;
> +
> + if (!(p2 = grub_strstr (p, "cache_pool = \"")))
> + goto cache_lv_fail;
> +
> + if (!(p2 = grub_strchr (p2, '"')))
> + goto cache_lv_fail;
> +
> + p3 = ++p2;
> + if (!(p3 = grub_strchr (p3, '"')))
> + goto cache_lv_fail;
> +
> + sz = p3 - p2;
> +
> + if (!(cache->cache_pool = grub_malloc (sz + 1)))
> + goto cache_lv_fail;
> + grub_memcpy (cache->cache_pool, p2, sz);
> + cache->cache_pool[sz] = '\0';
> +
> + if (!(p2 = grub_strstr (p, "origin = \"")))
> + goto cache_lv_fail;
> +
> + if (!(p2 = grub_strchr (p2, '"')))
> + goto cache_lv_fail;
> +
> + p3 = ++p2;
> + if (!(p3 = grub_strchr (p3, '"')))
> + goto cache_lv_fail;
> +
> + sz = p3 - p2;
> +
> + if (!(cache->origin = grub_malloc (sz + 1)))
> + goto cache_lv_fail;
> + grub_memcpy (cache->origin, p2, sz);
> + cache->origin[sz] = '\0';
> +
> + cache->next = cache_lvs;
> + cache_lvs = cache;
> + break;
> +
> + cache_lv_fail:
> + grub_free (cache->origin);
This will crash the GRUB if first grub_zalloc() fails...
> + grub_free (cache->cache_pool);
> + grub_free (cache->lv->fullname);
> + grub_free (cache->lv->idname);
> + grub_free (cache->lv->name);
> + grub_free (cache);
> + break;
> + }
> else
> {
> #ifdef GRUB_UTIL
> @@ -747,6 +830,72 @@ grub_lvm_detect (grub_disk_t disk,
> }
>
> }
> +
> + {
> + struct cache_lv *cache;
> +
> + for (cache = cache_lvs; cache; cache = cache->next)
> + {
> + struct grub_diskfilter_lv *lv;
> +
> + for (lv = vg->lvs; lv; lv = lv->next)
> + {
> + if (grub_strcmp (lv->name, cache->origin) == 0)
> + break;
> + }
You can drop these curly brackets.
> + if (lv)
> + {
> + if (!(cache->lv->segments = grub_malloc (lv->segment_count *
> sizeof (*lv->segments))))
> + continue;
Why do you silently ignore grub_malloc() failure?
> + grub_memcpy (cache->lv->segments, lv->segments,
> lv->segment_count * sizeof (*lv->segments));
> +
> + for (i = 0; i < lv->segment_count; ++i)
> + {
> + struct grub_diskfilter_node *nodes = lv->segments[i].nodes;
> + grub_size_t node_count = lv->segments[i].node_count;
Please add an empty line here.
> + if (!(cache->lv->segments[i].nodes = grub_malloc
> (node_count * sizeof (*nodes))))
> + {
I think that you should print an error here too.
> + for (j = 0; j < i; ++j)
> + grub_free (cache->lv->segments[j].nodes);
> + grub_free (cache->lv->segments);
> + cache->lv->segments = NULL;
> + break;
> + }
> + grub_memcpy (cache->lv->segments[i].nodes, nodes,
> node_count * sizeof (*nodes));
> + }
> +
> + if (cache->lv->segments)
> + {
> + cache->lv->segment_count = lv->segment_count;
> + cache->lv->vg = vg;
> + cache->lv->next = vg->lvs;
> + vg->lvs = cache->lv;
> + cache->lv = NULL;
Why do you need to set cache->lv to NULL?
> + }
> + }
> + }
> +
> + while ((cache = cache_lvs))
> + {
> + cache_lvs = cache_lvs->next;
> +
> + if (cache->lv)
> + {
> + for (i = 0; i < cache->lv->segment_count; ++i)
> + if (cache->lv->segments)
> + grub_free (cache->lv->segments[i].nodes);
> + grub_free (cache->lv->segments);
> + grub_free (cache->lv->fullname);
> + grub_free (cache->lv->idname);
> + grub_free (cache->lv->name);
> + }
> + grub_free (cache->lv);
> + grub_free (cache->origin);
> + grub_free (cache->cache_pool);
> + grub_free (cache);
> + }
> + }
> +
> if (grub_diskfilter_vg_register (vg))
> goto fail4;
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
