| Submitter | malahal naineni |
|---|---|
| Date | 2009-09-23 03:03:31 |
| Message ID | <20090923030331.GA26449@us.ibm.com> |
| Download | mbox | patch |
| Permalink | /patch/49461/ |
| State | Under Review |
| Delegated to: | Jonthan Brassow |
| Headers | show |
Comments
It would be nice if we didn't have to use '--mirroredlog' (a new option), but instead '--mirrorlog mirrored' (continuing use of the preferred option). I will be continuing to review this. brassow On Sep 22, 2009, at 10:03 PM, malahal@us.ibm.com wrote: > This patch adds '--mirroredlog' option to LVM commands to create a > mirror with mirrored log device. Rebased to the latest LVM code > (LVM2.2.02.51). Have not done 'lvconvert' related changes yet! > Appreciate any comments. > > diff -r c70315774a35 lib/activate/activate.c > --- a/lib/activate/activate.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/lib/activate/activate.c Tue Sep 22 19:51:15 2009 -0700 > @@ -979,6 +979,8 @@ > { > struct logical_volume *lv; > struct lvinfo info; > + struct lv_segment *seg, *log_seg; > + struct segment_type *type = get_segtype_from_string(cmd, "mirror"); > int r = 0; > > if (!activation()) > @@ -1012,6 +1014,20 @@ > if (!monitor_dev_for_events(cmd, lv, 0)) > stack; > > + if ( !dm_list_empty(&lv->segments) ) { > + seg = dm_list_item(dm_list_first(&lv->segments), struct > lv_segment); > + if (seg->log_lv) { > + log_very_verbose("lv %s is mirrored, check it's log %s...", lv- > >name, seg->log_lv->name); > + if ( !dm_list_empty(&seg->log_lv->segments) ) { > + log_seg = dm_list_item(dm_list_first(&seg->log_lv->segments), > struct lv_segment); > + if (log_seg->segtype == type) { > + log_verbose("log %s is mirrored, unregister for events", > log_seg->lv->name); > + monitor_dev_for_events(cmd, log_seg->lv, 0); > + } > + } > + } > + } > + > memlock_inc(); > r = _lv_deactivate(lv); > memlock_dec(); > diff -r c70315774a35 lib/metadata/lv_manip.c > --- a/lib/metadata/lv_manip.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/lib/metadata/lv_manip.c Tue Sep 22 19:51:15 2009 -0700 > @@ -3029,14 +3029,16 @@ > return_0; > > if (lp->mirrors > 1) { > - if (!lv_add_mirrors(cmd, lv, lp->mirrors - 1, lp->stripes, > + if (!lv_add_mirrors(cmd, lv, lp->segtype, lp->mirrors - 1, > + lp->stripes, lp->stripe_size, > adjusted_mirror_region_size( > vg->extent_size, > lv->le_count, > lp->region_size), > lp->corelog ? 0U : 1U, lp->pvh, lp->alloc, > MIRROR_BY_LV | > - (lp->nosync ? MIRROR_SKIP_INIT_SYNC : 0))) { > + (lp->nosync ? MIRROR_SKIP_INIT_SYNC : 0), > + lp->mirrored_log)) { > stack; > goto revert_new_lv; > } > diff -r c70315774a35 lib/metadata/metadata-exported.h > --- a/lib/metadata/metadata-exported.h Tue Sep 15 12:08:46 2009 -0700 > +++ b/lib/metadata/metadata-exported.h Tue Sep 22 19:51:15 2009 -0700 > @@ -528,6 +528,7 @@ > uint32_t region_size; /* mirror */ > > uint32_t mirrors; /* mirror */ > + uint32_t mirrored_log; /* mirror */ > > const struct segment_type *segtype; /* all */ > > @@ -637,9 +638,11 @@ > */ > struct lv_segment *find_mirror_seg(struct lv_segment *seg); > int lv_add_mirrors(struct cmd_context *cmd, struct logical_volume *lv, > - uint32_t mirrors, uint32_t stripes, > + const struct segment_type *segtype, > + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, > uint32_t region_size, uint32_t log_count, > - struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags); > + struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags, > + uint32_t mirrored_log); > int lv_remove_mirrors(struct cmd_context *cmd, struct logical_volume > *lv, > uint32_t mirrors, uint32_t log_count, > struct dm_list *pvs, uint32_t status_mask); > @@ -658,16 +661,21 @@ > int remove_mirror_images(struct logical_volume *lv, uint32_t > num_mirrors, > struct dm_list *removable_pvs, unsigned remove_log); > int add_mirror_images(struct cmd_context *cmd, struct logical_volume > *lv, > - uint32_t mirrors, uint32_t stripes, uint32_t region_size, > + const struct segment_type *segtype, > + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, > + uint32_t region_size, > struct dm_list *allocatable_pvs, alloc_policy_t alloc, > - uint32_t log_count); > + uint32_t log_count, uint32_t mirrored_log); > struct logical_volume *detach_mirror_log(struct lv_segment *seg); > int attach_mirror_log(struct lv_segment *seg, struct logical_volume > *lv); > int remove_mirror_log(struct cmd_context *cmd, struct logical_volume > *lv, > struct dm_list *removable_pvs); > int add_mirror_log(struct cmd_context *cmd, struct logical_volume *lv, > + const struct segment_type *segtype, > uint32_t log_count, uint32_t region_size, > - struct dm_list *allocatable_pvs, alloc_policy_t alloc); > + struct dm_list *allocatable_pvs, alloc_policy_t alloc, > + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, > + uint32_t mirrored_log); > > int reconfigure_mirror_images(struct lv_segment *mirrored_seg, > uint32_t num_mirrors, > struct dm_list *removable_pvs, unsigned remove_log); > diff -r c70315774a35 lib/metadata/mirror.c > --- a/lib/metadata/mirror.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/lib/metadata/mirror.c Tue Sep 22 19:51:15 2009 -0700 > @@ -1248,7 +1248,8 @@ > struct alloc_handle *ah, > alloc_policy_t alloc, > const char *lv_name, > - const char *suffix) > + const char *suffix, > + uint32_t mirrored_log) > { > struct logical_volume *log_lv; > char *log_name; > @@ -1270,7 +1271,7 @@ > alloc, lv->vg))) > return_NULL; > > - if (!lv_add_log_segment(ah, log_lv)) > + if (!mirrored_log && !lv_add_log_segment(ah, log_lv)) > return_NULL; > > return log_lv; > @@ -1279,10 +1280,16 @@ > static struct logical_volume *_set_up_mirror_log(struct cmd_context > *cmd, > struct alloc_handle *ah, > struct logical_volume *lv, > + const struct segment_type *segtype, > uint32_t log_count, > - uint32_t region_size __attribute((unused)), > + uint32_t region_size, > alloc_policy_t alloc, > - int in_sync) > + int in_sync, > + uint32_t mirrors, > + uint32_t stripes, > + uint32_t stripe_size, > + struct dm_list *allocatable_pvs, > + uint32_t mirrored_log) > { > struct logical_volume *log_lv; > const char *suffix, *c; > @@ -1324,11 +1331,20 @@ > } > > if (!(log_lv = _create_mirror_log(lv, ah, alloc, > - (const char *) lv_name, suffix))) { > + (const char *) lv_name, suffix, mirrored_log))) { > log_error("Failed to create mirror log."); > return NULL; > } > > + if (mirrored_log) { > + if (!lv_extend(log_lv, segtype, stripes, stripe_size, > + 1u, 1u, NULL, 0u, 0u, allocatable_pvs, alloc)) > + return NULL; > + > + add_mirror_images(cmd, log_lv, segtype, mirrors, stripes, > stripe_size, > + region_size, allocatable_pvs, alloc, 0, 0); > + } > + > if (!_init_mirror_log(cmd, log_lv, in_sync, &lv->tags, 1)) { > log_error("Failed to create mirror log."); > return NULL; > @@ -1346,8 +1362,11 @@ > } > > int add_mirror_log(struct cmd_context *cmd, struct logical_volume *lv, > + const struct segment_type *seg_type, > uint32_t log_count, uint32_t region_size, > - struct dm_list *allocatable_pvs, alloc_policy_t alloc) > + struct dm_list *allocatable_pvs, alloc_policy_t alloc, > + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, > + uint32_t mirrored_log) > { > struct alloc_handle *ah; > const struct segment_type *segtype; > @@ -1410,8 +1429,10 @@ > else > in_sync = 0; > > - if (!(log_lv = _set_up_mirror_log(cmd, ah, lv, log_count, > - region_size, alloc, in_sync))) > + if (!(log_lv = _set_up_mirror_log(cmd, ah, lv, seg_type, log_count, > + region_size, alloc, in_sync, > + mirrors, stripes, stripe_size, allocatable_pvs, > + mirrored_log))) > goto_out; > > if (!attach_mirror_log(first_seg(lv), log_lv)) > @@ -1427,9 +1448,10 @@ > * Convert "linear" LV to "mirror". > */ > int add_mirror_images(struct cmd_context *cmd, struct logical_volume > *lv, > - uint32_t mirrors, uint32_t stripes, uint32_t region_size, > + const struct segment_type *seg_type, uint32_t mirrors, > + uint32_t stripes, uint32_t stripe_size, uint32_t region_size, > struct dm_list *allocatable_pvs, alloc_policy_t alloc, > - uint32_t log_count) > + uint32_t log_count, uint32_t mirrored_log) > { > struct alloc_handle *ah; > const struct segment_type *segtype; > @@ -1452,9 +1474,18 @@ > if (!(segtype = get_segtype_from_string(cmd, "mirror"))) > return_0; > > - ah = allocate_extents(lv->vg, NULL, segtype, > - stripes, mirrors, log_count, region_size, lv->le_count, > - allocatable_pvs, alloc, parallel_areas); > + if (mirrored_log) > + /* Allocate mirror extents for the log */ > + ah = allocate_extents(lv->vg, NULL, segtype, > + stripes, mirrors, 0, > + region_size, 1, > + allocatable_pvs, alloc, parallel_areas); > + else > + ah = allocate_extents(lv->vg, NULL, segtype, > + stripes, mirrors, log_count, > + region_size, lv->le_count, > + allocatable_pvs, alloc, parallel_areas); > + > if (!ah) { > log_error("Unable to allocate extents for mirror(s)."); > return 0; > @@ -1463,11 +1494,25 @@ > /* > * create and initialize mirror log > */ > - if (log_count && > - !(log_lv = _set_up_mirror_log(cmd, ah, lv, log_count, > region_size, > - alloc, mirror_in_sync()))) { > - stack; > - goto out_remove_images; > + if (log_count) { > + if (!(log_lv = _set_up_mirror_log(cmd, ah, lv, seg_type, > log_count, region_size, > + alloc, mirror_in_sync(), mirrors, stripes, > + stripe_size, allocatable_pvs, mirrored_log))) { > + stack; > + goto out_remove_images; > + } > + if (mirrored_log) { > + /* Allocate extents for the mirror legs */ > + alloc_destroy(ah); > + ah = allocate_extents(lv->vg, NULL, segtype, > + stripes, mirrors, 0, > + region_size, lv->le_count, > + allocatable_pvs, alloc, parallel_areas); > + if (!ah) { > + stack; > + goto out_remove_images; > + } > + } > } > > /* The log initialization involves vg metadata commit. > @@ -1529,9 +1574,11 @@ > * 'pvs' is either allocatable pvs. > */ > int lv_add_mirrors(struct cmd_context *cmd, struct logical_volume *lv, > - uint32_t mirrors, uint32_t stripes, > + const struct segment_type *seg_type, uint32_t mirrors, > + uint32_t stripes, uint32_t stripe_size, > uint32_t region_size, uint32_t log_count, > - struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags) > + struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags, > + uint32_t mirrored_log) > { > if (!mirrors && !log_count) { > log_error("No conversion is requested"); > @@ -1564,11 +1611,13 @@ > region_size, pvs, alloc); > } else if (flags & MIRROR_BY_LV) { > if (!mirrors) > - return add_mirror_log(cmd, lv, log_count, > - region_size, pvs, alloc); > - return add_mirror_images(cmd, lv, mirrors, > - stripes, region_size, > - pvs, alloc, log_count); > + return add_mirror_log(cmd, lv, seg_type, log_count, > + region_size, pvs, alloc, mirrors, > + stripes, stripe_size, mirrored_log); > + return add_mirror_images(cmd, lv, seg_type, mirrors, > + stripes, stripe_size, region_size, > + pvs, alloc, log_count, > + mirrored_log); > } > > log_error("Unsupported mirror conversion type"); > diff -r c70315774a35 tools/args.h > --- a/tools/args.h Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/args.h Tue Sep 22 19:51:15 2009 -0700 > @@ -45,6 +45,7 @@ > arg(alloc_ARG, '\0', "alloc", alloc_arg, 0) > arg(separator_ARG, '\0', "separator", string_arg, 0) > arg(mirrorsonly_ARG, '\0', "mirrorsonly", NULL, 0) > +arg(mirroredlog_ARG, '\0', "mirroredlog", NULL, 0) > arg(nosync_ARG, '\0', "nosync", NULL, 0) > arg(resync_ARG, '\0', "resync", NULL, 0) > arg(corelog_ARG, '\0', "corelog", NULL, 0) > diff -r c70315774a35 tools/commands.h > --- a/tools/commands.h Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/commands.h Tue Sep 22 19:51:15 2009 -0700 > @@ -95,8 +95,7 @@ > "Change logical volume layout", > 0, > "lvconvert " > - "[-m|--mirrors Mirrors [{--mirrorlog {disk|core}|--corelog}]]\n" > - "\t[--repair [--use-policies]]\n" > + "[-m|--mirrors Mirrors [{--mirrorlog {disk|core}|--corelog|-- > mirroredlog}]]\n" > "\t[-R|--regionsize MirrorLogRegionSize]\n" > "\t[--alloc AllocationPolicy]\n" > "\t[-b|--background]\n" > @@ -122,7 +121,7 @@ > "\tOriginalLogicalVolume[Path] SnapshotLogicalVolume[Path]\n", > > alloc_ARG, background_ARG, chunksize_ARG, corelog_ARG, > interval_ARG, > - mirrorlog_ARG, mirrors_ARG, noudevsync_ARG, regionsize_ARG, > repair_ARG, > + mirrorlog_ARG, mirrors_ARG, noudevsync_ARG, regionsize_ARG, > repair_ARG, mirroredlog_ARG, > snapshot_ARG, test_ARG, use_policies_ARG, yes_ARG, force_ARG, > zero_ARG) > > xx(lvcreate, > @@ -139,7 +138,7 @@ > "\t{-l|--extents LogicalExtentsNumber |\n" > "\t -L|--size LogicalVolumeSize[bBsSkKmMgGtTpPeE]}\n" > "\t[-M|--persistent {y|n}] [--major major] [--minor minor]\n" > - "\t[-m|--mirrors Mirrors [--nosync] [{--mirrorlog {disk|core}|-- > corelog}]]\n" > + "\t[-m|--mirrors Mirrors [--nosync] [{--mirrorlog {disk|core}|-- > corelog|--mirroredlog}]]\n" > "\t[-n|--name LogicalVolumeName]\n" > "\t[--noudevsync]\n" > "\t[-p|--permission {r|rw}]\n" > @@ -173,9 +172,9 @@ > "\t[-t|--test]\n" > "\t[-v|--verbose]\n" > "\t[--version]\n" > + "\tOriginalLogicalVolume[Path] [PhysicalVolumePath...]\n\n", > > - "\t[PhysicalVolumePath...]\n\n", > - > + mirroredlog_ARG, > addtag_ARG, alloc_ARG, autobackup_ARG, chunksize_ARG, > contiguous_ARG, > corelog_ARG, extents_ARG, major_ARG, minor_ARG, mirrorlog_ARG, > mirrors_ARG, > name_ARG, nosync_ARG, noudevsync_ARG, permission_ARG, > persistent_ARG, > diff -r c70315774a35 tools/lvconvert.c > --- a/tools/lvconvert.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/lvconvert.c Tue Sep 22 19:51:15 2009 -0700 > @@ -31,6 +31,7 @@ > uint32_t region_size; > > uint32_t mirrors; > + uint32_t mirrored_log; > sign_t mirrors_sign; > > struct segment_type *segtype; > @@ -507,13 +508,16 @@ > int corelog) > { > struct logical_volume *original_lv = _original_lv(lv); > + > if (_using_corelog(lv) && !corelog) { > - if (!add_mirror_log(cmd, original_lv, 1, > + if (!add_mirror_log(cmd, original_lv, lp->segtype, 1, > adjusted_mirror_region_size( > lv->vg->extent_size, > lv->le_count, > lp->region_size), > - lp->pvh, lp->alloc)) > + lp->pvh, lp->alloc, > + lp->mirrors, 0, 0, > + lp->mirrored_log)) > return_0; > } else if (!_using_corelog(lv) && corelog) { > if (!remove_mirror_log(cmd, original_lv, > @@ -538,6 +542,8 @@ > int repair = arg_count(cmd, repair_ARG); > int replace_log = 1, replace_mirrors = 1; > > + lp->mirrored_log = arg_count(cmd, mirroredlog_ARG); > + > seg = first_seg(lv); > existing_mirrors = lv_mirror_count(lv); > > @@ -696,13 +702,13 @@ > * currently taken by the mirror? Would make more sense from > * user perspective. > */ > - if (!lv_add_mirrors(cmd, lv, lp->mirrors - 1, 1, > + if (!lv_add_mirrors(cmd, lv, lp->segtype, lp->mirrors - 1, 1, 0, > adjusted_mirror_region_size( > lv->vg->extent_size, > lv->le_count, > lp->region_size), > corelog ? 0U : 1U, lp->pvh, lp->alloc, > - MIRROR_BY_LV)) > + MIRROR_BY_LV, lp->mirrored_log)) > return_0; > if (lp->wait_completion) > lp->need_polling = 1; > @@ -726,13 +732,13 @@ > return 0; > } > /* FIXME: can't have multiple mlogs. force corelog. */ > - if (!lv_add_mirrors(cmd, lv, lp->mirrors - existing_mirrors, 1, > + if (!lv_add_mirrors(cmd, lv, lp->segtype, lp->mirrors - > existing_mirrors, 1, 0, > adjusted_mirror_region_size( > lv->vg->extent_size, > lv->le_count, > lp->region_size), > 0U, lp->pvh, lp->alloc, > - MIRROR_BY_LV)) > + MIRROR_BY_LV, lp->mirrored_log)) > return_0; > lv->status |= CONVERTING; > lp->need_polling = 1; > @@ -743,6 +749,7 @@ > if (!_lv_update_log_type(cmd, lp, lv, corelog)) > return_0; > } else { > + /*malahal this needs rework...*/ > log_error("Logical volume %s already has %" > PRIu32 " mirror(s).", lv->name, > lp->mirrors - 1); > diff -r c70315774a35 tools/lvcreate.c > --- a/tools/lvcreate.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/lvcreate.c Tue Sep 22 19:51:15 2009 -0700 > @@ -367,6 +367,9 @@ > lp->region_size = region_size; > } > > + if (arg_count(cmd, mirroredlog_ARG)) > + lp->mirrored_log = 1; > + > if (!_validate_mirror_params(cmd, lp)) > return 0; > > diff -r c70315774a35 tools/pvmove.c > --- a/tools/pvmove.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/pvmove.c Tue Sep 22 19:51:15 2009 -0700 > @@ -244,8 +244,8 @@ > return NULL; > } > > - if (!lv_add_mirrors(cmd, lv_mirr, 1, 1, 0, log_count, > - allocatable_pvs, alloc, MIRROR_BY_SEG)) { > + if (!lv_add_mirrors(cmd, lv_mirr, NULL, 1u, 1u, 0u, 0u, log_count, > + allocatable_pvs, alloc, MIRROR_BY_SEG, 0)) { > log_error("Failed to convert pvmove LV to mirrored"); > return_NULL; > } > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Jonathan Brassow [jbrassow@redhat.com] wrote: > It would be nice if we didn't have to use '--mirroredlog' (a new option), > but instead '--mirrorlog mirrored' (continuing use of the preferred > option). We can definitely change that. > I will be continuing to review this. Thank you very much. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
malahal@us.ibm.com wrote: > This patch adds '--mirroredlog' option to LVM commands to create a > mirror with mirrored log device. Rebased to the latest LVM code > (LVM2.2.02.51). Have not done 'lvconvert' related changes yet! > Appreciate any comments. Thank you for the update of your mirroredlog patch. I'll also review and test it, and then give some feedback here. Thanks, Taka -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Looking through the code before applying your patch, it seems that someone has already thought about this issue - even if it hasn't been implemented. For example, already in the top most function used to create mirrors, 'lv_add_mirrors', we see a 'log_count' parameter. That parameter can be traced down through 'add_mirror_images/log', '_set_up_mirror_log', and even the allocation functions. In fact, the first comment in 'add_mirror_log' is /* Unimplemented features */, followed by a check to see if 'log_count > 1'. Your patch seems to ignore 'log_count' and create new parameters (like mirroredlog), which seem unnecessary to me... I don't understand why any of the new parameters to the functions are necessary, can you explain? [I can see new parameters for '_create_mirror_log' though, as it doesn't seem to maintain the 'log_count' parameter - but you didn't do work in that function.] You also seem to have violated the allocation policies by ignoring the line of work that has been done up to '_set_up_mirror_log' by simply calling 'add_mirror_images'. This works, but it is oversimplified, I think. You can see this is incorrect by simply testing: prompt> lvcreate -m1 -L 500M -n lv vg /dev/sd[xy]1 # will fail because there are only two devices prompt> lvcreate ... --mirroredlog /dev/sd[xy]1 # should fail, but succeeds due to ignoring previous allocation work You may wish to push your enhancements into '_[create | init]_mirror_log'? Additionally, the 'log_count' parameter is more general than 'mirroredlog' and can support more log types. Consider the following: --mirrorlog core => (log_count = 0) --mirrorlog disk => (log_count = 1) --mirrorlog redundant => (log_count = 2) --mirrorlog fully_redundant => (log_count = # of mirror legs) You are looking to add "redundant" (you can call it "mirrored" if you like), but if we use 'log_count' in a general way, we get "fully_redundant" (almost) for free. I probably haven't given this code as much time as you have, so if there are reasons for the way you implemented it, please help inform me. As long as we are on the topic of mirror logs (and this is unrelated to your patch), I'd like to complain about the current behavior of log allocation vs the log policy. I don't think it should take 'alloc_anywhere' for a log to be placed on the same disk as one of the mirror legs... that should be 'alloc_normal'. I think it should be something like: ALLOC_CONTIGUOUS: logs and images on all separate devices - no overlap ALLOC_NORMAL: Allow logs to exist on the same devices as the mirror images. ALLOC_ANYWHERE: Should we allow mirror images on the same device!?! (i.e. everything can be packed on one device) It is silly to me that a user with two devices cannot create the default mirror. It makes even less sense when you consider mirrors with redundant logs, as this just adds to the number of devices you need. I'd love to see this fixed, but overthrowing and established policy may be difficult. brassow On Sep 22, 2009, at 10:03 PM, malahal@us.ibm.com wrote: > This patch adds '--mirroredlog' option to LVM commands to create a > mirror with mirrored log device. Rebased to the latest LVM code > (LVM2.2.02.51). Have not done 'lvconvert' related changes yet! > Appreciate any comments. > > diff -r c70315774a35 lib/activate/activate.c > --- a/lib/activate/activate.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/lib/activate/activate.c Tue Sep 22 19:51:15 2009 -0700 > @@ -979,6 +979,8 @@ > { > struct logical_volume *lv; > struct lvinfo info; > + struct lv_segment *seg, *log_seg; > + struct segment_type *type = get_segtype_from_string(cmd, "mirror"); > int r = 0; > > if (!activation()) > @@ -1012,6 +1014,20 @@ > if (!monitor_dev_for_events(cmd, lv, 0)) > stack; > > + if ( !dm_list_empty(&lv->segments) ) { > + seg = dm_list_item(dm_list_first(&lv->segments), struct > lv_segment); > + if (seg->log_lv) { > + log_very_verbose("lv %s is mirrored, check it's log %s...", lv- > >name, seg->log_lv->name); > + if ( !dm_list_empty(&seg->log_lv->segments) ) { > + log_seg = dm_list_item(dm_list_first(&seg->log_lv->segments), > struct lv_segment); > + if (log_seg->segtype == type) { > + log_verbose("log %s is mirrored, unregister for events", > log_seg->lv->name); > + monitor_dev_for_events(cmd, log_seg->lv, 0); > + } > + } > + } > + } > + > memlock_inc(); > r = _lv_deactivate(lv); > memlock_dec(); > diff -r c70315774a35 lib/metadata/lv_manip.c > --- a/lib/metadata/lv_manip.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/lib/metadata/lv_manip.c Tue Sep 22 19:51:15 2009 -0700 > @@ -3029,14 +3029,16 @@ > return_0; > > if (lp->mirrors > 1) { > - if (!lv_add_mirrors(cmd, lv, lp->mirrors - 1, lp->stripes, > + if (!lv_add_mirrors(cmd, lv, lp->segtype, lp->mirrors - 1, > + lp->stripes, lp->stripe_size, > adjusted_mirror_region_size( > vg->extent_size, > lv->le_count, > lp->region_size), > lp->corelog ? 0U : 1U, lp->pvh, lp->alloc, > MIRROR_BY_LV | > - (lp->nosync ? MIRROR_SKIP_INIT_SYNC : 0))) { > + (lp->nosync ? MIRROR_SKIP_INIT_SYNC : 0), > + lp->mirrored_log)) { > stack; > goto revert_new_lv; > } > diff -r c70315774a35 lib/metadata/metadata-exported.h > --- a/lib/metadata/metadata-exported.h Tue Sep 15 12:08:46 2009 -0700 > +++ b/lib/metadata/metadata-exported.h Tue Sep 22 19:51:15 2009 -0700 > @@ -528,6 +528,7 @@ > uint32_t region_size; /* mirror */ > > uint32_t mirrors; /* mirror */ > + uint32_t mirrored_log; /* mirror */ > > const struct segment_type *segtype; /* all */ > > @@ -637,9 +638,11 @@ > */ > struct lv_segment *find_mirror_seg(struct lv_segment *seg); > int lv_add_mirrors(struct cmd_context *cmd, struct logical_volume *lv, > - uint32_t mirrors, uint32_t stripes, > + const struct segment_type *segtype, > + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, > uint32_t region_size, uint32_t log_count, > - struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags); > + struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags, > + uint32_t mirrored_log); > int lv_remove_mirrors(struct cmd_context *cmd, struct logical_volume > *lv, > uint32_t mirrors, uint32_t log_count, > struct dm_list *pvs, uint32_t status_mask); > @@ -658,16 +661,21 @@ > int remove_mirror_images(struct logical_volume *lv, uint32_t > num_mirrors, > struct dm_list *removable_pvs, unsigned remove_log); > int add_mirror_images(struct cmd_context *cmd, struct logical_volume > *lv, > - uint32_t mirrors, uint32_t stripes, uint32_t region_size, > + const struct segment_type *segtype, > + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, > + uint32_t region_size, > struct dm_list *allocatable_pvs, alloc_policy_t alloc, > - uint32_t log_count); > + uint32_t log_count, uint32_t mirrored_log); > struct logical_volume *detach_mirror_log(struct lv_segment *seg); > int attach_mirror_log(struct lv_segment *seg, struct logical_volume > *lv); > int remove_mirror_log(struct cmd_context *cmd, struct logical_volume > *lv, > struct dm_list *removable_pvs); > int add_mirror_log(struct cmd_context *cmd, struct logical_volume *lv, > + const struct segment_type *segtype, > uint32_t log_count, uint32_t region_size, > - struct dm_list *allocatable_pvs, alloc_policy_t alloc); > + struct dm_list *allocatable_pvs, alloc_policy_t alloc, > + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, > + uint32_t mirrored_log); > > int reconfigure_mirror_images(struct lv_segment *mirrored_seg, > uint32_t num_mirrors, > struct dm_list *removable_pvs, unsigned remove_log); > diff -r c70315774a35 lib/metadata/mirror.c > --- a/lib/metadata/mirror.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/lib/metadata/mirror.c Tue Sep 22 19:51:15 2009 -0700 > @@ -1248,7 +1248,8 @@ > struct alloc_handle *ah, > alloc_policy_t alloc, > const char *lv_name, > - const char *suffix) > + const char *suffix, > + uint32_t mirrored_log) > { > struct logical_volume *log_lv; > char *log_name; > @@ -1270,7 +1271,7 @@ > alloc, lv->vg))) > return_NULL; > > - if (!lv_add_log_segment(ah, log_lv)) > + if (!mirrored_log && !lv_add_log_segment(ah, log_lv)) > return_NULL; > > return log_lv; > @@ -1279,10 +1280,16 @@ > static struct logical_volume *_set_up_mirror_log(struct cmd_context > *cmd, > struct alloc_handle *ah, > struct logical_volume *lv, > + const struct segment_type *segtype, > uint32_t log_count, > - uint32_t region_size __attribute((unused)), > + uint32_t region_size, > alloc_policy_t alloc, > - int in_sync) > + int in_sync, > + uint32_t mirrors, > + uint32_t stripes, > + uint32_t stripe_size, > + struct dm_list *allocatable_pvs, > + uint32_t mirrored_log) > { > struct logical_volume *log_lv; > const char *suffix, *c; > @@ -1324,11 +1331,20 @@ > } > > if (!(log_lv = _create_mirror_log(lv, ah, alloc, > - (const char *) lv_name, suffix))) { > + (const char *) lv_name, suffix, mirrored_log))) { > log_error("Failed to create mirror log."); > return NULL; > } > > + if (mirrored_log) { > + if (!lv_extend(log_lv, segtype, stripes, stripe_size, > + 1u, 1u, NULL, 0u, 0u, allocatable_pvs, alloc)) > + return NULL; > + > + add_mirror_images(cmd, log_lv, segtype, mirrors, stripes, > stripe_size, > + region_size, allocatable_pvs, alloc, 0, 0); > + } > + > if (!_init_mirror_log(cmd, log_lv, in_sync, &lv->tags, 1)) { > log_error("Failed to create mirror log."); > return NULL; > @@ -1346,8 +1362,11 @@ > } > > int add_mirror_log(struct cmd_context *cmd, struct logical_volume *lv, > + const struct segment_type *seg_type, > uint32_t log_count, uint32_t region_size, > - struct dm_list *allocatable_pvs, alloc_policy_t alloc) > + struct dm_list *allocatable_pvs, alloc_policy_t alloc, > + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, > + uint32_t mirrored_log) > { > struct alloc_handle *ah; > const struct segment_type *segtype; > @@ -1410,8 +1429,10 @@ > else > in_sync = 0; > > - if (!(log_lv = _set_up_mirror_log(cmd, ah, lv, log_count, > - region_size, alloc, in_sync))) > + if (!(log_lv = _set_up_mirror_log(cmd, ah, lv, seg_type, log_count, > + region_size, alloc, in_sync, > + mirrors, stripes, stripe_size, allocatable_pvs, > + mirrored_log))) > goto_out; > > if (!attach_mirror_log(first_seg(lv), log_lv)) > @@ -1427,9 +1448,10 @@ > * Convert "linear" LV to "mirror". > */ > int add_mirror_images(struct cmd_context *cmd, struct logical_volume > *lv, > - uint32_t mirrors, uint32_t stripes, uint32_t region_size, > + const struct segment_type *seg_type, uint32_t mirrors, > + uint32_t stripes, uint32_t stripe_size, uint32_t region_size, > struct dm_list *allocatable_pvs, alloc_policy_t alloc, > - uint32_t log_count) > + uint32_t log_count, uint32_t mirrored_log) > { > struct alloc_handle *ah; > const struct segment_type *segtype; > @@ -1452,9 +1474,18 @@ > if (!(segtype = get_segtype_from_string(cmd, "mirror"))) > return_0; > > - ah = allocate_extents(lv->vg, NULL, segtype, > - stripes, mirrors, log_count, region_size, lv->le_count, > - allocatable_pvs, alloc, parallel_areas); > + if (mirrored_log) > + /* Allocate mirror extents for the log */ > + ah = allocate_extents(lv->vg, NULL, segtype, > + stripes, mirrors, 0, > + region_size, 1, > + allocatable_pvs, alloc, parallel_areas); > + else > + ah = allocate_extents(lv->vg, NULL, segtype, > + stripes, mirrors, log_count, > + region_size, lv->le_count, > + allocatable_pvs, alloc, parallel_areas); > + > if (!ah) { > log_error("Unable to allocate extents for mirror(s)."); > return 0; > @@ -1463,11 +1494,25 @@ > /* > * create and initialize mirror log > */ > - if (log_count && > - !(log_lv = _set_up_mirror_log(cmd, ah, lv, log_count, > region_size, > - alloc, mirror_in_sync()))) { > - stack; > - goto out_remove_images; > + if (log_count) { > + if (!(log_lv = _set_up_mirror_log(cmd, ah, lv, seg_type, > log_count, region_size, > + alloc, mirror_in_sync(), mirrors, stripes, > + stripe_size, allocatable_pvs, mirrored_log))) { > + stack; > + goto out_remove_images; > + } > + if (mirrored_log) { > + /* Allocate extents for the mirror legs */ > + alloc_destroy(ah); > + ah = allocate_extents(lv->vg, NULL, segtype, > + stripes, mirrors, 0, > + region_size, lv->le_count, > + allocatable_pvs, alloc, parallel_areas); > + if (!ah) { > + stack; > + goto out_remove_images; > + } > + } > } > > /* The log initialization involves vg metadata commit. > @@ -1529,9 +1574,11 @@ > * 'pvs' is either allocatable pvs. > */ > int lv_add_mirrors(struct cmd_context *cmd, struct logical_volume *lv, > - uint32_t mirrors, uint32_t stripes, > + const struct segment_type *seg_type, uint32_t mirrors, > + uint32_t stripes, uint32_t stripe_size, > uint32_t region_size, uint32_t log_count, > - struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags) > + struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags, > + uint32_t mirrored_log) > { > if (!mirrors && !log_count) { > log_error("No conversion is requested"); > @@ -1564,11 +1611,13 @@ > region_size, pvs, alloc); > } else if (flags & MIRROR_BY_LV) { > if (!mirrors) > - return add_mirror_log(cmd, lv, log_count, > - region_size, pvs, alloc); > - return add_mirror_images(cmd, lv, mirrors, > - stripes, region_size, > - pvs, alloc, log_count); > + return add_mirror_log(cmd, lv, seg_type, log_count, > + region_size, pvs, alloc, mirrors, > + stripes, stripe_size, mirrored_log); > + return add_mirror_images(cmd, lv, seg_type, mirrors, > + stripes, stripe_size, region_size, > + pvs, alloc, log_count, > + mirrored_log); > } > > log_error("Unsupported mirror conversion type"); > diff -r c70315774a35 tools/args.h > --- a/tools/args.h Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/args.h Tue Sep 22 19:51:15 2009 -0700 > @@ -45,6 +45,7 @@ > arg(alloc_ARG, '\0', "alloc", alloc_arg, 0) > arg(separator_ARG, '\0', "separator", string_arg, 0) > arg(mirrorsonly_ARG, '\0', "mirrorsonly", NULL, 0) > +arg(mirroredlog_ARG, '\0', "mirroredlog", NULL, 0) > arg(nosync_ARG, '\0', "nosync", NULL, 0) > arg(resync_ARG, '\0', "resync", NULL, 0) > arg(corelog_ARG, '\0', "corelog", NULL, 0) > diff -r c70315774a35 tools/commands.h > --- a/tools/commands.h Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/commands.h Tue Sep 22 19:51:15 2009 -0700 > @@ -95,8 +95,7 @@ > "Change logical volume layout", > 0, > "lvconvert " > - "[-m|--mirrors Mirrors [{--mirrorlog {disk|core}|--corelog}]]\n" > - "\t[--repair [--use-policies]]\n" > + "[-m|--mirrors Mirrors [{--mirrorlog {disk|core}|--corelog|-- > mirroredlog}]]\n" > "\t[-R|--regionsize MirrorLogRegionSize]\n" > "\t[--alloc AllocationPolicy]\n" > "\t[-b|--background]\n" > @@ -122,7 +121,7 @@ > "\tOriginalLogicalVolume[Path] SnapshotLogicalVolume[Path]\n", > > alloc_ARG, background_ARG, chunksize_ARG, corelog_ARG, > interval_ARG, > - mirrorlog_ARG, mirrors_ARG, noudevsync_ARG, regionsize_ARG, > repair_ARG, > + mirrorlog_ARG, mirrors_ARG, noudevsync_ARG, regionsize_ARG, > repair_ARG, mirroredlog_ARG, > snapshot_ARG, test_ARG, use_policies_ARG, yes_ARG, force_ARG, > zero_ARG) > > xx(lvcreate, > @@ -139,7 +138,7 @@ > "\t{-l|--extents LogicalExtentsNumber |\n" > "\t -L|--size LogicalVolumeSize[bBsSkKmMgGtTpPeE]}\n" > "\t[-M|--persistent {y|n}] [--major major] [--minor minor]\n" > - "\t[-m|--mirrors Mirrors [--nosync] [{--mirrorlog {disk|core}|-- > corelog}]]\n" > + "\t[-m|--mirrors Mirrors [--nosync] [{--mirrorlog {disk|core}|-- > corelog|--mirroredlog}]]\n" > "\t[-n|--name LogicalVolumeName]\n" > "\t[--noudevsync]\n" > "\t[-p|--permission {r|rw}]\n" > @@ -173,9 +172,9 @@ > "\t[-t|--test]\n" > "\t[-v|--verbose]\n" > "\t[--version]\n" > + "\tOriginalLogicalVolume[Path] [PhysicalVolumePath...]\n\n", > > - "\t[PhysicalVolumePath...]\n\n", > - > + mirroredlog_ARG, > addtag_ARG, alloc_ARG, autobackup_ARG, chunksize_ARG, > contiguous_ARG, > corelog_ARG, extents_ARG, major_ARG, minor_ARG, mirrorlog_ARG, > mirrors_ARG, > name_ARG, nosync_ARG, noudevsync_ARG, permission_ARG, > persistent_ARG, > diff -r c70315774a35 tools/lvconvert.c > --- a/tools/lvconvert.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/lvconvert.c Tue Sep 22 19:51:15 2009 -0700 > @@ -31,6 +31,7 @@ > uint32_t region_size; > > uint32_t mirrors; > + uint32_t mirrored_log; > sign_t mirrors_sign; > > struct segment_type *segtype; > @@ -507,13 +508,16 @@ > int corelog) > { > struct logical_volume *original_lv = _original_lv(lv); > + > if (_using_corelog(lv) && !corelog) { > - if (!add_mirror_log(cmd, original_lv, 1, > + if (!add_mirror_log(cmd, original_lv, lp->segtype, 1, > adjusted_mirror_region_size( > lv->vg->extent_size, > lv->le_count, > lp->region_size), > - lp->pvh, lp->alloc)) > + lp->pvh, lp->alloc, > + lp->mirrors, 0, 0, > + lp->mirrored_log)) > return_0; > } else if (!_using_corelog(lv) && corelog) { > if (!remove_mirror_log(cmd, original_lv, > @@ -538,6 +542,8 @@ > int repair = arg_count(cmd, repair_ARG); > int replace_log = 1, replace_mirrors = 1; > > + lp->mirrored_log = arg_count(cmd, mirroredlog_ARG); > + > seg = first_seg(lv); > existing_mirrors = lv_mirror_count(lv); > > @@ -696,13 +702,13 @@ > * currently taken by the mirror? Would make more sense from > * user perspective. > */ > - if (!lv_add_mirrors(cmd, lv, lp->mirrors - 1, 1, > + if (!lv_add_mirrors(cmd, lv, lp->segtype, lp->mirrors - 1, 1, 0, > adjusted_mirror_region_size( > lv->vg->extent_size, > lv->le_count, > lp->region_size), > corelog ? 0U : 1U, lp->pvh, lp->alloc, > - MIRROR_BY_LV)) > + MIRROR_BY_LV, lp->mirrored_log)) > return_0; > if (lp->wait_completion) > lp->need_polling = 1; > @@ -726,13 +732,13 @@ > return 0; > } > /* FIXME: can't have multiple mlogs. force corelog. */ > - if (!lv_add_mirrors(cmd, lv, lp->mirrors - existing_mirrors, 1, > + if (!lv_add_mirrors(cmd, lv, lp->segtype, lp->mirrors - > existing_mirrors, 1, 0, > adjusted_mirror_region_size( > lv->vg->extent_size, > lv->le_count, > lp->region_size), > 0U, lp->pvh, lp->alloc, > - MIRROR_BY_LV)) > + MIRROR_BY_LV, lp->mirrored_log)) > return_0; > lv->status |= CONVERTING; > lp->need_polling = 1; > @@ -743,6 +749,7 @@ > if (!_lv_update_log_type(cmd, lp, lv, corelog)) > return_0; > } else { > + /*malahal this needs rework...*/ > log_error("Logical volume %s already has %" > PRIu32 " mirror(s).", lv->name, > lp->mirrors - 1); > diff -r c70315774a35 tools/lvcreate.c > --- a/tools/lvcreate.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/lvcreate.c Tue Sep 22 19:51:15 2009 -0700 > @@ -367,6 +367,9 @@ > lp->region_size = region_size; > } > > + if (arg_count(cmd, mirroredlog_ARG)) > + lp->mirrored_log = 1; > + > if (!_validate_mirror_params(cmd, lp)) > return 0; > > diff -r c70315774a35 tools/pvmove.c > --- a/tools/pvmove.c Tue Sep 15 12:08:46 2009 -0700 > +++ b/tools/pvmove.c Tue Sep 22 19:51:15 2009 -0700 > @@ -244,8 +244,8 @@ > return NULL; > } > > - if (!lv_add_mirrors(cmd, lv_mirr, 1, 1, 0, log_count, > - allocatable_pvs, alloc, MIRROR_BY_SEG)) { > + if (!lv_add_mirrors(cmd, lv_mirr, NULL, 1u, 1u, 0u, 0u, log_count, > + allocatable_pvs, alloc, MIRROR_BY_SEG, 0)) { > log_error("Failed to convert pvmove LV to mirrored"); > return_NULL; > } > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, Sep 30, 2009 at 10:50:06AM -0500, Jon Brassow wrote: > As long as we are on the topic of mirror logs (and this is unrelated to > your patch), I'd like to complain about the current behavior of log > allocation vs the log policy. I don't think it should take > 'alloc_anywhere' for a log to be placed on the same disk as one of the > mirror legs... that should be 'alloc_normal'. It was assumed that performance would be better this way, but I have never seen any tests to prove or disprove that. Maybe someone could run some? One way we could handle this is by creating a new policy between NORMAL and ANYWHERE that does this, then renaming the old NORMAL to something else and the new policy to NORMAL. Alasdair -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Sep 30, 2009, at 11:35 AM, Alasdair G Kergon wrote: > On Wed, Sep 30, 2009 at 10:50:06AM -0500, Jon Brassow wrote: >> As long as we are on the topic of mirror logs (and this is >> unrelated to >> your patch), I'd like to complain about the current behavior of log >> allocation vs the log policy. I don't think it should take >> 'alloc_anywhere' for a log to be placed on the same disk as one of >> the >> mirror legs... that should be 'alloc_normal'. > > It was assumed that performance would be better this way, but I have > never seen any tests to prove or disprove that. Maybe someone could > run > some? > > One way we could handle this is by creating a new policy between > NORMAL and ANYWHERE that does this, then renaming the old NORMAL > to something else and the new policy to NORMAL. In addition to performance, I would also think that "preserving hardware" might be a minor priority. You don't want to go banging around the heads of your hard-drives if you don't have to. However, the user will get these benefits if they have 1 more device than the mirror images they are trying to create... If not, it should naturally fall back to allowing the overlap of images and logs. Inserting a new allocation level is a possibility. I don't think it's necessary to solve (what I see as) the problem. Do you have a reason for this? (Remember, the change would only affect how mirrors behave with the various allocation policies. If the user wanted the old behavior, they could get it with ALLOC_CONTIGUOUS.... However, the user would be stupid to choose that, because if they have enough disks, they will get CONTIGUOUS, and if they don't it would fail... Perhaps they would rather have the command fail then allow them to do what they are trying?) brassow -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, Sep 30, 2009 at 02:48:22PM -0500, Jon Brassow wrote: > Inserting a new allocation level is a possibility. I don't think it's > necessary to solve (what I see as) the problem. Do you have a reason > for this? It doesn't actually fit into the existing hierarchy very well at all - it's pretty much an independent property (which the existing allocation routines do not support). E.g. consider if you require 'cling' with the log on the same disk as some of the mirror's data. Falling through to 'Normal' or 'anywhere' and allowing the log to share a disk could give you a wrong layout. But you want to give priority to arrangements with the log on a different disk where possible. So do you have to try all the policies twice, first with the existing find_parallel_areas, and then with a new flag which allows some specified areas to be 'anywhere' (almost doubling the number of policies), or perhaps break it into two independent allocations at that point, one for the data and then one for the log? Alasdair -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Jonathan Brassow [jbrassow@redhat.com] wrote: > > On Sep 30, 2009, at 11:35 AM, Alasdair G Kergon wrote: > >> On Wed, Sep 30, 2009 at 10:50:06AM -0500, Jon Brassow wrote: >>> As long as we are on the topic of mirror logs (and this is unrelated to >>> your patch), I'd like to complain about the current behavior of log >>> allocation vs the log policy. I don't think it should take >>> 'alloc_anywhere' for a log to be placed on the same disk as one of the >>> mirror legs... that should be 'alloc_normal'. >> >> It was assumed that performance would be better this way, but I have >> never seen any tests to prove or disprove that. Maybe someone could run >> some? >> >> One way we could handle this is by creating a new policy between >> NORMAL and ANYWHERE that does this, then renaming the old NORMAL >> to something else and the new policy to NORMAL. > > In addition to performance, I would also think that "preserving hardware" > might be a minor priority. You don't want to go banging around the heads > of your hard-drives if you don't have to. > > However, the user will get these benefits if they have 1 more device than > the mirror images they are trying to create... If not, it should naturally > fall back to allowing the overlap of images and logs. > > Inserting a new allocation level is a possibility. I don't think it's > necessary to solve (what I see as) the problem. Do you have a reason for > this? (Remember, the change would only affect how mirrors behave with the > various allocation policies. If the user wanted the old behavior, they > could get it with ALLOC_CONTIGUOUS.... However, the user would be stupid > to choose that, because if they have enough disks, they will get > CONTIGUOUS, and if they don't it would fail... Perhaps they would rather > have the command fail then allow them to do what they are trying?) Imagine that I have lots of PVs and only two of them have free space. When I create a mirror, your proposal could put the log on a PV that has one of the mirror legs. It is silent! Had it failed, I could have extended any of the full PVs and re-ran lvcreate. I think I like the current behaviour for that reason. I would rather have a system that fail and tell me why it failed rather than do something that I don't expect! Maybe, a better failure message is an answer? If such default lvcreate failures bother someone, he can probably ask for something in lvm.conf he can set at one time to behave the way you want it. Thanks, Malahal. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Wed, Sep 30, 2009 at 10:18:26PM +0100, Alasdair G Kergon wrote: > or perhaps break it into two independent allocations at that point, > one for the data and then one for the log? Perhaps that's the solution: a new lvm.conf/cmdline parm that permits independent log placement. Allocation is initially performed the same as currently. But if it fails and this new setting was chosen, then the allocation is re-attempted without the log, and if that succeeds, an independent attempt is made to allocate the log. Alasdair -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Jonathan Brassow [jbrassow@redhat.com] wrote: > Looking through the code before applying your patch, it seems that someone > has already thought about this issue - even if it hasn't been implemented. > For example, already in the top most function used to create mirrors, > 'lv_add_mirrors', we see a 'log_count' parameter. That parameter can be > traced down through 'add_mirror_images/log', '_set_up_mirror_log', and even > the allocation functions. In fact, the first comment in 'add_mirror_log' > is /* Unimplemented features */, followed by a check to see if 'log_count > > 1'. Your patch seems to ignore 'log_count' and create new parameters (like > mirroredlog), which seem unnecessary to me... Yes, I saw the log_count but not sure if I can use that for my purpose. You can have multiple logs (reserved/standby mode implementations) without the logs themselves mirrored. > I don't understand why any > of the new parameters to the functions are necessary, can you explain? [I > can see new parameters for '_create_mirror_log' though, as it doesn't seem > to maintain the 'log_count' parameter - but you didn't do work in that > function.] _set_up_mirror_log() needs those extra parameters while calling lv_extend() in the patch. > You also seem to have violated the allocation policies by ignoring the line > of work that has been done up to '_set_up_mirror_log' by simply calling > 'add_mirror_images'. This works, but it is oversimplified, I think. You > can see this is incorrect by simply testing: > prompt> lvcreate -m1 -L 500M -n lv vg /dev/sd[xy]1 # will fail because > there are only two devices > prompt> lvcreate ... --mirroredlog /dev/sd[xy]1 # should fail, but succeeds > due to ignoring previous allocation work > You may wish to push your enhancements into '_[create | init]_mirror_log'? Thank you for spotting it. I will look into your suggestion. > Additionally, the 'log_count' parameter is more general than 'mirroredlog' > and can support more log types. Consider the following: > --mirrorlog core => (log_count = 0) > --mirrorlog disk => (log_count = 1) > --mirrorlog redundant => (log_count = 2) > --mirrorlog fully_redundant => (log_count = # of mirror legs) > You are looking to add "redundant" (you can call it "mirrored" if you > like), but if we use 'log_count' in a general way, we get "fully_redundant" > (almost) for free. The current patch actually creates fully_redundant log based what you described. If we are not going to use log_count for anything else other than creating "mirrored" logs, I can use it. I remember Heinz implementing a log device per mirror leg but the logs not not mirrored at all. Alasdair, any comments? Thanks, Malahal. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Patch
diff -r c70315774a35 lib/activate/activate.c --- a/lib/activate/activate.c Tue Sep 15 12:08:46 2009 -0700 +++ b/lib/activate/activate.c Tue Sep 22 19:51:15 2009 -0700 @@ -979,6 +979,8 @@ { struct logical_volume *lv; struct lvinfo info; + struct lv_segment *seg, *log_seg; + struct segment_type *type = get_segtype_from_string(cmd, "mirror"); int r = 0; if (!activation()) @@ -1012,6 +1014,20 @@ if (!monitor_dev_for_events(cmd, lv, 0)) stack; + if ( !dm_list_empty(&lv->segments) ) { + seg = dm_list_item(dm_list_first(&lv->segments), struct lv_segment); + if (seg->log_lv) { + log_very_verbose("lv %s is mirrored, check it's log %s...", lv->name, seg->log_lv->name); + if ( !dm_list_empty(&seg->log_lv->segments) ) { + log_seg = dm_list_item(dm_list_first(&seg->log_lv->segments), struct lv_segment); + if (log_seg->segtype == type) { + log_verbose("log %s is mirrored, unregister for events", log_seg->lv->name); + monitor_dev_for_events(cmd, log_seg->lv, 0); + } + } + } + } + memlock_inc(); r = _lv_deactivate(lv); memlock_dec(); diff -r c70315774a35 lib/metadata/lv_manip.c --- a/lib/metadata/lv_manip.c Tue Sep 15 12:08:46 2009 -0700 +++ b/lib/metadata/lv_manip.c Tue Sep 22 19:51:15 2009 -0700 @@ -3029,14 +3029,16 @@ return_0; if (lp->mirrors > 1) { - if (!lv_add_mirrors(cmd, lv, lp->mirrors - 1, lp->stripes, + if (!lv_add_mirrors(cmd, lv, lp->segtype, lp->mirrors - 1, + lp->stripes, lp->stripe_size, adjusted_mirror_region_size( vg->extent_size, lv->le_count, lp->region_size), lp->corelog ? 0U : 1U, lp->pvh, lp->alloc, MIRROR_BY_LV | - (lp->nosync ? MIRROR_SKIP_INIT_SYNC : 0))) { + (lp->nosync ? MIRROR_SKIP_INIT_SYNC : 0), + lp->mirrored_log)) { stack; goto revert_new_lv; } diff -r c70315774a35 lib/metadata/metadata-exported.h --- a/lib/metadata/metadata-exported.h Tue Sep 15 12:08:46 2009 -0700 +++ b/lib/metadata/metadata-exported.h Tue Sep 22 19:51:15 2009 -0700 @@ -528,6 +528,7 @@ uint32_t region_size; /* mirror */ uint32_t mirrors; /* mirror */ + uint32_t mirrored_log; /* mirror */ const struct segment_type *segtype; /* all */ @@ -637,9 +638,11 @@ */ struct lv_segment *find_mirror_seg(struct lv_segment *seg); int lv_add_mirrors(struct cmd_context *cmd, struct logical_volume *lv, - uint32_t mirrors, uint32_t stripes, + const struct segment_type *segtype, + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, uint32_t region_size, uint32_t log_count, - struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags); + struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags, + uint32_t mirrored_log); int lv_remove_mirrors(struct cmd_context *cmd, struct logical_volume *lv, uint32_t mirrors, uint32_t log_count, struct dm_list *pvs, uint32_t status_mask); @@ -658,16 +661,21 @@ int remove_mirror_images(struct logical_volume *lv, uint32_t num_mirrors, struct dm_list *removable_pvs, unsigned remove_log); int add_mirror_images(struct cmd_context *cmd, struct logical_volume *lv, - uint32_t mirrors, uint32_t stripes, uint32_t region_size, + const struct segment_type *segtype, + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, + uint32_t region_size, struct dm_list *allocatable_pvs, alloc_policy_t alloc, - uint32_t log_count); + uint32_t log_count, uint32_t mirrored_log); struct logical_volume *detach_mirror_log(struct lv_segment *seg); int attach_mirror_log(struct lv_segment *seg, struct logical_volume *lv); int remove_mirror_log(struct cmd_context *cmd, struct logical_volume *lv, struct dm_list *removable_pvs); int add_mirror_log(struct cmd_context *cmd, struct logical_volume *lv, + const struct segment_type *segtype, uint32_t log_count, uint32_t region_size, - struct dm_list *allocatable_pvs, alloc_policy_t alloc); + struct dm_list *allocatable_pvs, alloc_policy_t alloc, + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, + uint32_t mirrored_log); int reconfigure_mirror_images(struct lv_segment *mirrored_seg, uint32_t num_mirrors, struct dm_list *removable_pvs, unsigned remove_log); diff -r c70315774a35 lib/metadata/mirror.c --- a/lib/metadata/mirror.c Tue Sep 15 12:08:46 2009 -0700 +++ b/lib/metadata/mirror.c Tue Sep 22 19:51:15 2009 -0700 @@ -1248,7 +1248,8 @@ struct alloc_handle *ah, alloc_policy_t alloc, const char *lv_name, - const char *suffix) + const char *suffix, + uint32_t mirrored_log) { struct logical_volume *log_lv; char *log_name; @@ -1270,7 +1271,7 @@ alloc, lv->vg))) return_NULL; - if (!lv_add_log_segment(ah, log_lv)) + if (!mirrored_log && !lv_add_log_segment(ah, log_lv)) return_NULL; return log_lv; @@ -1279,10 +1280,16 @@ static struct logical_volume *_set_up_mirror_log(struct cmd_context *cmd, struct alloc_handle *ah, struct logical_volume *lv, + const struct segment_type *segtype, uint32_t log_count, - uint32_t region_size __attribute((unused)), + uint32_t region_size, alloc_policy_t alloc, - int in_sync) + int in_sync, + uint32_t mirrors, + uint32_t stripes, + uint32_t stripe_size, + struct dm_list *allocatable_pvs, + uint32_t mirrored_log) { struct logical_volume *log_lv; const char *suffix, *c; @@ -1324,11 +1331,20 @@ } if (!(log_lv = _create_mirror_log(lv, ah, alloc, - (const char *) lv_name, suffix))) { + (const char *) lv_name, suffix, mirrored_log))) { log_error("Failed to create mirror log."); return NULL; } + if (mirrored_log) { + if (!lv_extend(log_lv, segtype, stripes, stripe_size, + 1u, 1u, NULL, 0u, 0u, allocatable_pvs, alloc)) + return NULL; + + add_mirror_images(cmd, log_lv, segtype, mirrors, stripes, stripe_size, + region_size, allocatable_pvs, alloc, 0, 0); + } + if (!_init_mirror_log(cmd, log_lv, in_sync, &lv->tags, 1)) { log_error("Failed to create mirror log."); return NULL; @@ -1346,8 +1362,11 @@ } int add_mirror_log(struct cmd_context *cmd, struct logical_volume *lv, + const struct segment_type *seg_type, uint32_t log_count, uint32_t region_size, - struct dm_list *allocatable_pvs, alloc_policy_t alloc) + struct dm_list *allocatable_pvs, alloc_policy_t alloc, + uint32_t mirrors, uint32_t stripes, uint32_t stripe_size, + uint32_t mirrored_log) { struct alloc_handle *ah; const struct segment_type *segtype; @@ -1410,8 +1429,10 @@ else in_sync = 0; - if (!(log_lv = _set_up_mirror_log(cmd, ah, lv, log_count, - region_size, alloc, in_sync))) + if (!(log_lv = _set_up_mirror_log(cmd, ah, lv, seg_type, log_count, + region_size, alloc, in_sync, + mirrors, stripes, stripe_size, allocatable_pvs, + mirrored_log))) goto_out; if (!attach_mirror_log(first_seg(lv), log_lv)) @@ -1427,9 +1448,10 @@ * Convert "linear" LV to "mirror". */ int add_mirror_images(struct cmd_context *cmd, struct logical_volume *lv, - uint32_t mirrors, uint32_t stripes, uint32_t region_size, + const struct segment_type *seg_type, uint32_t mirrors, + uint32_t stripes, uint32_t stripe_size, uint32_t region_size, struct dm_list *allocatable_pvs, alloc_policy_t alloc, - uint32_t log_count) + uint32_t log_count, uint32_t mirrored_log) { struct alloc_handle *ah; const struct segment_type *segtype; @@ -1452,9 +1474,18 @@ if (!(segtype = get_segtype_from_string(cmd, "mirror"))) return_0; - ah = allocate_extents(lv->vg, NULL, segtype, - stripes, mirrors, log_count, region_size, lv->le_count, - allocatable_pvs, alloc, parallel_areas); + if (mirrored_log) + /* Allocate mirror extents for the log */ + ah = allocate_extents(lv->vg, NULL, segtype, + stripes, mirrors, 0, + region_size, 1, + allocatable_pvs, alloc, parallel_areas); + else + ah = allocate_extents(lv->vg, NULL, segtype, + stripes, mirrors, log_count, + region_size, lv->le_count, + allocatable_pvs, alloc, parallel_areas); + if (!ah) { log_error("Unable to allocate extents for mirror(s)."); return 0; @@ -1463,11 +1494,25 @@ /* * create and initialize mirror log */ - if (log_count && - !(log_lv = _set_up_mirror_log(cmd, ah, lv, log_count, region_size, - alloc, mirror_in_sync()))) { - stack; - goto out_remove_images; + if (log_count) { + if (!(log_lv = _set_up_mirror_log(cmd, ah, lv, seg_type, log_count, region_size, + alloc, mirror_in_sync(), mirrors, stripes, + stripe_size, allocatable_pvs, mirrored_log))) { + stack; + goto out_remove_images; + } + if (mirrored_log) { + /* Allocate extents for the mirror legs */ + alloc_destroy(ah); + ah = allocate_extents(lv->vg, NULL, segtype, + stripes, mirrors, 0, + region_size, lv->le_count, + allocatable_pvs, alloc, parallel_areas); + if (!ah) { + stack; + goto out_remove_images; + } + } } /* The log initialization involves vg metadata commit. @@ -1529,9 +1574,11 @@ * 'pvs' is either allocatable pvs. */ int lv_add_mirrors(struct cmd_context *cmd, struct logical_volume *lv, - uint32_t mirrors, uint32_t stripes, + const struct segment_type *seg_type, uint32_t mirrors, + uint32_t stripes, uint32_t stripe_size, uint32_t region_size, uint32_t log_count, - struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags) + struct dm_list *pvs, alloc_policy_t alloc, uint32_t flags, + uint32_t mirrored_log) { if (!mirrors && !log_count) { log_error("No conversion is requested"); @@ -1564,11 +1611,13 @@ region_size, pvs, alloc); } else if (flags & MIRROR_BY_LV) { if (!mirrors) - return add_mirror_log(cmd, lv, log_count, - region_size, pvs, alloc); - return add_mirror_images(cmd, lv, mirrors, - stripes, region_size, - pvs, alloc, log_count); + return add_mirror_log(cmd, lv, seg_type, log_count, + region_size, pvs, alloc, mirrors, + stripes, stripe_size, mirrored_log); + return add_mirror_images(cmd, lv, seg_type, mirrors, + stripes, stripe_size, region_size, + pvs, alloc, log_count, + mirrored_log); } log_error("Unsupported mirror conversion type"); diff -r c70315774a35 tools/args.h --- a/tools/args.h Tue Sep 15 12:08:46 2009 -0700 +++ b/tools/args.h Tue Sep 22 19:51:15 2009 -0700 @@ -45,6 +45,7 @@ arg(alloc_ARG, '\0', "alloc", alloc_arg, 0) arg(separator_ARG, '\0', "separator", string_arg, 0) arg(mirrorsonly_ARG, '\0', "mirrorsonly", NULL, 0) +arg(mirroredlog_ARG, '\0', "mirroredlog", NULL, 0) arg(nosync_ARG, '\0', "nosync", NULL, 0) arg(resync_ARG, '\0', "resync", NULL, 0) arg(corelog_ARG, '\0', "corelog", NULL, 0) diff -r c70315774a35 tools/commands.h --- a/tools/commands.h Tue Sep 15 12:08:46 2009 -0700 +++ b/tools/commands.h Tue Sep 22 19:51:15 2009 -0700 @@ -95,8 +95,7 @@ "Change logical volume layout", 0, "lvconvert " - "[-m|--mirrors Mirrors [{--mirrorlog {disk|core}|--corelog}]]\n" - "\t[--repair [--use-policies]]\n" + "[-m|--mirrors Mirrors [{--mirrorlog {disk|core}|--corelog|--mirroredlog}]]\n" "\t[-R|--regionsize MirrorLogRegionSize]\n" "\t[--alloc AllocationPolicy]\n" "\t[-b|--background]\n" @@ -122,7 +121,7 @@ "\tOriginalLogicalVolume[Path] SnapshotLogicalVolume[Path]\n", alloc_ARG, background_ARG, chunksize_ARG, corelog_ARG, interval_ARG, - mirrorlog_ARG, mirrors_ARG, noudevsync_ARG, regionsize_ARG, repair_ARG, + mirrorlog_ARG, mirrors_ARG, noudevsync_ARG, regionsize_ARG, repair_ARG, mirroredlog_ARG, snapshot_ARG, test_ARG, use_policies_ARG, yes_ARG, force_ARG, zero_ARG) xx(lvcreate, @@ -139,7 +138,7 @@ "\t{-l|--extents LogicalExtentsNumber |\n" "\t -L|--size LogicalVolumeSize[bBsSkKmMgGtTpPeE]}\n" "\t[-M|--persistent {y|n}] [--major major] [--minor minor]\n" - "\t[-m|--mirrors Mirrors [--nosync] [{--mirrorlog {disk|core}|--corelog}]]\n" + "\t[-m|--mirrors Mirrors [--nosync] [{--mirrorlog {disk|core}|--corelog|--mirroredlog}]]\n" "\t[-n|--name LogicalVolumeName]\n" "\t[--noudevsync]\n" "\t[-p|--permission {r|rw}]\n" @@ -173,9 +172,9 @@ "\t[-t|--test]\n" "\t[-v|--verbose]\n" "\t[--version]\n" + "\tOriginalLogicalVolume[Path] [PhysicalVolumePath...]\n\n", - "\t[PhysicalVolumePath...]\n\n", - + mirroredlog_ARG, addtag_ARG, alloc_ARG, autobackup_ARG, chunksize_ARG, contiguous_ARG, corelog_ARG, extents_ARG, major_ARG, minor_ARG, mirrorlog_ARG, mirrors_ARG, name_ARG, nosync_ARG, noudevsync_ARG, permission_ARG, persistent_ARG, diff -r c70315774a35 tools/lvconvert.c --- a/tools/lvconvert.c Tue Sep 15 12:08:46 2009 -0700 +++ b/tools/lvconvert.c Tue Sep 22 19:51:15 2009 -0700 @@ -31,6 +31,7 @@ uint32_t region_size; uint32_t mirrors; + uint32_t mirrored_log; sign_t mirrors_sign; struct segment_type *segtype; @@ -507,13 +508,16 @@ int corelog) { struct logical_volume *original_lv = _original_lv(lv); + if (_using_corelog(lv) && !corelog) { - if (!add_mirror_log(cmd, original_lv, 1, + if (!add_mirror_log(cmd, original_lv, lp->segtype, 1, adjusted_mirror_region_size( lv->vg->extent_size, lv->le_count, lp->region_size), - lp->pvh, lp->alloc)) + lp->pvh, lp->alloc, + lp->mirrors, 0, 0, + lp->mirrored_log)) return_0; } else if (!_using_corelog(lv) && corelog) { if (!remove_mirror_log(cmd, original_lv, @@ -538,6 +542,8 @@ int repair = arg_count(cmd, repair_ARG); int replace_log = 1, replace_mirrors = 1; + lp->mirrored_log = arg_count(cmd, mirroredlog_ARG); + seg = first_seg(lv); existing_mirrors = lv_mirror_count(lv); @@ -696,13 +702,13 @@ * currently taken by the mirror? Would make more sense from * user perspective. */ - if (!lv_add_mirrors(cmd, lv, lp->mirrors - 1, 1, + if (!lv_add_mirrors(cmd, lv, lp->segtype, lp->mirrors - 1, 1, 0, adjusted_mirror_region_size( lv->vg->extent_size, lv->le_count, lp->region_size), corelog ? 0U : 1U, lp->pvh, lp->alloc, - MIRROR_BY_LV)) + MIRROR_BY_LV, lp->mirrored_log)) return_0; if (lp->wait_completion) lp->need_polling = 1; @@ -726,13 +732,13 @@ return 0; } /* FIXME: can't have multiple mlogs. force corelog. */ - if (!lv_add_mirrors(cmd, lv, lp->mirrors - existing_mirrors, 1, + if (!lv_add_mirrors(cmd, lv, lp->segtype, lp->mirrors - existing_mirrors, 1, 0, adjusted_mirror_region_size( lv->vg->extent_size, lv->le_count, lp->region_size), 0U, lp->pvh, lp->alloc, - MIRROR_BY_LV)) + MIRROR_BY_LV, lp->mirrored_log)) return_0; lv->status |= CONVERTING; lp->need_polling = 1; @@ -743,6 +749,7 @@ if (!_lv_update_log_type(cmd, lp, lv, corelog)) return_0; } else { + /*malahal this needs rework...*/ log_error("Logical volume %s already has %" PRIu32 " mirror(s).", lv->name, lp->mirrors - 1); diff -r c70315774a35 tools/lvcreate.c --- a/tools/lvcreate.c Tue Sep 15 12:08:46 2009 -0700 +++ b/tools/lvcreate.c Tue Sep 22 19:51:15 2009 -0700 @@ -367,6 +367,9 @@ lp->region_size = region_size; } + if (arg_count(cmd, mirroredlog_ARG)) + lp->mirrored_log = 1; + if (!_validate_mirror_params(cmd, lp)) return 0; diff -r c70315774a35 tools/pvmove.c --- a/tools/pvmove.c Tue Sep 15 12:08:46 2009 -0700 +++ b/tools/pvmove.c Tue Sep 22 19:51:15 2009 -0700 @@ -244,8 +244,8 @@ return NULL; } - if (!lv_add_mirrors(cmd, lv_mirr, 1, 1, 0, log_count, - allocatable_pvs, alloc, MIRROR_BY_SEG)) { + if (!lv_add_mirrors(cmd, lv_mirr, NULL, 1u, 1u, 0u, 0u, log_count, + allocatable_pvs, alloc, MIRROR_BY_SEG, 0)) { log_error("Failed to convert pvmove LV to mirrored"); return_NULL; }