Re: [PATCH v2] lvm: add lvm cache logical volume handling

[Daniel Kiper]

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 <address@hidden>
> ---
>
> 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