Minimally PV resizing would be AWESOME..  And fairly simple to do (for increasing).  simply "recreate" the partition entry to be the bigger size and execute the pvresize command on the volume.  I've done the process manually on a few systems before. Even live on a running system after I used the LSI rais tools to convert my RAID-1 into a RAID-5 :-D

Hi Curtis et al,

I am working on this bug again and I am after your opinion ...

Should deletion of an LVM2 Physical Volume continue to be allowed even
when it contains unknown data, or should it enforce the PV being empty
first?

Back when adding PV read-only support we argued, just let it be deleted
regardless of its contents because it was the only way GParted could do
it.  Now we are adding write support we could make a different decision.

Factors:
* Volume Groups can span multiple partitions on multiple disks which
  GParted doesn't display in one go.
  -> Potentially fixable by displaying it in the Information dialog, a
     new dialog or somewhere else.
* GParted can't show the label of a file system in a Logical Volume.
  -> This is getting into VG/LV support so possibly out of scope for the
     current PV read-write support.
* GParted can't mount a file system in a Logical Volume to allow the
  user to examine the contents.
  -> Ditto.
* GParted does show the VG name and usage of the PV so the user can see
  if it is empty before they choose to delete it.
* GParted doesn't provide any mechanism to empty a PV before deleting
  it.
  -> Could add partition menu option "Remove from Volume Group" which
     does "lvm vgreduce VGNAME /dev/DISK".

Options I can see are:
1) Do nothing.  Allow GParted to delete PV regardless.
2) Enforce PV being empty (not a member of a VG) before deletion.
2.1) Enforce via GParted UI, and/or
2.2) Enforce using "lvm pvremove /dev/DISK" which will only succeed if
     the PV is empty.
3) Provide extra information to be able to see which PVs are members of
   a VG.
4) Implement mechanism to remove PV from a VG.

I am inclined to implement options 2.1, 2.2, 3 & 4.
What are your thoughts?

Thanks,
Mike

IMHO, keeping the "safest" route is always the best route.  So disallow deleting a PV if it is not the only member of a VG.  Though "2.2" actually sounds a reasonable and safe option.   Yes it doesn't provide any automation in moving the contents, but does ensure safe removal.   So I agree with your inclination of how to approach this.     Ideally further down the road when gparted gets more advanced and can "open" multiple disks, then full LVM support would be awesome.  Currently the only other tool available is the redhat created system-config-lvm tool included in Fedora/RHEL.  Which does nearly everything around LVM except resizing PVs.

I am in agreement.  We want to keep our users informed as best we can, so that they can make decisions based on facts.  If a PV is not empty, it would help to show the user why the PV is not empty so at least they will have an idea about how to proceed.

Long term it would be great to be able to manage VGs and LVs.  To keep the scope to something reasonable I think it is best to take one step at a time, so handling PVs is a good first step.

(In reply to comment #2)
> Hi Curtis et al,
> 
> I am working on this bug again and I am after your opinion ...
> 
> Should deletion of an LVM2 Physical Volume continue to be allowed even
> when it contains unknown data, or should it enforce the PV being empty
> first?
> 
> Back when adding PV read-only support we argued, just let it be deleted
> regardless of its contents because it was the only way GParted could do
> it.  Now we are adding write support we could make a different decision.
> 
> Factors:
> * Volume Groups can span multiple partitions on multiple disks which
>   GParted doesn't display in one go.
>   -> Potentially fixable by displaying it in the Information dialog, a
>      new dialog or somewhere else.
> * GParted can't show the label of a file system in a Logical Volume.
>   -> This is getting into VG/LV support so possibly out of scope for the
>      current PV read-write support.
> * GParted can't mount a file system in a Logical Volume to allow the
>   user to examine the contents.
>   -> Ditto.
> * GParted does show the VG name and usage of the PV so the user can see
>   if it is empty before they choose to delete it.
> * GParted doesn't provide any mechanism to empty a PV before deleting
>   it.
>   -> Could add partition menu option "Remove from Volume Group" which
>      does "lvm vgreduce VGNAME /dev/DISK".
> 
> Options I can see are:
> 1) Do nothing.  Allow GParted to delete PV regardless.
> 2) Enforce PV being empty (not a member of a VG) before deletion.
> 2.1) Enforce via GParted UI, and/or
> 2.2) Enforce using "lvm pvremove /dev/DISK" which will only succeed if
>      the PV is empty.
> 3) Provide extra information to be able to see which PVs are members of
>    a VG.
> 4) Implement mechanism to remove PV from a VG.
> 
> I am inclined to implement options 2.1, 2.2, 3 & 4.
> What are your thoughts?
> 
> Thanks,
> Mike

