* [PAYCH] Don't allow resizing of internal logical volumes
@ 2010-03-18 23:53 Mikulas Patocka
2010-03-19 20:08 ` Mike Snitzer
0 siblings, 1 reply; 6+ messages in thread
From: Mikulas Patocka @ 2010-03-18 23:53 UTC (permalink / raw)
To: lvm-devel
Hi
This patch prevents resizing of internal volumes.
BTW.: do you think that *-shared should be treated as a reseved logical
volume name?
It is really the internal volume name, but the potential problem with it
is that there may be some systems with existing names *-shared, and
reserving the name breaks those systems.
Mikulas
---
Don't allow resizing of internal logical volumes.
This patch prevents lvresize from being able to resize internal LVs: mirror
legs (*_mimage_*), mirror log (*_mlog), snapshot placeholder LVs (snapshot*)
and others. Resizing these would leads to unexpected metadata and sometimes
crashes (in case of growing snapshot*).
Note that test for VISIBLE_LV is not sufficient because snapshot0 volumes
have the VISIBLE_LV flag set. So we test the name instead of the flags.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
---
tools/lvresize.c | 5 +++++
1 file changed, 5 insertions(+)
Index: LVM2/tools/lvresize.c
===================================================================
--- LVM2.orig/tools/lvresize.c 2010-03-19 00:25:31.000000000 +0100
+++ LVM2/tools/lvresize.c 2010-03-19 00:44:52.000000000 +0100
@@ -323,6 +323,11 @@ static int _lvresize(struct cmd_context
lv = lvl->lv;
+ if (is_reserved_lvname(lv->name)) {
+ log_error("Can't resize internal logical volume %s", lv->name);
+ return ECMD_FAILED;
+ }
+
if (lv->status & LOCKED) {
log_error("Can't resize locked LV %s", lv->name);
return ECMD_FAILED;
^ permalink raw reply [flat|nested] 6+ messages in thread* Don't allow resizing of internal logical volumes 2010-03-18 23:53 [PAYCH] Don't allow resizing of internal logical volumes Mikulas Patocka @ 2010-03-19 20:08 ` Mike Snitzer 2010-03-19 21:06 ` Alasdair G Kergon 2010-03-19 22:27 ` Mikulas Patocka 0 siblings, 2 replies; 6+ messages in thread From: Mike Snitzer @ 2010-03-19 20:08 UTC (permalink / raw) To: lvm-devel On Thu, Mar 18 2010 at 7:53pm -0400, Mikulas Patocka <mpatocka@redhat.com> wrote: > Hi > > This patch prevents resizing of internal volumes. > > BTW.: do you think that *-shared should be treated as a reseved logical > volume name? > It is really the internal volume name, but the potential problem with it > is that there may be some systems with existing names *-shared, and > reserving the name breaks those systems. > > Mikulas > > --- > > Don't allow resizing of internal logical volumes. > > This patch prevents lvresize from being able to resize internal LVs: mirror > legs (*_mimage_*), mirror log (*_mlog), snapshot placeholder LVs (snapshot*) > and others. Resizing these would leads to unexpected metadata and sometimes > crashes (in case of growing snapshot*). > > Note that test for VISIBLE_LV is not sufficient because snapshot0 volumes > have the VISIBLE_LV flag set. So we test the name instead of the flags. OK, but snapshotX volumes are part of the LVM2 snapshot "fiction". They never get exposed to the end user (even with lvs -a). Only the user facing snapshot LV name gets exposed: # lvs test/snapshot0 One or more specified logical volume(s) not found. # lvs test/testlv1_snap LV VG Attr LSize Origin Snap% Move Log Copy% Convert testlv1_snap test swi-a- 4.00g testlv1 As such we shouldn't have to worry about the user attempting to resize the hidden snapshotX volume. Am I still missing something? That said the LVM2 snapshot "fiction" is really unfortunate. Keeps getting in the way (mostly a mental barrier for me; quite unnatural). > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > --- > tools/lvresize.c | 5 +++++ > 1 file changed, 5 insertions(+) > > Index: LVM2/tools/lvresize.c > =================================================================== > --- LVM2.orig/tools/lvresize.c 2010-03-19 00:25:31.000000000 +0100 > +++ LVM2/tools/lvresize.c 2010-03-19 00:44:52.000000000 +0100 > @@ -323,6 +323,11 @@ static int _lvresize(struct cmd_context > > lv = lvl->lv; > > + if (is_reserved_lvname(lv->name)) { > + log_error("Can't resize internal logical volume %s", lv->name); > + return ECMD_FAILED; > + } > + > if (lv->status & LOCKED) { > log_error("Can't resize locked LV %s", lv->name); > return ECMD_FAILED; Given my recent ACCESS_HIDDEN_LV patch here: http://people.redhat.com/msnitzer/patches/multisnap/lvm2/LVM2-2.02.63/lvm-shared-add-ACCESS_HIDDEN_LV-flag.patch I was thinking we'd just have something like this: diff --git a/tools/lvresize.c b/tools/lvresize.c index 849fd2e..f08be60 100644 --- a/tools/lvresize.c +++ b/tools/lvresize.c @@ -323,6 +323,11 @@ static int _lvresize(struct cmd_context *cmd, struct volume_group *vg, lv = lvl->lv; + if (!lv_is_visible(lv) && !lv_is_accessible_hidden(lv)) { + log_error("Can't resize internal logical volume %s", lv->name); + return ECMD_FAILED; + } + if (lv->status & LOCKED) { log_error("Can't resize locked LV %s", lv->name); return ECMD_FAILED; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Don't allow resizing of internal logical volumes 2010-03-19 20:08 ` Mike Snitzer @ 2010-03-19 21:06 ` Alasdair G Kergon 2010-03-19 22:27 ` Mikulas Patocka 1 sibling, 0 replies; 6+ messages in thread From: Alasdair G Kergon @ 2010-03-19 21:06 UTC (permalink / raw) To: lvm-devel > > Note that test for VISIBLE_LV is not sufficient because snapshot0 volumes > > have the VISIBLE_LV flag set. So we test the name instead of the flags. We only test against flags. The code should be the same as other code in toollib. (We do actually allow LVs with reserved names to be changed if the use of the reserved name pre-dates the introduction of the restriction. The reserved names validation is only used on *newly-supplied* LV names, not pre-existing ones. Well that's how it was meant to work, anyway...) Alasdair ^ permalink raw reply [flat|nested] 6+ messages in thread
* Don't allow resizing of internal logical volumes 2010-03-19 20:08 ` Mike Snitzer 2010-03-19 21:06 ` Alasdair G Kergon @ 2010-03-19 22:27 ` Mikulas Patocka 2010-03-19 23:44 ` Mike Snitzer 1 sibling, 1 reply; 6+ messages in thread From: Mikulas Patocka @ 2010-03-19 22:27 UTC (permalink / raw) To: lvm-devel On Fri, 19 Mar 2010, Mike Snitzer wrote: > On Thu, Mar 18 2010 at 7:53pm -0400, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > Hi > > > > This patch prevents resizing of internal volumes. > > > > BTW.: do you think that *-shared should be treated as a reseved logical > > volume name? > > It is really the internal volume name, but the potential problem with it > > is that there may be some systems with existing names *-shared, and > > reserving the name breaks those systems. > > > > Mikulas > > > > --- > > > > Don't allow resizing of internal logical volumes. > > > > This patch prevents lvresize from being able to resize internal LVs: mirror > > legs (*_mimage_*), mirror log (*_mlog), snapshot placeholder LVs (snapshot*) > > and others. Resizing these would leads to unexpected metadata and sometimes > > crashes (in case of growing snapshot*). > > > > Note that test for VISIBLE_LV is not sufficient because snapshot0 volumes > > have the VISIBLE_LV flag set. So we test the name instead of the flags. > > OK, but snapshotX volumes are part of the LVM2 snapshot "fiction". They > never get exposed to the end user (even with lvs -a). Only the user > facing snapshot LV name gets exposed: > # lvs test/snapshot0 > One or more specified logical volume(s) not found. > # lvs test/testlv1_snap > LV VG Attr LSize Origin Snap% Move Log Copy% Convert > testlv1_snap test swi-a- 4.00g testlv1 > > As such we shouldn't have to worry about the user attempting to resize > the hidden snapshotX volume. Am I still missing something? If you do lvresize vgs/snapshot0, it allows it. If you extend it, it crashes. Try it. snapshotX isn't visible enen with "a", but for lvresize it exists. > That said the LVM2 snapshot "fiction" is really unfortunate. Keeps > getting in the way (mostly a mental barrier for me; quite unnatural). I don't like it too. At least, if it were named "snapshot-cow" and "snapshot" to be consistent with dm device names... > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > > > --- > > tools/lvresize.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > Index: LVM2/tools/lvresize.c > > =================================================================== > > --- LVM2.orig/tools/lvresize.c 2010-03-19 00:25:31.000000000 +0100 > > +++ LVM2/tools/lvresize.c 2010-03-19 00:44:52.000000000 +0100 > > @@ -323,6 +323,11 @@ static int _lvresize(struct cmd_context > > > > lv = lvl->lv; > > > > + if (is_reserved_lvname(lv->name)) { > > + log_error("Can't resize internal logical volume %s", lv->name); > > + return ECMD_FAILED; > > + } > > + > > if (lv->status & LOCKED) { > > log_error("Can't resize locked LV %s", lv->name); > > return ECMD_FAILED; > > Given my recent ACCESS_HIDDEN_LV patch here: > http://people.redhat.com/msnitzer/patches/multisnap/lvm2/LVM2-2.02.63/lvm-shared-add-ACCESS_HIDDEN_LV-flag.patch > > I was thinking we'd just have something like this: > > diff --git a/tools/lvresize.c b/tools/lvresize.c > index 849fd2e..f08be60 100644 > --- a/tools/lvresize.c > +++ b/tools/lvresize.c > @@ -323,6 +323,11 @@ static int _lvresize(struct cmd_context *cmd, struct volume_group *vg, > > lv = lvl->lv; > > + if (!lv_is_visible(lv) && !lv_is_accessible_hidden(lv)) { > + log_error("Can't resize internal logical volume %s", lv->name); > + return ECMD_FAILED; > + } > + > if (lv->status & LOCKED) { > log_error("Can't resize locked LV %s", lv->name); > return ECMD_FAILED; > OK. It is cleaner than my patch that checks the name, so commit it (except !lv_is_accessible_hidden(lv), because that has to wait until we commit shared snapshots). I tested it and it refuses "snapshotX" volumes. Mikulas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Don't allow resizing of internal logical volumes 2010-03-19 22:27 ` Mikulas Patocka @ 2010-03-19 23:44 ` Mike Snitzer 2010-03-20 2:04 ` Mike Snitzer 0 siblings, 1 reply; 6+ messages in thread From: Mike Snitzer @ 2010-03-19 23:44 UTC (permalink / raw) To: lvm-devel On Fri, Mar 19 2010 at 6:27pm -0400, Mikulas Patocka <mpatocka@redhat.com> wrote: > > > On Fri, 19 Mar 2010, Mike Snitzer wrote: > > > On Thu, Mar 18 2010 at 7:53pm -0400, > > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > Don't allow resizing of internal logical volumes. > > > > > > This patch prevents lvresize from being able to resize internal LVs: mirror > > > legs (*_mimage_*), mirror log (*_mlog), snapshot placeholder LVs (snapshot*) > > > and others. Resizing these would leads to unexpected metadata and sometimes > > > crashes (in case of growing snapshot*). > > > > > > Note that test for VISIBLE_LV is not sufficient because snapshot0 volumes > > > have the VISIBLE_LV flag set. So we test the name instead of the flags. > > > > OK, but snapshotX volumes are part of the LVM2 snapshot "fiction". They > > never get exposed to the end user (even with lvs -a). Only the user > > facing snapshot LV name gets exposed: > > # lvs test/snapshot0 > > One or more specified logical volume(s) not found. > > # lvs test/testlv1_snap > > LV VG Attr LSize Origin Snap% Move Log Copy% Convert > > testlv1_snap test swi-a- 4.00g testlv1 > > > > As such we shouldn't have to worry about the user attempting to resize > > the hidden snapshotX volume. Am I still missing something? > > If you do lvresize vgs/snapshot0, it allows it. If you extend it, it > crashes. Try it. snapshotX isn't visible enen with "a", but for lvresize > it exists. I didn't realize that, OK. > > diff --git a/tools/lvresize.c b/tools/lvresize.c > > index 849fd2e..f08be60 100644 > > --- a/tools/lvresize.c > > +++ b/tools/lvresize.c > > @@ -323,6 +323,11 @@ static int _lvresize(struct cmd_context *cmd, struct volume_group *vg, > > > > lv = lvl->lv; > > > > + if (!lv_is_visible(lv) && !lv_is_accessible_hidden(lv)) { > > + log_error("Can't resize internal logical volume %s", lv->name); > > + return ECMD_FAILED; > > + } > > + > > if (lv->status & LOCKED) { > > log_error("Can't resize locked LV %s", lv->name); > > return ECMD_FAILED; > > > > OK. It is cleaner than my patch that checks the name, so commit it (except > !lv_is_accessible_hidden(lv), because that has to wait until we commit > shared snapshots). > > I tested it and it refuses "snapshotX" volumes. But as you said above: "snapshot0 volumes have the VISIBLE_LV flag set." That said, I do see that in practice the existing lvm2 code does reject resizing "snapshot" type snapshot0 using !lv_is_visible(lv). I'll have a closer look to understand that. But when I tried it with the "multisnapshot" type snapshot0 (w/ multisnap enabled LVM2 2.02.63 from my people page + the above patch) lvresize allowed it and then crashed in _setup_alloced_segment(): # lvresize -L 5.0G test/snapshot0 Extending logical volume snapshot0 to 5.00 GiB Segmentation fault (core dumped) (gdb) bt #0 0x00000000004520ea in _setup_alloced_segment (region_size=<value optimized out>, aa=<value optimized out>, segtype=<value optimized out>, stripe_size=<value optimized out>, area_count=<value optimized out>, status=<value optimized out>, lv=<value optimized out>) at metadata/lv_manip.c:690 #1 _setup_alloced_segments (region_size=<value optimized out>, aa=<value optimized out>, segtype=<value optimized out>, stripe_size=<value optimized out>, area_count=<value optimized out>, status=<value optimized out>, lv=<value optimized out>) at metadata/lv_manip.c:727 #2 lv_add_segment (region_size=<value optimized out>, aa=<value optimized out>, segtype=<value optimized out>, stripe_size=<value optimized out>, area_count=<value optimized out>, status=<value optimized out>, lv=<value optimized out>) at metadata/lv_manip.c:1387 #3 0x000000000045498a in lv_extend (lv=0xe66878, segtype=0xe40770, stripes=<value optimized out>, stripe_size=<value optimized out>, mirrors=0, extents=256, mirrored_pv=<value optimized out>, mirrored_pe=<value optimized out>, status=<value optimized out>, allocatable_pvs=<value optimized out>, alloc=<value optimized out>) at metadata/lv_manip.c:1638 #4 0x000000000041d9ea in _lvresize (lp=<value optimized out>, vg=<value optimized out>, cmd=<value optimized out>) at lvresize.c:629 #5 lvresize (lp=<value optimized out>, vg=<value optimized out>, cmd=<value optimized out>) at lvresize.c:702 #6 0x0000000000418550 in lvm_run_command (cmd=0xdd5fe0, argc=1, argv=0x7fff3bc2c1a8) at lvmcmdline.c:1071 #7 0x000000000041ab20 in lvm2_main (argc=4, argv=0x7fff3bc2c190) at lvmcmdline.c:1423 #8 0x000000335341eb1d in __libc_start_main () from /lib64/libc.so.6 #9 0x000000000040f089 in _start () (gdb) l 685 uint32_t s, extents, area_multiple; 686 struct lv_segment *seg; 687 688 area_multiple = calc_area_multiple(segtype, area_count); 689 690 if (!(seg = alloc_lv_segment(lv->vg->cmd->mem, segtype, lv, 691 lv->le_count, 692 aa[0].len * area_multiple, 693 status, stripe_size, NULL, 694 area_count, Mike ^ permalink raw reply [flat|nested] 6+ messages in thread
* Don't allow resizing of internal logical volumes 2010-03-19 23:44 ` Mike Snitzer @ 2010-03-20 2:04 ` Mike Snitzer 0 siblings, 0 replies; 6+ messages in thread From: Mike Snitzer @ 2010-03-20 2:04 UTC (permalink / raw) To: lvm-devel On Fri, Mar 19 2010 at 7:44pm -0400, Mike Snitzer <snitzer@redhat.com> wrote: > On Fri, Mar 19 2010 at 6:27pm -0400, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > OK. It is cleaner than my patch that checks the name, so commit it (except > > !lv_is_accessible_hidden(lv), because that has to wait until we commit > > shared snapshots). > > > > I tested it and it refuses "snapshotX" volumes. > > But as you said above: "snapshot0 volumes have the VISIBLE_LV flag set." > > That said, I do see that in practice the existing lvm2 code does reject > resizing "snapshot" type snapshot0 using !lv_is_visible(lv). I'll have > a closer look to understand that. lv_is_visible() prevents resize on "snapshot" type snapshot0 because of this out: (lv->status & SNAPSHOT) As you suggested, I'll commit the lv_is_visible() change to tools/lvresize.c > But when I tried it with the "multisnapshot" type snapshot0 (w/ > multisnap enabled LVM2 2.02.63 from my people page + the above patch) > lvresize allowed it and then crashed in _setup_alloced_segment(): I've added the same out in lv_is_accessible_hidden() and this prevents the "multisnapshot" type snapshot0 from being resized: int lv_is_accessible_hidden(const struct logical_volume *lv) { if (lv->status & SNAPSHOT) return 0; if (lv_is_cow(lv)) return (find_cow(lv)->lv->status & ACCESS_HIDDEN_LV) ? 1 : 0; return (lv->status & ACCESS_HIDDEN_LV) ? 1 : 0; } ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-03-20 2:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-03-18 23:53 [PAYCH] Don't allow resizing of internal logical volumes Mikulas Patocka 2010-03-19 20:08 ` Mike Snitzer 2010-03-19 21:06 ` Alasdair G Kergon 2010-03-19 22:27 ` Mikulas Patocka 2010-03-19 23:44 ` Mike Snitzer 2010-03-20 2:04 ` Mike Snitzer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.