Re: [PATCH] RHEL4.8 #233050 - ports of lvm PV size clamping from RHEL5

[Joel Granados <jgranado redhat com>]

These lvm, clamp things really bother me.....
Have some questions about the patch... I would just like some additional explination in some parts.

1. 
@@ -161,12 +168,38 @@ def vgremove(vgname):
.
     args = ["lvm", "vgremove", vgname]
.
+    log(string.join(args, ' '))
     rc = iutil.execWithRedirect(args[0], args,
                                 stdout = output,
                                 stderr = output,
                                 searchPath = 1)
     if rc:
         raise SystemError, "vgremove failed"
+    # now iterate all the PVs we've just freed up, so we reclaim the metadata
+    # space.  This is an LVM bug, AFAICS.
+    for pvname in pvs:
+        args = ["lvm", "pvremove", pvname]
+
+        log(string.join(args, ' '))
+        rc = iutil.execWithRedirect(args[0], args,
+                                    stdout = output,
+                                    stderr = output,
+                                    searchPath = 1)
+
+        if rc:
+            raise SystemError, "pvremove failed"
+
+        args = ["lvm", "pvcreate", "-ff", "-y", "-v", pvname]
+
+        log(string.join(args, ' '))
+        rc = iutil.execWithRedirect(args[0], args,
+                                    stdout = output,
+                                    stderr = output,
+                                    searchPath = 1)
+
+        if rc:
+            raise SystemError, "pvcreate failed for %s" % (pvname,)
+

So let me get this strait.  When we remove a vg we also remove the pv that the vg was in. only to create the pv again?  what the deal.  Whats the failure?

2. 
@@ -409,5 +421,10 @@ def getVGUsedSpace(vgreq, requests, diskset):
.
 def getVGFreeSpace(vgreq, requests, diskset):
     used = getVGUsedSpace(vgreq, requests, diskset)
+    log("used space is %s" % (used,))
+.....
+    total = vgreq.getActualSize(requests, diskset)
+    log("actual space is %s" % (total,))
+    return total - used
.....
     return vgreq.getActualSize(requests, diskset) - used
Do we ever reach this second return ^^^^

3.
@@ -752,21 +752,19 @@ class VolumeGroupRequestSpec(RequestSpec):
     def getActualSize(self, partitions, diskset):
         """Return the actual size allocated for the request in megabytes."""
.
-        # this seems like a bogus check too...
-        if self.physicalVolumes is None:
-            return 0
-
         # if we have a preexisting size, use it
         if self.preexist and self.preexist_size:
-            totalspace = ((self.preexist_size / self.pesize) *
-                          self.pesize)
+            totalspace = lvm.clampPVSize(self.preexist_size, self.pesize)
         else:
             totalspace = 0
             for pvid in self.physicalVolumes:
                 pvreq = partitions.getRequestByID(pvid)
                 size = pvreq.getActualSize(partitions, diskset)
-                size = lvm.clampPVSize(size, self.pesize)
-                totalspace = totalspace + size
+                #log("size for pv %s is %s" % (pvid, size))

                 ^^^^^ can we erase this?

+                clamped = lvm.clampPVSize(size, self.pesize)
+                log("  got pv.size of %s, clamped to %s" % (size,clamped))
+                #log("  clamped size is %s" % (size,))

                 ^^^^^ can we erase this?

+                totalspace = totalspace + clamped
.



For the rest of the patch I see a lot of clamping going on, Which I guess is to avoid a miscalculation in the pvs.  In the worst case scenario we will be creating a vg/lv/partition that does not *completely* occupy the pv.  Additionally, It very complex to truly see the extent of the patch in the whole partitioning code.  In this case I would put my vote on "commit and let the automatic testing catch stuff that has been overlooked".
When the comment above are addressed/answered, I would give it a go ahead :)
----- "Radek Vykydal" <rvykydal redhat com> wrote:

> Hi,
> 
> lvm partitioning (clamping of PV size) again, this time for 4.8.
> This is fix for 4.8 #233050. It required some commits to be
> ported from 5.3:
> 
> commit cc9d0f4e57b11a37fc5d33d0374509e43a97840c
> commit 8b4c702d0c2c6130c5263a4944405efa1301ced9
> commit 4f9c6e49d113a88a28c55c51bb5eab6ad756612b
> commit a68ab7d2823836ee90171cf419864e248ad99ce7
> commit 4aa9ca1c35b867fa5a4d94c41591700ca7ab5edb
> (5.3 #415871)
> commit 08233b0c42f8b453ff7d8bde03c9adc57d92d7ed
> (5.3 #463780)
> 
> I tested the patch with reproducer from bz and reproducers
> from #415871 and #463780 which are the last two commits
> of the list above.
> 
> Note: I didn't port patches fixing other corner cases hit and
> fixed in 5.3:
> - wiping old lvm metadata
>   https://bugzilla.redhat.com/show_bug.cgi?id=468431
>   https://bugzilla.redhat.com/show_bug.cgi?id=469700
> - handling corner case when clamped size == partition size
>   https://bugzilla.redhat.com/show_bug.cgi?id=468944
> - computing size of preexisting VG in UI
>   https://bugzilla.redhat.com/show_bug.cgi?id=217913
> - lvm on raid chunk alignment
>   https://bugzilla.redhat.com/show_bug.cgi?id=463431
> 
> Radek
> 
> 
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list redhat com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-- 
Joel Andres Granados
Red Hat / Brno Czech Republic