I agree with 2.2, 3, and 4.  2.1 is not a bad idea, but is probably redundant with 2.2.

Also, if 4 doesn't happen as part of the pvremove call, it should probably be done by GParted as part of the pv deletion (before the pvremove call).

*** Bug 171144 has been marked as a duplicate of this bug. ***

Created attachment 218867 [details] [review]
LVM2 PV read-write support (Draft 1)

Hi Curtis,

I would like you to try this code and see if the usage model is correct.
If these are the right choices to present to the user, particularly with
respect to removal of PVs.  It's only a small change but I want it to
be spot on.
(Completing PV read-write support is currently the limit of my ambition
for adding LVM capabilities.  There are a few other improvements I want
to make to GParted, plus I think adding VG and LV support requires more
significant UI changes based on an agreed vision.  So even though the
current UI changes are small I want them right and be able to last a
good while).

A user can remove an empty PV just the same as any other file system.
However in contrast to my earlier thoughts (option 2) in comment #2
above) I have decided that a user should be able to remove a PV even if
it is a member of a VG, when given suitable a warning with relevant
information.  I also don't intend to implement a mechanism to remove a
PV from a VG (option 4), as I see this as VG management.

The code is not fully complete, the new "Delete non-empty PV" dialog
still displays a FIXME but the rest of it is there.  It creates, moves,
deletes and resizes PVs as intended.  Also the patchset and comments are
still work in progress so you don't need to review those at this time.

Rather than explain all the changes in detail please just run through
the following test cases to see them for yourself.

Set-up.  Create 3 new partitions the same size:
    Ptn1    Any file system
    Ptn2    PV which is NOT a member of a VG
    Ptn3    PV which is a member of a VG

Tests to see the UI changes:
    1)  Show partition Info for all 3 partitions
    2)  Try deleting each partition (and then undo all operations)
    3)  Try copying Ptn1 over Ptn2 and then Ptn3 (and then undo all
        operations)
    4)  Try formatting all 3 partitions (and then undo all operations)
(The code is there to apply all operations, but it is unnecessary to see
the UI changes and takes longer too).

The extra information in the Partition Information dialog about the VG
and its members is what I intend to add into the new "Delete non-empty
PV".  I also intend to remove it from the Information dialog as I think
that it is doesn't belong there.  Finally if you want to suggest wording
in the "Delete non-empty PV" dialog, that would be welcome too.

Thanks,
Mike

Hi Mike,

Thank you for this new patch.  Today I only had time to apply the patch and try the tests you suggested.  Based on these tests I like what I see so far.  :-)

In the next day or so I hope to review the code and suggest new wording.

Cheers,
Curtis

Hi Mike,

I have been busy and still haven't had a chance to review the code.
However, I did take a look at the message text so I can provide my
input in this area.

Your first draft of the wording looks pretty good.  To improve upon
it, I fixed a few spelling mistakes.  Also it is better to place the
question immediately preceding the buttons.

New Suggested wording:

     Deleting or overwriting the Physical Volume is irrecoverable
     and will damage the Volume Group.

     To avoid damaging the Volume Group, you are advised to cancel
     and use external LVM commands to free the Physical Volume
     before attempting this operation.

     Do you want to continue to forcibly delete the Physical Volume?


Also, since some desktop environments do not display the dialog window
title (I've noticed this with Gnome 3), I suggest adding the device
name to the following:

     You are deleting non-empty LVM2 Physical Volume %1.

     You are formatting non-empty LVM2 Physical Volume %1.

     You are pasting over non-empty LVM2 Physical Volume %1.


With the new warning dialog, I am once again debating which is better:

A)  Provide option to cancel or proceed

B)  Only provide proceed, but user can undo after.

In
     Bug #667278 - Add support for setting UUID
we opted to go with (B).  See:
https://bugzilla.gnome.org/show_bug.cgi?id=667278#c33

Now I think that option (A) might be better from a users perspective
because they can make the choice immediately, and not have to click
"OK", and then remember to "Undo".


What are your thoughts?

Curtis

Hi Curtis,

Don't worry about reviewing the code at the moment, it's incomplete and
some of it is a bit messy.  The code is just the vehicle by which to
show you the new proposed Delete non-empty PV dialog.  I want to make
sure we get the UI changes correct and secondarily the wording.

