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

[Michael Chang]

On Wed, Nov 13, 2019 at 03:10:41PM +0100, Daniel Kiper wrote:
> 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,/?

Yes, to be more straight forward here. :)

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

No problem.

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

You are right, and sorry for the mistake. I'll fix in next version.

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

OK.

> 
> > +       if (lv)
> > +         {
> > +           if (!(cache->lv->segments = grub_malloc (lv->segment_count * 
> > sizeof (*lv->segments))))
> > +             continue;
> 
> Why do you silently ignore grub_malloc() failure?

I think the boot process may be able to carry on as the cached lv could
be optional. But after thinking it twice turns out to be pretty lame
excuse as grub_malloc failure is fatal and no point to continue. I'll
fix this in next version.

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

OK.

> 
> > +               if (!(cache->lv->segments[i].nodes = grub_malloc 
> > (node_count * sizeof (*nodes))))
> > +                 {
> 
> I think that you should print an error here too.

OK. I'll do in next version.

> 
> > +                   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?

The cache->lv is prepended to vg->lvs list so removing its reference by
the cache->lv to avoid being cleaned up later made to the cache lv list.

> 
> > +             }
> > +         }
> > +     }
> > +
> > +     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

Thanks for review and feedback.

Regards,
Michael

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