----- "Radek Vykydal" <rvykydal@xxxxxxxxxx> wrote:
> 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.
ok, guess if this commit is part of the whole back port it might be better to leave it there. If the automated test show any misbehavior we can always tweak and/or revert.
> > 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).
This is my opinion as well, we can let the autotesting catch any regressions. I think this patch is a very good candidate to through a quality engineering and see what strangeness they find. And if any issue in encountered we can iron it out after the first testing cycle.
I would go ahead an commit
>
> 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
--
Joel Andres Granados
Red Hat / Brno Czech Republic
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