I'll go with your wording, or something similar.

With a warning dialog I definitely would prefer to be able to cancel
immediately rather than only being able to press OK and have to undo it
so as not to apply it.  But the answer could change depending on the
situation.  For example "Warning your OS may not boot after moving /boot
partition until you run lilo" may not need a cancel, but "Warning you
are about to delete a VG" (or part of one) definitely does.  GParted
already has an "Are you sure you want to apply the pending operations?"
for example.  I don't understand WPA so I can't really use that as an
example to argue with.  One reason a PV might require special treatment
is it can be a member of a VG spanning multiple PVs and the user might
wipe part of their data without fully realising.  Anyway I will have a
read of the following to see if it has any recommendations on the
subject:
    GNOME Human Interface Guidelines
    http://developer.gnome.org/hig-book/3.0/

Do you agree with my idea to add VGNAME and member information into the
new Delete non-empty PV dialog (assuming I can work out how to code a
Gtk table into a dialog)?  And not include it in the Information dialog
(where it currently is just to show it fully working)?  Other choices
are just do without it all together or create another new dialog?

Mike

(In reply to comment #10)
> Don't worry about reviewing the code at the moment...

Okay, I will await another patch set before reviewing the code.


> With a warning dialog I definitely would prefer to be able to cancel
> immediately rather than only being able to press OK and have to undo it
> so as not to apply it.

Your logic sounds good to me.  Let's go with the choice in the dialog
for LVM PV.  If you find something specific in the GNOME HIG, I would
like to know.  At the moment I do not recall anything specifically on
this choice.


> Do you agree with my idea to add VGNAME and member information into the
> new Delete non-empty PV dialog (assuming I can work out how to code a
> Gtk table into a dialog)?  And not include it in the Information dialog
> (where it currently is just to show it fully working)?  Other choices
> are just do without it all together or create another new dialog?

I do like the VGNAME and member information in the Delete non-empty PV
dialog.  This info helps our users with the decision to proceed to
damage the LV, or to cancel and free it up from the command line.

If a table cannot be coded into the dialog, then placing each piece
of information on it's own line is a good compromise.

E.g.,
Volume Group:  myVGName
Members:  myMembers

I also like this information in the Information dialog.  No need to
remove this information in my opinion.

(In reply to comment #11)
> ...  If you find something specific in the GNOME HIG, I would
> like to know.

Here's what I have found from the GNOME HIG ...


http://developer.gnome.org/hig-book/3.0/principles-forgiveness.html.en
> Forgive the User
...
> If an action is very dangerous, and there is no way to undo the
> result, warn the user and ask for confirmation. Only do this in
> extreme cases, though; if frequently faced with such confirmation
> messages, users begin to ignore them, making them worse than useless.

It seems to imply there should be a cancel button, but doesn't say so
explicitly.  It's also a bit self justifying to claim deleting non-empty
PVs is such a dangerous case.


http://developer.gnome.org/hig-book/3.0/windows-alert.html.en#alert-button-order
> 3.4.2. Alert Buttons
...
> Provide a Cancel button for all alerts displayed in response to a user
actions, ...

Explicit this time.


http://developer.gnome.org/hig-book/3.0/windows-alert.html.en#alerts-confirmation
> 3.4.6. Confirmation Alerts
...
> presents a Cancel button that will prevent execution of the user's
> command.

This one clinches it for me.


The new delete non-empty PV dialog gives us the ability to explain to
the user exactly what the danger is about with details of the VG to show
the impact of any decision.  GParted already has a "Are you sure you
want to apply the pending operation?" confirmation dialog with a cancel
button, but can't explain explicit impacts of each pending operation.


(In reply to comment #9)
> Also, since some desktop environments do not display the dialog window
> title (I've noticed this with Gnome 3), I suggest adding the device
> name

http://developer.gnome.org/hig-book/3.0/windows-alert.html.en
...
> Title Format
> Alert windows have no titles, as the title would usually unnecessarily
> duplicate the alert's primary text. This way, users can read and
> respond to alerts more quickly as there is less visual noise and
> confounding text.

So I'll leave the dialog window title blank and will include the device
name in the primary text.


http://developer.gnome.org/hig-book/3.0/windows-alert.html.en#alert-text
> 3.4.1 Alert Text
...
> The primary text is punctuated in 'newspaper headline' style, that is,
> it has no terminating period, but it may have a terminating question
> mark.

About half the dialogs in Win_GParted.cc break this.  Opportunity for
another tidy-up patch :-)


Now we're happy with the UI I'll work on the code some more.  Just
setting expectations ... it could be a couple of weeks.

(In reply to comment #12)
> <...stuff deleted about how alert dialogs should work...>
> 
> About half the dialogs in Win_GParted.cc break this.  Opportunity for
> another tidy-up patch :-)

Always something more that can be done.  :-)


> Now we're happy with the UI I'll work on the code some more.  Just
> setting expectations ... it could be a couple of weeks.

Thanks for looking up the relevant GNOME HIG standards.  No worries
on delivery dates either.

Hi Curtis,

I have coded the display of the VG name and members into the new delete
non-empty PV dialog using Gtk::MessageDialog::get_message_area() which
was new in gtkmm 2.22 released September 2010.

    get_message_area()
    http://developer.gnome.org/gtkmm/stable/classGtk_1_1MessageDialog.html#ab514233dce507f797ecb83021bd65930

Looking through DistroWatch it will be available from these versions of
these distributions:

    Fedora 14, openSUSE 11.4, Debian testing & unstable,
    Ubuntu 10.10 & 12.04 LTS, GParted LiveCD 0.8.1

but not the latest versions of these distributions:

    Red Hat (& CentOS) 6.3, SUSE Linux Enterprise 11-SP2, Debian 6.0


Is it OK for GParted 0.14.0 to require gtkmm >= 2.22 or do I need to
look for another way to code the dialog?  If it's OK do I need to code
an autoconf test to ensure the function is available?

Thanks,
Mike

For now I would prefer to maintain backward compatibility for releases
that are still supported, such as Ubuntu 10.04 LTS.

To set up an autoconf test for gtkmm-2.4 >= 2.22 you could copy the
existing text for the gtkmm-2.4 2.16 test.  See

   http://git.gnome.org/browse/gparted/tree/configure.in#n254

Note that gtkmm-2.4 is the version for the ABI, not the actual version.  :-)
You could use a name like "HAVE_GTKMM_2_22_PLUS" to be more generic.


An example of using HAVE_GTK_SHOW_URI exists in Win_GParted.cc.  See

   http://git.gnome.org/browse/gparted/tree/src/Win_GParted.cc#n1350


For the situation where the dialog tables are not available, I think
it would be okay to list "Volume Group" and "Members" on their own lines.

Created attachment 220983 [details] [review]
LVM2 PV read-write support (v1)

Hi Curtis,

Here's the PV read-write patchset finally ready for review.  I've been
distracted by watching the Olympics :-).

