[lvm-devel] [PATCH vgsplit 2/4] Update vgsplit to allow a list of LVs or PVs on the commandline.
Jun'ichi Nomura
j-nomura at ce.jp.nec.com
Tue Mar 25 01:11:23 UTC 2008
Hi Dave,
I quickly looked through the patch.
Please see comments below.
> Add some helper functions to allow vgsplit to continue in more cases.
> Introduce "related volumes" concept, as defined in the comments below.
>
>
> --- a/lib/metadata/metadata-exported.h
> +++ b/lib/metadata/metadata-exported.h
> @@ -151,6 +151,9 @@ struct pv_segment {
> uint32_t lv_area; /* Index to area in LV segment */
> };
>
> +#define pvseg_is_allocated(pvseg) (pvseg->lvseg)
> +#define pvseg_is_free(pvseg) (!pvseg_is_allocated(pvseg))
Macro parameters should be surrounded by ().
#define pvseg_is_allocated(pvseg) ((pvseg)->lvseg)
#define pvseg_is_free(pvseg) (!pvseg_is_allocated((pvseg)))
> +struct related_volumes {
> + struct volume_group *vg;
> + struct list ll; /* struct lv_list */
> + struct list pl; /* struct pv_list */
> +};
I think 'lvs' and 'pvs' are more descriptive.
struct related_volumes {
struct volume_group *vg;
struct list lvs; /* struct lv_list */
struct list pvs; /* struct pv_list */
};
> +void pv_add_related_volume(struct related_volumes *rvs, struct pv_list *pvl);
> +void lv_add_related_volume(struct related_volumes *rvs, struct lv_list *lvl);
On the analogy of other function names in LVM2,
these sound like 'adding "related volume" to pv/lv'.
What these functions actually do is collecting related volumes for
given LV/PV and adding them to rvs.
How about collect_related_volumes_for_{pv,lv} or collect_rvs_for_{pv,lv}?
Or IOW, the functions add given PV/LV (and its relatives) to rvs.
So add_{pv,lv}_to_related_volumes() might be good as well.
> +static void _move_pv(struct volume_group *vg_from, struct volume_group *vg_to,
> + struct pv_list *pvl)
> +{
> + struct physical_volume *pv;
> +
> + list_del(&pvl->list);
> + list_add(&vg_to->pvs, &pvl->list);
Since this del-and-add logic appears many times through this patch,
how about adding list_move() to lib/datastruct/list.h?
static inline list_move(struct list *item, struct list *head)
{
list_del(item);
list_add(head, item);
}
> + pv = list_item(pvl, struct pv_list)->pv;
This is a bug, though it works with luck by struct pv_list layout.
It should be:
pv = pvl->pv;
Not a problem of this patch, as it exists in the original code.
> +static int find_in_pv_list(const struct list *pl,
> + const struct physical_volume *pv)
I think other find_* functions in LVM2 returns what it finds.
Also, names of static functions tend to start with '_'.
So how about _find_pv_in_pv_list() and return a pointer to
the matched pvl otherwise NULL?
Or _pv_is_in_pv_list(), if the matched pvl has no use.
> +{
> + const struct pv_list *pvl;
> +
> + list_iterate_items(pvl, pl)
> + if (pvl->pv == pv)
> + return 1;
> + return 0;
> +}
_add_pvs() in lib/metadata/lv_manip.c can use this function, too.
> +static int find_in_lv_list(const struct list *ll,
> + const struct logical_volume *lv)
Ditto about the naming.
> +static void add_related_pv(struct related_volumes *rvs,
> + const struct physical_volume *pv)
...
> +static void add_related_lv(struct related_volumes *rvs,
> + const struct logical_volume *lv)
...
> +static void add_related_segment(struct related_volumes *rvs,
> + const struct lv_segment *lvseg)
These names sound like 'adding given pv/lv/lvseg to rvs'.
but actually are 'adding related volumes for pv/lv/lvseg to rvs'.
So similar renaming as pv_add_related_volume() would be nice, IMO.
> + if (!list_empty(&lvseg->lv->segs_using_this_lv)) {
> + lvseg2 = get_only_segment_using_this_lv(lvseg->lv);
> + if (lvseg2)
> + add_related_segment(rvs, lvseg2);
> + }
First, I think this part should be moved to lv_find_related_volumes()
because segs_using_this_lv is a property of lv, not lvseg.
Second, you should walk through segs_using_this_lv, instead of
using get_only_segment_using_this_lv(). e.g. "pvmove" LV may be used by
multiple lvsegs.
> @@ -302,28 +146,64 @@ int vgsplit(struct cmd_context *cmd, int argc, char **argv)
...
> + init_related_volumes(&rvs, vg_from);
> + lv_count_cmdline = 0;
> + pv_count_cmdline = 0;
> + for (opt = 0; opt < argc; opt++) {
> + /* FIXME: handle tags */
> + if ((lvl = find_lv_in_vg(vg_from, argv[opt]))) {
> + lv_count_cmdline++;
> + lv_add_related_volume(&rvs, lvl);
> + } else if ((pvl = find_pv_in_vg(vg_from, argv[opt]))) {
> + pv_count_cmdline++;
> + pv_add_related_volume(&rvs, pvl);
> + }
> }
>
> - /* Move required LVs across, checking consistency */
> - if (!(_move_lvs(vg_from, vg_to)))
> - goto error;
> + /*
> + * Now that we have the lists of all PVs and LVs involved in the
> + * split, check if this list contains PVs or LVs above what the user
> + * requested. If so, ask for confirmation before continuing, as this
> + * is most likely what the user would expect.
> + */
> + if ( ((pv_count_cmdline && list_size(&rvs.pl) > pv_count_cmdline) ||
> + (lv_count_cmdline && list_size(&rvs.ll) > lv_count_cmdline)) &&
> + !arg_count(cmd, force_ARG) ) {
> + if (!confirm_vgsplit(&rvs.ll, &rvs.pl))
> + goto error;
> + }
The above condition won't work as expected if specified LV is composed
of some hidden LVs, e.g. mirror?
Thanks,
--
Jun'ichi Nomura, NEC Corporation of America
More information about the lvm-devel
mailing list