pvresize patch pending

A friend and I have been working on a patch to provide pvresize to the lvm2 distribution. We have a working version now and are looking for some code review/testing. Currently it only supports growing a pv and it only supports growing it to the actual size of the device that backs the pv.

Is this the right venue to distribute this patch or is there another way that it should be disseminated?

Thank you,
Zac Slade
[email protected]

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

Yes!

Alasdair

[email protected]

Quoting Alasdair G Kergon <[email protected]>:

Well alright. Attached is a unified diff I validated that it merges successfully against LVM2.2.01.13.tar.gz and LVM2.2.01.14.tar.gz. Those files I obtained through gentoo's portage system. I also checked out the cvs and the patch does apply successfully. To integrate the patch just enter the distribution directory where the LVM2 sources are and execute, patch -p1 < path-to.patch.

For testing we used an lv that was mounted on a loopback device. The only option that pvresize will take is a pv pathname.

Here's your patch!
Zac Slade
[email protected]

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

Quoting Alasdair G Kergon <[email protected]>:

Opps. See previous in thread for patching:

diff -uNdr LVM2.2.01.13/lib/format_text/format-text.c LVM2.2.01.13.patched/lib/format_text/format-text.c
--- LVM2.2.01.13/lib/format_text/format-text.c 2005-10-28 06:54:22.000000000 -0500 +++ LVM2.2.01.13.patched/lib/format_text/format-text.c 2005-10-28 06:55:13.000000000 -0500
@@ -1059,7 +1059,7 @@

        /* FIXME Test mode don't update cache? */

        if (!(info = lvmcache_add(fmt->labeller, (char *) &pv->id, pv->dev,
-                                 ORPHAN, NULL))) {
+                                 pv->vg_name, NULL))) {
                stack;
                return 0;
        }

diff -uNdr LVM2.2.01.13/tools/Makefile.in LVM2.2.01.13.patched/tools/Makefile.in
@@ -39,6 +39,7 @@
        polldaemon.c \
        pvchange.c \
        pvcreate.c \
+       pvresize.c \
        pvdisplay.c \
        pvmove.c \
        pvremove.c \