Compared to my ideas from comment #2 the code does the following:
* Displays the discussed delete non-empty LVM2 PV dialog
* Removes PVs, forcibly for non-empty ones, updating LVM2 metadata
  recording the fact.
  + This precludes recovery from partition deletion.  Rational as I
    see it is documented in the relevant commit message.
* Doesn't implement copying of PVs.  Again rational is in the relevant
  commit message.
* Doesn't implement a mechanism to free a PV from a VG.  It can be
  forcibly removed after acknowledging the delete non-empty LVM2 PV
  dialog.

There are 8 core patches followed by 7 tidyup patches.

There are 2 known limitations/bugs:
1) A VG with 2 or more missing PVs will only show one "unknown device".
2) Check, Move or Resize of a PV in an exported VG will fail.  LVM
   doesn't allow any command, other than vgimport, on an exported VG so
   the PV check fails.
I will fix both with subsequent patches.

Thanks,
Mike

Hi Mike,

This patch set looks great.  The code is logical and the commit comments are very helpful when reviewing each commit.

This time around I only have one suggestion for a change in wording.

Since the two verbs in "... destroying or damaging the Volume Group ..."  mean almost the same thing, we should choose one o the other.
To me "damage" implies that there is hope of recovery.
To me "destroy" means there is no hope of recovery.

If I follow the comments correctly, there is no hope of recovery so we should probably use "... destroying the Volume Group ..."

Does that sound reasonable to you?

Thanks,
Curtis

