Thanks for review,
Joel Granados wrote:
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?
The comment from ported RHEL5/fedora commits says:
* lvm.py: remove and recreate all PVs when we do a vgremove, so they
don't lose 1 PE each time due to an lvm2 bug.
I am not sure if it still aplies, but the code haven't been removed from
rhel5 and fedora.
Given the amount of patches I am backporting here and their possible
interdependency, I decided to keep the code in rhel4 branch too.
We do this both in rawhide and rhel5, and I was wondering why when
I was fixing lvm there. I think we can investigate and patch this issue
later
(for rhel5 and fedora too), but with this commit I just want to backport
what is needed to fix the bug.
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 ^^^^
Nope, I'll cut it out, thanks.
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
.
Again, I preferred to port the patches as they are to keep the
commit/porting
history as clear as possible. The comments don't hurt and when we'll try
hard
to find what was ported and what wasn't next time, it might prevent
distraction.
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 :)
Oh yes, it's not easy to follow the history of clamping fixes. I hope to
bring
rhel4 branch in state close to where we are in 5.3, except for mentioned
corner cases which can be addressed later (after a bug is open).
Radek
----- "Radek Vykydal" <rvykydal@xxxxxxxxxx> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
