[lvm-devel] [PATCH 4/7] Introduce vg_add_lc and vg_remove_lv functions.
Petr Rockai
prockai at redhat.com
Sun May 10 19:06:37 UTC 2009
Milan Broz <mbroz at redhat.com> writes:
> vg_add_lv and vg_remove_lv are the only functions for
> adding/removing logical volume into volume group.
Another good refactor.
> Only these function should manipulate with vg->lvs list and (together with
> following patches) will update VG logical volume counter (vg->lv_count).
I guess there's no easy way in C to enforce this? (vg->lvs is only ever touched
by vg_add_lv, vg_remove_lv). Not necessary, but would be nice to have.
Btw., would vg_link_lv and vg_unlink_lv describe better what these two
functions do? It seems a little easy to confuse _add_lv and vg_add_lv, while
the two do quite a different thing.
Acked-By: Petr Rockai <prockai at redhat.com>
(Detailed review comments inline.)
LVM1 format
-----------
> diff --git a/lib/format1/disk-rep.h b/lib/format1/disk-rep.h
> index 4a3b31a..138794e 100644
> --- a/lib/format1/disk-rep.h
> +++ b/lib/format1/disk-rep.h
> @@ -215,7 +215,8 @@ int import_vg(struct dm_pool *mem,
> struct volume_group *vg, struct disk_list *dl);
> int export_vg(struct vg_disk *vgd, struct volume_group *vg);
>
> -int import_lv(struct dm_pool *mem, struct logical_volume *lv, struct lv_disk *lvd);
> +int import_lv(struct cmd_context *cmd, struct dm_pool *mem,
> + struct logical_volume *lv, struct lv_disk *lvd);
>
> int import_extents(struct cmd_context *cmd, struct volume_group *vg,
> struct dm_list *pvds);
> diff --git a/lib/format1/import-export.c b/lib/format1/import-export.c
> index 627ee05..573836d 100644
> --- a/lib/format1/import-export.c
> +++ b/lib/format1/import-export.c
> @@ -294,10 +294,9 @@ int export_vg(struct vg_disk *vgd, struct volume_group *vg)
> return 1;
> }
>
> -int import_lv(struct dm_pool *mem, struct logical_volume *lv, struct lv_disk *lvd)
> +int import_lv(struct cmd_context *cmd, struct dm_pool *mem,
> + struct logical_volume *lv, struct lv_disk *lvd)
> {
> - lvid_from_lvnum(&lv->lvid, &lv->vg->id, lvd->lv_number);
> -
> if (!(lv->name = _create_lv_name(mem, (char *)lvd->lv_name)))
> return_0;
>
> @@ -331,7 +330,7 @@ int import_lv(struct dm_pool *mem, struct logical_volume *lv, struct lv_disk *lv
> lv->alloc = ALLOC_NORMAL;
>
> if (!lvd->lv_read_ahead)
> - lv->read_ahead = lv->vg->cmd->default_settings.read_ahead;
> + lv->read_ahead = cmd->default_settings.read_ahead;
> else
> lv->read_ahead = lvd->lv_read_ahead;
>
I'm not sure why is this change needed? (Adding cmd_context parameter to
import_lv.)
> @@ -456,22 +455,19 @@ static struct logical_volume *_add_lv(struct dm_pool *mem,
> struct volume_group *vg,
> struct lv_disk *lvd)
> {
> - struct lv_list *ll;
> struct logical_volume *lv;
>
> - if (!(ll = dm_pool_zalloc(mem, sizeof(*ll))) ||
> - !(ll->lv = dm_pool_zalloc(mem, sizeof(*ll->lv))))
> + if (!(lv = dm_pool_zalloc(mem, sizeof(*lv))))
> return_NULL;
> - lv = ll->lv;
> - lv->vg = vg;
>
> - if (!import_lv(mem, lv, lvd))
> - return_NULL;
> + lvid_from_lvnum(&lv->lvid, &vg->id, lvd->lv_number);
>
> - dm_list_add(&vg->lvs, &ll->list);
> - vg->lv_count++;
> + if (!import_lv(vg->cmd, mem, lv, lvd)) {
> + dm_pool_free(mem, lv);
> + return_NULL;
> + }
>
> - return lv;
> + return vg_add_lv(vg, lv) ? lv : NULL;
> }
Does not change what the code does, just shuffles the lines somewhat. It does
change semantics of import_lv, but that function is never used outside _add_lv
anyway (would it make sense to make it static _import_lv, in a separate
patch?).
Pool Format
-----------
> diff --git a/lib/format_pool/import_export.c b/lib/format_pool/import_export.c
> index 4d2163d..91e3bcd 100644
> --- a/lib/format_pool/import_export.c
> +++ b/lib/format_pool/import_export.c
> @@ -58,22 +58,14 @@ int import_pool_vg(struct volume_group *vg, struct dm_pool *mem, struct dm_list
> int import_pool_lvs(struct volume_group *vg, struct dm_pool *mem, struct dm_list *pls)
> {
> struct pool_list *pl;
> - struct lv_list *lvl = dm_pool_zalloc(mem, sizeof(*lvl));
> struct logical_volume *lv;
>
> - if (!lvl) {
> - log_error("Unable to allocate lv list structure");
> - return 0;
> - }
> -
> - if (!(lvl->lv = dm_pool_zalloc(mem, sizeof(*lvl->lv)))) {
> + if (!(lv = dm_pool_zalloc(mem, sizeof(*lv)))) {
> log_error("Unable to allocate logical volume structure");
> return 0;
> }
>
> - lv = lvl->lv;
> lv->status = 0;
> - lv->vg = vg;
> lv->alloc = ALLOC_NORMAL;
> lv->size = 0;
> lv->name = NULL;
> @@ -115,11 +107,8 @@ int import_pool_lvs(struct volume_group *vg, struct dm_pool *mem, struct dm_list
> }
>
> lv->le_count = lv->size / POOL_PE_SIZE;
> - lvl->lv = lv;
> - dm_list_add(&vg->lvs, &lvl->list);
> - vg->lv_count++;
>
> - return 1;
> + return vg_add_lv(vg, lv);
> }
Again, harmless reordering of code. As a side-effect, unifies the error
handling code with other instances of similar code.
Text (LVM2) format
------------------
> diff --git a/lib/format_text/import_vsn1.c b/lib/format_text/import_vsn1.c
> index a9748aa..52ef075 100644
> --- a/lib/format_text/import_vsn1.c
> +++ b/lib/format_text/import_vsn1.c
> @@ -495,15 +495,11 @@ static int _read_lvnames(struct format_instance *fid __attribute((unused)),
> struct dm_hash_table *pv_hash __attribute((unused)))
> {
> struct logical_volume *lv;
> - struct lv_list *lvl;
> struct config_node *cn;
>
> - if (!(lvl = dm_pool_zalloc(mem, sizeof(*lvl))) ||
> - !(lvl->lv = dm_pool_zalloc(mem, sizeof(*lvl->lv))))
> + if (!(lv = dm_pool_zalloc(mem, sizeof(*lv))))
> return_0;
>
> - lv = lvl->lv;
> -
> if (!(lv->name = dm_pool_strdup(mem, lvn->key)))
> return_0;
>
> @@ -561,11 +557,7 @@ static int _read_lvnames(struct format_instance *fid __attribute((unused)),
> return 0;
> }
>
> - lv->vg = vg;
> - vg->lv_count++;
> - dm_list_add(&vg->lvs, &lvl->list);
> -
> - return 1;
> + return vg_add_lv(vg, lv);
> }
The new code is equivalent to the older version, just the ordering is
(harmlessly) different.
General metadata code
---------------------
> diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
> index 7214b27..1903320 100644
> --- a/lib/metadata/lv_manip.c
> +++ b/lib/metadata/lv_manip.c
> @@ -402,7 +402,6 @@ static int _lv_segment_reduce(struct lv_segment *seg, uint32_t reduction)
> */
> static int _lv_reduce(struct logical_volume *lv, uint32_t extents, int delete)
> {
> - struct lv_list *lvl;
> struct lv_segment *seg;
> uint32_t count = extents;
> uint32_t reduction;
> @@ -434,13 +433,11 @@ static int _lv_reduce(struct logical_volume *lv, uint32_t extents, int delete)
>
> /* Remove the LV if it is now empty */
> if (!lv->le_count) {
> - if (!(lvl = find_lv_in_vg(lv->vg, lv->name)))
> + if(!vg_remove_lv(lv))
> return_0;
> -
> - dm_list_del(&lvl->list);
> -
> - if (!(lv->status & SNAPSHOT))
> - lv->vg->lv_count--;
> + //FIXME: cow still in VG, fix count, remove this later
> + if (lv->status & SNAPSHOT)
> + lv->vg->lv_count++;
^^ This is indeed ugly (see also my previous mail: it might be worth getting
rid of lv_count altogether) ... although this patch is a definite improvement,
it is still suboptimal in this respect.
> } else if (lv->vg->fid->fmt->ops->lv_setup &&
> !lv->vg->fid->fmt->ops->lv_setup(lv->vg->fid, lv))
> return_0;
> @@ -1822,8 +1819,6 @@ struct logical_volume *lv_create_empty(const char *name,
> struct volume_group *vg)
> {
> struct format_instance *fi = vg->fid;
> - struct cmd_context *cmd = vg->cmd;
> - struct lv_list *ll = NULL;
> struct logical_volume *lv;
> char dname[NAME_LEN];
>
> @@ -1843,23 +1838,11 @@ struct logical_volume *lv_create_empty(const char *name,
> if (!import)
> log_verbose("Creating logical volume %s", name);
>
> - if (!(ll = dm_pool_zalloc(cmd->mem, sizeof(*ll))) ||
> - !(ll->lv = dm_pool_zalloc(cmd->mem, sizeof(*ll->lv)))) {
> - log_error("lv_list allocation failed");
> - if (ll)
> - dm_pool_free(cmd->mem, ll);
> - return NULL;
> - }
> + if (!(lv = dm_pool_zalloc(vg->vgmem, sizeof(*lv))))
> + return_NULL;
.
>
> - lv = ll->lv;
> - lv->vg = vg;
> -
> - if (!(lv->name = dm_pool_strdup(cmd->mem, name))) {
> - log_error("lv name strdup failed");
> - if (ll)
> - dm_pool_free(cmd->mem, ll);
> - return NULL;
> - }
> + if (!(lv->name = dm_pool_strdup(vg->vgmem, name)))
> + goto_bad;
.
>
> lv->status = status;
> lv->alloc = alloc;
> @@ -1877,18 +1860,20 @@ struct logical_volume *lv_create_empty(const char *name,
> if (lvid)
> lv->lvid = *lvid;
>
> - if (fi->fmt->ops->lv_setup && !fi->fmt->ops->lv_setup(fi, lv)) {
> - if (ll)
> - dm_pool_free(cmd->mem, ll);
> - return_NULL;
> - }
> + if (!vg_add_lv(vg, lv))
> + goto_bad;
.
>
> - if (!import)
> - vg->lv_count++;
> + if (fi->fmt->ops->lv_setup && !fi->fmt->ops->lv_setup(fi, lv))
> + goto_bad;
>
> - dm_list_add(&vg->lvs, &ll->list);
> + //FIXME: remove that
> + if (import)
> + vg->lv_count--;
The same about the FIXME, see above.
>
> return lv;
> +bad:
> + dm_pool_free(vg->vgmem, lv);
> + return_NULL;
> }
Other than that, the code looks equivalent to me, although there's a fair
amount of logic shuffling. It goes to a great length to make the function less
tangled though, so I guess it's a good thing (for example it gets rid of the
repeated error handler by using goto_bad instead).
> static int _add_pvs(struct cmd_context *cmd, struct pv_segment *peg,
> @@ -1957,6 +1942,36 @@ struct dm_list *build_parallel_areas_from_lv(struct cmd_context *cmd,
> return parallel_areas;
> }
>
> +int vg_add_lv(struct volume_group *vg, struct logical_volume *lv)
> +{
> + struct lv_list *lvl;
> +
> + if (!(lvl = dm_pool_zalloc(vg->vgmem, sizeof(*lvl))))
> + return_0;
> +
> + lvl->lv = lv;
> + lv->vg = vg;
> + dm_list_add(&vg->lvs, &lvl->list);
> +
> + vg->lv_count++;
> +
> + return 1;
> +}
> +
> +int vg_remove_lv(struct logical_volume *lv)
> +{
> + struct lv_list *lvl;
> +
> + if (!(lvl = find_lv_in_vg(lv->vg, lv->name)))
> + return_0;
> +
> + dm_list_del(&lvl->list);
> +
> + lvl->lv->vg->lv_count--;
> +
> + return 1;
> +}
These two look good, and are implemented in accord with how they are used in
the code above.
Yours,
Petr.
--
Peter Rockai | me()mornfall!net | prockai()redhat!com
http://blog.mornfall.net | http://web.mornfall.net
"In My Egotistical Opinion, most people's C programs should be
indented six feet downward and covered with dirt."
-- Blair P. Houghton on the subject of C program indentation
More information about the lvm-devel
mailing list