(In reply to comment #17)
> This patch set looks great.  The code is logical and the commit comments are
> very helpful when reviewing each commit.

Thank you.


> Since the two verbs in "... destroying or damaging the Volume Group ..."  mean
> almost the same thing, we should choose one o the other.
> To me "damage" implies that there is hope of recovery.
> To me "destroy" means there is no hope of recovery.
>
> If I follow the comments correctly, there is no hope of recovery so we should
> probably use "... destroying the Volume Group ..."
>
> Does that sound reasonable to you?

It's possible to use a VG with a missing PV (in a degraded state), but I
see the primary use case as deleting all the PVs in a VG, rather than
choosing to delete just one PV to achieve a degraded state, so lets just
go with your suggestion of "To avoid destroying the Volume Group ...".

I assume you'll do a git rebase to update the wording.


(In reply to comment #16)
> There are 2 known limitations/bugs:
> ...
> 2) Check, Move or Resize of a PV in an exported VG will fail.  LVM
>    doesn't allow any command, other than vgimport, on an exported VG so
>    the PV check fails.
> I will fix both with subsequent patches.

I have re-tested Check, Move and Resize operations on a PV in an exported
VG.  Checking the PV works, but it's resizing the PV which fails.

    # lvm pvresize -v /dev/sda15
        Using physical volume(s) on command line
      Volume group Test_VG4 is exported
      Unable to read volume group "Test_VG4".
      0 physical volume(s) resized / 1 physical volume(s) not resized
    # echo $?
    5

The options I see are:
A1) Ignore.  Let it fail allowing the user to discover the above error.
    But for Resize the operation will be left half applied.
A2) Add a dialog reporting Check/Move/Resize can't be performed on a
    PV in an exported VG.
A3) Disable Check/Move/Resize for a PV in an exported VG, but how is the
    user meant to know why they have been disabled.
A4) Something else.
What are your thoughts?

Thanks,
Mike

(In reply to comment #18)
> I assume you'll do a git rebase to update the wording.

Yes, I can do a git rebase to update the wording

I plan to do some more testing on this patch set prior to committing it to the git repository.


> I have re-tested Check, Move and Resize operations on a PV in an exported
> VG.  Checking the PV works, but it's resizing the PV which fails.
> 
>     # lvm pvresize -v /dev/sda15
>         Using physical volume(s) on command line
>       Volume group Test_VG4 is exported
>       Unable to read volume group "Test_VG4".
>       0 physical volume(s) resized / 1 physical volume(s) not resized
>     # echo $?
>     5
> 
> The options I see are:
> A1) Ignore.  Let it fail allowing the user to discover the above error.
>     But for Resize the operation will be left half applied.
> A2) Add a dialog reporting Check/Move/Resize can't be performed on a
>     PV in an exported VG.
> A3) Disable Check/Move/Resize for a PV in an exported VG, but how is the
>     user meant to know why they have been disabled.
> A4) Something else.
> What are your thoughts?

My preference would be for (A2) since this option provides the user with an explanation as to why the check/resize/move cannot be performed.

This choice is more in line with PalmOS philosophy.  If I recall correctly, Gnome HIG says to disable the menu entries.  Unfortunately disabled menu entries won't tell a user why these entries are disabled.  In this case I think that (A2) is the better way to proceed.

(In reply to comment #18)
> (In reply to comment #17)
> > Since the two verbs in "... destroying or damaging the Volume Group ..."  mean
> > almost the same thing, we should choose one o the other.
> > To me "damage" implies that there is hope of recovery.
> > To me "destroy" means there is no hope of recovery.
> >
> > If I follow the comments correctly, there is no hope of recovery so we should
> > probably use "... destroying the Volume Group ..."
> >
> > Does that sound reasonable to you?
> 
> It's possible to use a VG with a missing PV (in a degraded state), but I
> see the primary use case as deleting all the PVs in a VG, rather than
> choosing to delete just one PV to achieve a degraded state, so lets just
> go with your suggestion of "To avoid destroying the Volume Group ...".
> 
> I assume you'll do a git rebase to update the wording.

After re-reading this, and seeing that "it is possible to use a VG with a missing PV (in a degraded state)", then perhaps "damaging" is the right word.

Since I have waffled on the wording, perhaps we should just leave the wording as originally spelled out.

Created attachment 221710 [details] [review]
LVM2 PV read-write support (v2)

Here's version 2 of the patchset.  First 15 patches are identical to
version 1.  2 extra patches resolve the only showing one "unknown
device" bug.

Check, Move or Resize operation failing for a PV in an exported VG is
still outstanding.

Mike

Great work again Mike!  I tested this on Fedora17 and Ubuntu 10.04 and my results were as expected.  The code looks good too.  :-)

I was trying to think if we needed any updates to the GParted Manual.  Since we are treating the LVM2 PVs as a type of file system, my first thought is that the existing documentation is sufficient since the docs do not specifically go into details for each and every file system.

What are your thoughts with regards to the documentation?

(In reply to comment #19)
> (In reply to comment #18)
> > I have re-tested Check, Move and Resize operations on a PV in an exported
> > VG.  Checking the PV works, but it's resizing the PV which fails.
...
> > 
> > The options I see are:
> > A1) Ignore.  Let it fail allowing the user to discover the above error.
> >     But for Resize the operation will be left half applied.
> > A2) Add a dialog reporting Check/Move/Resize can't be performed on a
> >     PV in an exported VG.
> > A3) Disable Check/Move/Resize for a PV in an exported VG, but how is the
> >     user meant to know why they have been disabled.
> > A4) Something else.
> > What are your thoughts?
> 
> My preference would be for (A2) since this option provides the user with an
> explanation as to why the check/resize/move cannot be performed.
> 
> This choice is more in line with PalmOS philosophy.  If I recall correctly,
> Gnome HIG says to disable the menu entries.  Unfortunately disabled menu
> entries won't tell a user why these entries are disabled.  In this case I think
> that (A2) is the better way to proceed.