diff -uNdr LVM2.2.01.13/tools/commands.h LVM2.2.01.13.patched/tools/commands.h
@@ -486,8 +487,9 @@
    force_ARG, test_ARG, yes_ARG)
 
 xx(pvresize,
-   "Resize a physical volume in use by a volume group",
-   "Not implemented.  Use pvcreate options.",
+   "Resize (grow only) a physical volume",
+   "pvresize " "\n"
+   "\tPhysicalVolume\n",
 /***
    "pvresize "
    "[-A|--autobackup {y|n}] "

Files LVM2.2.01.13/tools/lvm.o and LVM2.2.01.13.patched/tools/lvm.o differ

diff -uNdr LVM2.2.01.13/tools/stub.h LVM2.2.01.13.patched/tools/stub.h
@@ -19,7 +19,7 @@
 /*int e2fsadm(struct cmd_context *cmd, int argc, char **argv) unimplemented*/
 int lvmsadc(struct cmd_context *cmd, int argc, char **argv) unimplemented
 int lvmsar(struct cmd_context *cmd, int argc, char **argv) unimplemented
-int pvresize(struct cmd_context *cmd, int argc, char **argv) unimplemented
+/*int pvresize(struct cmd_context *cmd, int argc, char **argv) unimplemented*/
 
 int pvdata(struct cmd_context *cmd, int argc, char **argv) {
        log_error("There's no 'pvdata' command in LVM2.");

Zac Slade
[email protected]

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

If the PV belongs to a VG (the only case that matters - pvcreate is quite adequate otherwise), after the pv_write (equivalent to 'pvcreate'), more fields need updating I think - see add_pv_to_vg() - incl. the various extent counters and the segment lists.

[If you only allow 'pvresize' with format_text this might be easier - compatibility with LVM1 size calculations and restrictions is always tricky and needs careful testing - is it really worth the effort?]

Try process_each_pv(), which also handles the locking for you.

For shrinking, you need to ensure that all the extents that are about to be removed are unallocated - if they aren't, then the metadata should be considered corrupt and it's best to refuse to change it. [Separate recovery options needed.]

Alasdair

[email protected]

Indeed. I've recently been working on an implementation myself (primarily because I wanted to grow a live PV on my own system), which I've attached in its current (somewhat rough, but seemingly working) state as a patch against CVS HEAD.

In addition to updating pv->size it also adjusts pv->pe_count, pe->segments, vg->extent_count and vg->free_count.

In the shrinking case, it checks that the removed extents are free (though ideally it would relocate them elsewhere in the VG, like pvmove.

There is no requirement for LVs to be deactivated, as the actual LVs don't get touched during the operation.

I've tested it (both growing and shrinking) on loop devices containing small LVM systems, and successfully used it to grow the PV housing a running system (including the root filesystem).

The main issues I'm not sure about are:

* Support for metadata formats other than format_text.

* Support for more than one mda on a PV (I think it will currently clobber ones at the end of the PV, since it assumes all space after pe_start is available for extents).

I hadn't planned on posting it quite yet, but since the discussion has started it might be useful.

Zak Kipling.

Index: lib/metadata/pv_alloc.h
===================================================================
RCS file: /cvs/lvm2/LVM2/lib/metadata/pv_alloc.h,v retrieving revision 1.5

diff -b -u -r1.5 pv_alloc.h
@@ -25,4 +25,8 @@
 int check_pv_segments(struct volume_group *vg);
 void merge_pv_segments(struct pv_segment *peg1, struct pv_segment *peg2);
 
+/* This code is experimental. Do NOT complain if it trashes your LVM system */
+int pv_resize(struct physical_volume *pv, struct volume_group *vg,
+	      uint32_t new_pe_count);
+
 #endif

Index: lib/metadata/pv_manip.c
===================================================================
RCS file: /cvs/lvm2/LVM2/lib/metadata/pv_manip.c,v retrieving revision 1.8

Index: man/Makefile.in
===================================================================
RCS file: /cvs/lvm2/LVM2/man/Makefile.in,v
retrieving revision 1.14

diff -b -u -r1.14 Makefile.in
@@ -19,10 +19,10 @@
 MAN5=lvm.conf.5
 MAN8=lvchange.8 lvcreate.8 lvdisplay.8 lvextend.8 lvm.8 lvmchange.8 \
 	lvmdiskscan.8 lvreduce.8 lvremove.8 lvrename.8 lvresize.8 lvs.8 \
-	lvscan.8 pvchange.8 pvcreate.8 pvdisplay.8 pvmove.8 pvremove.8 pvs.8 \
-	pvscan.8 vgcfgbackup.8 vgcfgrestore.8 vgchange.8 vgck.8 vgcreate.8 \
-	vgconvert.8 vgdisplay.8 vgexport.8 vgextend.8 vgimport.8 \
-	vgmerge.8 vgmknodes.8 vgreduce.8 vgremove.8 vgrename.8 \
+	lvscan.8 pvchange.8 pvcreate.8 pvdisplay.8 pvmove.8 pvremove.8 \
+	pvresize.8 pvs.8 pvscan.8 vgcfgbackup.8 vgcfgrestore.8 vgchange.8 \
+	vgck.8 vgcreate.8 vgconvert.8 vgdisplay.8 vgexport.8 vgextend.8 \
+	vgimport.8 vgmerge.8 vgmknodes.8 vgreduce.8 vgremove.8 vgrename.8 \
 	vgs.8 vgscan.8 vgsplit.8
 MAN8CLUSTER=clvmd.8
 MAN5DIR=${mandir}/man5

Index: man/pvresize.8
===================================================================
RCS file: man/pvresize.8

Index: tools/Makefile.in
===================================================================
RCS file: /cvs/lvm2/LVM2/tools/Makefile.in,v
retrieving revision 1.68

diff -b -u -r1.68 Makefile.in
@@ -42,6 +42,7 @@
 	pvdisplay.c \
 	pvmove.c \
 	pvremove.c \
+	pvresize.c \
 	pvscan.c \
 	reporter.c \
 	segtypes.c \

Index: tools/commands.h
===================================================================
RCS file: /cvs/lvm2/LVM2/tools/commands.h,v
retrieving revision 1.75

diff -b -u -r1.75 commands.h
@@ -373,6 +373,22 @@
    all_ARG, allocatable_ARG, allocation_ARG, autobackup_ARG, deltag_ARG,
    addtag_ARG, test_ARG, uuid_ARG)
 
+xx(pvresize,
+   "Resize physical volume(s) in use by LVM [EXPERIMENTAL]",
+   "pvresize " "\n"
+   "\t[-d|--debug]" "\n"
+   "\t[-h|-?|--help] " "\n"
+   "\t[--setphysicalvolumesize PhysicalVolumeSize[kKmMgGtT]" "\n"
+   "\t[-t|--test] " "\n"
+   "\t[-v|--verbose] " "\n"
+   "\t[--version] " "\n"
+   "\tPhysicalVolume [PhysicalVolume...]\n"
+   "\t\n"
+   "\tpvresize is EXPERIMENTAL -- ensure you have a backup and\n"
+   "\tdo NOT complain if it trashes your LVM system!\n",
+
+   test_ARG, physicalvolumesize_ARG)
+
 xx(pvcreate,
    "Initialize physical volume(s) for use by LVM",
    "pvcreate " "\n"

Index: tools/pvresize.c
===================================================================
RCS file: tools/pvresize.c

Index: tools/stub.h
===================================================================
RCS file: /cvs/lvm2/LVM2/tools/stub.h,v
retrieving revision 1.43

diff -b -u -r1.43 stub.h
@@ -19,7 +19,6 @@
 /*int e2fsadm(struct cmd_context *cmd, int argc, char **argv) unimplemented*/
 int lvmsadc(struct cmd_context *cmd, int argc, char **argv) unimplemented
 int lvmsar(struct cmd_context *cmd, int argc, char **argv) unimplemented
-int pvresize(struct cmd_context *cmd, int argc, char **argv) unimplemented
 
 int pvdata(struct cmd_context *cmd, int argc, char **argv) {
 	log_error("There's no 'pvdata' command in LVM2.");

Unless I've missed something while skimming through, I think the code still needs a preceding 'pvcreate --restorefile' equivalent to ensure the size of the device is set correctly in the label (for PVs in a VG). (But since the 'Reinstate full PV size when removing from VG.' fix, perhaps a code audit would show that we can safely override that label field with the one taken from the VG and allow it to remain wrong on disk.)

https://www.redhat.com/archives/linux-lvm/2005-July/msg00033.html
https://www.redhat.com/archives/linux-lvm/2005-July/msg00029.html
pvs -o +dev_size

Apart from that and the process_each_pv() simplification, it looks pretty good!

Let's just exclude them with a FMT_RESIZE_PV feature flag.

Check the number of mdas on the PV and refuse to proceed if it exceeds 1. We need to write separate new tools to add/remove/manipulate mdas.

Alasdair

[email protected]

Quoting Alasdair G Kergon <[email protected]>:

I'm going to work on integrating his patch with ours and see if we can't make it more cohesive.

I think that to get this part write might take a fundemental shift in the way pv_write() and _pv_write() behave. What would be nice is to make pv_write() distinguish between a pv_create calling it and a pv_resize. Then have _pv_write only write the pv mda to the disk. By doing this _pv_write becomes generic and allows for further extension. Also then we can probably implement atomicity for pv updates (though updating a pv was not initially engineered into lvm it can become very handy).

Actually there is no need to use process_each_pv(). I don't think that it's necessary or even wise to support resizing more than one pv at a time. Instead it should act less like pvcreate and more like lvextend. That way supporting --setphysicalvolumesize makes sense.

Well it would be nice to support format_pool I'm not sure what the win for supporting format1 is.... What would be required architecurally to support alternative formats?

Can't we just borrow the code from pvcreate to manipulate the metadatacopies? This shouldn't be the end of the world to get working while the infrastructure gets put into place to support it more gracefully.

Zac Slade
[email protected]

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

I'm reluctant to extend pv_write - long term I want to get rid of it (and pv_read also) as PV operations are always awkward and getting in the way. Everything should be done with (new-style) VGs and LVs.

pvcreate is also superfluous - I want to absorb it into vgcreate/vgextend etc. So every labelled volume will always belong to a VG - with enhanced vgsplit/vgmerge & allocation facilities.
CVS now lets you create PVs on LVs - another step towards eliminating PVs.

If you don't use it, you have to duplicate the logic in it as you have already done - incompletely (e.g. tag support is missing).

If ppl want to do that, why stop them?

Not yet. That problem needs solving as part of a general mda-manipulating feature. No point in doing it first for a rarely-used case.

Alasdair

[email protected]

Are any of you are interested in working on something else in a related area of the code?

New tool:
lvimport - Bring an existing filesystem on a partition under LVM control

e.g. you have an ext3 filesystem on /dev/hda3 and you want to turn it into an LV without moving it.

Requires an existing VG with at least one pre-existing mda.
(could be from metadata/dirs of course)
Involves (equivalent of) running pvcreate -Zn --metadatacopies 0 on the partition,
probably with --labelsector 1 (may depend on fs), changing pe_start to 0,
getting the last sector used by the filesystem (add a new helper function to fsadm to give this info based on filesystem type?), vgchange --physicalextentsize if necessary so that the entire filesystem is enclosed by PEs,
adding the new PV to the VG,
creating an LV enclosing the filesystem.

Now the PV label is within the new LV - but stored in a sector that the filesystem doesn't use.

There might be a case for reducing the size of the filesystem slightly before doing this if it's close to the end of the partition, so that the pe size isn't too small.
[The number of PEs is 32-bits, so tiny PEs put constraints on the VG size - need to make sure the code detects problems here, or consider implications of switching to 64 bits.]

Ensure none of the tools can leave you with a VG with no mdas (eg split off the new PV with vgsplit)

Require filesystem to be unmounted first.

Alasdair

[email protected]

Quoting Alasdair G Kergon <[email protected]>:

So instead of using pv_read and pv_write (except for backward compatibility) this functionality should then be migrated into vgcreate? Okay, I can see that.
Doing away with the pv* methods will subtract some abstraction. However with
this fundamental shift how do you reconcile the pv/vg structures? Just do away with the notion of pvs all together or instead have them, but they only exists as an internal structure not visible to the user?

I'm still learning the codebase and I'm not sure there is an obvious way to start this. I'm assuming that you don't want pvcreate reinvented inside vgcreate, but instead have vgcreate do something new to abstract the underlying devices by only utilizing a vg structure not having seperate pv structures. This code may already exist. A few hints in the right direction would be nice.
I'll keep poking around.

Zac Slade
[email protected]

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

I've merged this into CVS - we can finish testing it there alongside the other development going on at the moment.

Alasdair

[email protected]