How about:

A4) Partition warning message like "It is not possible to resize a
Physical Volume which is a member of an exported Volume Group.
Resize/Move and Check operations disabled".

along with:

A3) Disable Resize/Move and Check operations in the Partition menu.

Good idea or not?  This will add a warning to every PV in an exported
VG.


For reference this is what the GNOME HIG says:

http://developer.gnome.org/hig-book/3.4/menus.html.en
> Guidelines
...
> * Make a menu item insensitive when its command is unavailable. For
>   example, the Edit > Copy item, which issues the command to copy
>   selected data to the clipboard, should not be active when there is
>   no data selected.

(In reply to comment #22)
> I was trying to think if we needed any updates to the GParted Manual.  Since we
> are treating the LVM2 PVs as a type of file system, my first thought is that
> the existing documentation is sufficient since the docs do not specifically go
> into details for each and every file system.
> 
> What are your thoughts with regards to the documentation?

Agreed.  The manual doesn't go into any details about what file systems
are supported other than saying to choose: View -> File System Support.

(In reply to comment #23)
> How about:
> 
> A4) Partition warning message like "It is not possible to resize a
> Physical Volume which is a member of an exported Volume Group.
> Resize/Move and Check operations disabled".
> 
> along with:
> 
> A3) Disable Resize/Move and Check operations in the Partition menu.
> 
> Good idea or not?  This will add a warning to every PV in an exported
> VG.

I would be okay with this approach too.  And as you point out (A3) is better aligned with GNOME HIG.  By adding (A4) that helps to explain to the user why the exported VG cannot be moved/resized/checked.

(In reply to comment #25)
> (In reply to comment #23)
> > How about:
> > 
> > A4) Partition warning message like "It is not possible to resize a
> > Physical Volume which is a member of an exported Volume Group.
> > Resize/Move and Check operations disabled".
> > 
> > along with:
> > 
> > A3) Disable Resize/Move and Check operations in the Partition menu.
> > 
> > Good idea or not?  This will add a warning to every PV in an exported
> > VG.
> 
> I would be okay with this approach too.  And as you point out (A3) is better
> aligned with GNOME HIG.  By adding (A4) that helps to explain to the user why
> the exported VG cannot be moved/resized/checked.

Though of another idea, but I don't know I can do it yet.  (A4) with:

A5) Have Resize/Move and Check work as though resize wasn't implemented,
    but just for PVs in an exported VG.

I'm making sure I understand all the code so I can work out how to do it
without grossly hacking .shrink and .grow temporarily to NONE.

I think I better understand the challenge we face with LVM PVs and
trying to map this to the GParted file system paradigm.  In the next
few paragraphs I will try to explain how I see LVM PVs and file
systems being similar, where these also differ, and thoughts on how to
proceed.

GParted actions available are determined at program startup based on
the settings of flags like .shrink, and .grow.

Further, actions are controlled by whether file systems:
 - are either mounted (active) or unmounted (inactive)
     which is determined at device refresh time.
 - are found to be consistent, or inconsistent
     which is determined when a "check" action is performed.

Similarly, actions are controlled by whether LVM Physical volumes:
 - are either active or inactive
 - are found to be consistent, or inconsistent

However, LVM PVs differ in that another state is possible,
specifically "exported" volume group.

Since the "exported" status means that the LVM PV cannot be resized,
we need to somehow take this into account.  I assume that the LVM PV
can still be moved, even with the "exported" status.  The challenge is
how to handle this in GParted.

My first thought was to add some more code to the LVM PV "check" to
ensure that the partition is not "exported".  If the partition is
"exported" then return failure on the "check" method.  Unfortunately
this will also prevent "move" from working since a successful "check"
is required before "move" is performed.

Another thought I had was to add a check in the core resize section to
also check for "exported" status if the partition was an LVM PV.  The
drawback to this method is that this logic would be in
GParted_Core.cc, and not contained solely within the lvm2_pv.cc file.

Your suggestion of somehow toggling .shrink and .grow might work, but
might also be a gross hack as you point out.  This is certainly worth
investigating.

Yet another option might be to add another type of action-limiting
control check to each file system type.  For most file systems this
would simply return true, but for LVM PV it would check for "exported"
status.  There might be some gotchas if we took this route too,
though at the moment I can't think of any.

Ultimately we desire to have the proper functionality available to the
user, but also in a form that is easy to support, and possibly extend
in the future to handle LVM Logical Volumes.

We've both had similar thoughts on the options to handle non-resizeable
PVs in exported VGs.

I had drafted lots of discussion agreeing with you and expanding a few
points on what my ideas were for implementing (A5).  Then I started
looking at the code.  It seems only about a couple of hundred of lines
of code change.  So I'll chuck together a draft patch to implement (A4)
and (A5) and see what you think.

Created attachment 222196 [details] [review]
Non-resizable PV in exported VG patch (Draft 1)

Hi Curtis,

Here's a patch which handles a PV not being resizable because it's a
member of an exported VG.  It implements:
A4) Partition warning about PV not being resizable;
A5) Have Check operation gracefully skip file system maximise if
    disabled and prevents the Move/Resize dialog from creating a resize
    operation if resizing disabled.

In terms of user experience this maximises functionality.  The patch is
quite small.  What do you think?  OK to tidy up and submit?  There's
also a notable question in the patch about adding runtime checks into
resize_move() or its callees.

Thanks,
Mike

Thanks for the new patch.  Wow you sure are quick.  :-)

I will review the patch and get back to you tomorrow.

Don't be too impressed.  The code tries to be generic about preventing
FS resizing, but I have realised that it only prevents Move/Resize
dialog submitting an operation which could resize, not Copy & Paste.
It's enough for LVM2 PVs as they can't be copies but not yet the full
generic FS resizing prevention I was after.  Anyway it gives an idea of
what I am suggesting.

Hi Mike,

After reviewing the patch in comment #29, and testing it with exported and non-exported LVM PVs, I think that your proposed solution can work.

I particularly liked that you made a separate filesystem_resize_disallowed method so that this functionality can be more easily extended to "file systems" other than LVM2 PV.

In fact this approach is similar to another method GParted_Core::update_bootsector which currently only has an entry for NTFS.

Ideally it would be nice to have all of the file system specific stuff in the separate file system classes.  Having said that, it has not been hard to maintain the update_bootsector method, so I expect filesystem_resize_disallowed to be similar.

One minor note is with a spelling mistake in one of the strings in lvm2_pv.cc.
Specifically,
     The Physical Volume can not be resize because it is a member
     of an exported Volume Group.
should be:
     The Physical Volume can not be resized because it is a member
     of an exported Volume Group.
(E.g., resized instead of resize).  :-)

Created attachment 222491 [details] [review]
LVM2 PV read-write support (v3)

Hi Curtis,

Here's version 3 of the patchset.  First 17 patches are identical to
version 2, with the exception of updating the comment for number 16 to
add the missing "Bug #670171 - ..." footer.  Additions are:

* Two patches to resolve PVs not being resizable when they are members
  of exported VGs.  Follows the method outlined in the draft patch from
  comment #29.

* Third patch which fixes an issue with not updating supported actions
  for lvm2 pvs when rescanning for supported actions.

Rambling ...

To give some level of confidence that the patches to resolve the non-
resizability of PVs in an exported VG are correct I did the following
additional testing:

1) Made nilfs2 not support resizing by removing nilfs-resize command.
2) Made reiserfs resizing not allowed by temporarily adding into
   filesystem_resize_disallowed().
3) Tested Paste and Move/Resize dialogs.
4) Tested copy, resize/move and check operations.  (Copy test included
   pasting into a new partition, pasting into into an existing partition
   if the same size and larger).

Behaviour for nilfs2 and reiserfs was identical, except for the
different warning message detailing the reason for resizing not being
available.

All outstanding issues resolved.

Thanks,
Mike

Hi Mike,

Wow!  This new patch set 'just works!'.  I had no problem with any of my testing, and your extensive testing is much appreciated too.  This might be the most bug free piece of code that I've ever committed.  :-)

The 20 individual changes in the patch set from comment #33 have been committed to the git repository for inclusion in the next release of GParted.

The relevant commits can be viewed at the following links:

Add creation of LVM2 PVs (#670171)
http://git.gnome.org/browse/gparted/commit/?id=c3ab62591b73266f43b379d9a7aef3be13f3c384

Enable LVM2 VG activation / deactivation (#670171)
http://git.gnome.org/browse/gparted/commit/?id=619bda5d8bf9b9aeb393aa4c435735c4c2a8d228

Add LVM2 PV resize, check and move operations (#670171)
http://git.gnome.org/browse/gparted/commit/?id=566ebc1b8808c423def95e877f12ebbfb305c4b1

Add file system specific remove() methods (#670171)
http://git.gnome.org/browse/gparted/commit/?id=795a92f5b2d4438664f7f520c7131add4c27ca1c

Implement LVM2 PV remove() method (#670171)
http://git.gnome.org/browse/gparted/commit/?id=1a6235499594ffcced939b93bab937e928a0d6f3

Add LVM2 VG member details to the Information dialog (#670171)
http://git.gnome.org/browse/gparted/commit/?id=307f489177bf7216c7458e88cc0ed445698ffa7a

Add warning dialog when deleting non-empty LVM2 PVs (#670171)
http://git.gnome.org/browse/gparted/commit/?id=69c8acce759e902f2d8eb81b5077c4f5a088465e

Add fallback implementation to new delete LVM2 PV warning dialog (#670171)
http://git.gnome.org/browse/gparted/commit/?id=590f1377f9dd5999564ddb93b2e2054fc2f6750d

Rename *toggle_swap_mount* -> *toggle_busy*
http://git.gnome.org/browse/gparted/commit/?id=ca3d40d9c72caacb172231428405ec53cc482771

Only lookup LVM2 VG name once in Display_Info()
http://git.gnome.org/browse/gparted/commit/?id=fae040c63ed96eefc095143ee9209a3b5d907a04

Remove redundant lines from LVM2_PV_Info functions
http://git.gnome.org/browse/gparted/commit/?id=7c8156b7d2914b1bc02eda6debf2542357fef46c

Update declarations of some LVM2_PV_Info member functions
http://git.gnome.org/browse/gparted/commit/?id=c90365a6dbe4a809c6e2fa60ce8a6222e2b90e6e

New LVM2_PV_Info::bit_set() testing VG and LV attribs "bits"
http://git.gnome.org/browse/gparted/commit/?id=5cb6c687ba7f9a7fd616ce948b10a0238efa4a04

Remove outdated FIXME comment from active_partitions_on_device_count()
http://git.gnome.org/browse/gparted/commit/?id=85e9ce342881af35af459689dd72f62fdae273ff

Remove full stops from the end of primary text in dialogs
http://git.gnome.org/browse/gparted/commit/?id=fbc4e9e941f8b8d4b90a19e3f05fceeadeb27717

Correctly show multiple "unknown device" LVM2 VG members (#670171)
http://git.gnome.org/browse/gparted/commit/?id=fdb7e9fe89c951efd93a3d8d92a61a9ff995b4ee

Implement common LVM2_PV_Info cache search and index functions
http://git.gnome.org/browse/gparted/commit/?id=96c9fc129c7ccb79526f377146d6a599c203134c

Disallow resizing of LVM2 PVs which are members of exported VGs (#670171)
http://git.gnome.org/browse/gparted/commit/?id=ee49891611c9633a6e3c63b9e202b1c20a779b78

Add a partition warning for LVM2 PVs which can't be resized (#670171)
http://git.gnome.org/browse/gparted/commit/?id=60d772817706e305a3b6c97c6b08752228b6f906

Recognise lvm command immediately when rescanning for supported actions
http://git.gnome.org/browse/gparted/commit/?id=99abbb06ff43329069e341de3dff35135b1072c3

Thank you Mike for this great new functionality.

Sincerely,
Curtis Gedak

This major enhancment has been included in GParted 0.14.0 released on October 10, 2012.

This help page has been updated: https://help.ubuntu.com/community/ResizeEncryptedPartitions