----- "Radek Vykydal" <firstname.lastname@example.org> wrote:
> Joel Granados wrote:
> > ----- "Radek Vykydal" <email@example.com> wrote:
> >> Joel Granados Moreno wrote:
> >>> Hi list:
> >>> Here is my second wack at solving this issue (468944). It is
> >> ugly. but I felt corned and see no other way out. The bug ocurrs
> >> when the pv size (partition size) is a multiple of the pe size.
> >> means that when you clamp the pv size in the vg size calculation
> >> will be no difference between the unclamped and the clamped value.
> >> This means that we are calculating the vg available size as the sum
> >> the total sizes of all the pvs (partitions) that compose the vg,
> >> is not accurate. So to make sure that the vg size is less than
> >> total sum of the pv sizes (partition sizes) I take away 1 pe. Its
> >> ugly I know...
> >> When calculating VG size available for LVs we don't count space
> >> for
> >> metadata in.
> >> Most of the time, clamping will protect us.
> >> Moreover, in Fedora subtracting of one PE after clamping protects
> >> for
> >> sure
> >> (though it is probably overkill). This subtracting was patched in
> >> 5.3
> >> (commit 68ab7d2823836ee90), but not fw ported to Fedora.
> sorry, commit 68ab7d282383 doesn't exitst, the right hash is
> a68ab7d282383, but you found it...
> >> So subtracting one PE in rhel 5.3 as in the patch, should solve
> >> bug
> >> case, but as it
> >> happens here only when clamped size is equal to partition size, it
> >> could
> >> appear in another corner case when the partition size is clamped
> >> by
> > This is true. and it probably does. But I think that the
> probabilities of the partition table being in such a state are quite
> low. And the commit that you mentioned fixed bug 217913, which would
> come back if we were to subtrace 1 pe at that point.
> 217913 concerned bad computing of VG size in UI for
> preexisting partitions, that means the function computeVGSize.
> IMO, the correct patch of 217913 should have been
> the if clause for preexisting VGs that you added in your patch:
> + if self.origvgrequest.preexist and
> + availSpaceMB =
> lvm.clampPVSize(self.origvgrequest.preexist_size, curpe)
> + else:
> that is: in dialog (in computeVGSize),
> use size of preexisting VG, and don't compute anything
> (do it as in VolumeGroupRequestSpec.getActualSize).
> With this portion of patch added, things I suggested (subtracting
> one PE after clamping in any case, or subtracting some amount before
> wouldn't break 217913 as well as your conditional subtracting of PE
Ok, I see your point. I'll add this one line change to the size calculation in VG request and test further. See what happens
. I don't expect anything serious.
> >> size less then is needed
> >> for metadata - then we wouldn't subtract the PE. I think
> >> some reserve
> >> (but what size?) for metadata from partition size before clamping,
> >> better in this sense.
> > It may be safer, or it may take us back to bug 217913
> as I said above, with if branch for preexisting VGs, it wouldn't
> >> Or, subtracting one PE in any case, like in Fedora.
> >> Also, it is patched here only for interactive case, and not for
> >> VolumeGroupRequestSpec.getActualSize
> > Which in this case will not break anything because if the VG already
> existed the value that is in the VGRequest will be used. Only if the
> VG is new will we use the new code. And after the new VG is created,
> VG.getActualSize will return whatever the new function calculated.
> >> used in ks case.
> here I am talking about kickstart case. I'm not saying that it would
> break something, but
> that the fix of computing (be it yours or extra PE subtraction or
> should be added to VolumeGroupRequestSpec.getActualSize which is used
> ks case, too,
> because the special case of your bug could occur in ks I think
> (when creating VG with one PV of partition with size same as clamped
> size for LV --grow can be computed wrongly).
> But I agree it is just another (maybe even less probable) corner
I think that adding the extra condition in the VG request will catch these problems.
> To sum up:
> The thing is that we are using duplicate code in 2 functions:
> VolumeGroupRequestSpec.getActualSize, and its
> copy VolumeGroupEditor.computeVGSize used in UI,
> which differs only in that it takes PE size set in dialog as
> I think that both functions should be same in other respects:
> 1) compute the size value in the same way
> 2) use preexisting VG size if dealing with preexisting VG
> You make them same wrt 2) in your patch, and I'd like to
> see them same wrt 1) too (in ideal world).
> So to my comments to the patch are:
> - it fixes only the corner case of the bug (not others when PV size is
> a little bigger than clamped size - which would extracting of safe
> amount before
> clamping or PE after clamping do)
> - it is only for UI case (not ks case) - but yes, the bug is for UI
> case, and I
> am not sure if I should or want create reproducer for ks case
> - but given the stage of cycle we are in, the patch seems ok to me to
> fix the bug with minimal risks.
> (Apologies for long comment, I am doing it also as a notice in case we
> other corner cases in future.)
What I want to do is to put the "take a way 1 PE in case the clamped size and the calculated size are the same. in both the lvm dialog and the VG request.
I'm really not confortable just taking away 1 PE always and would rather not consider it unless something else breaks (which is totally possible.)
I'll review the patch once more test and see what happens.
> Anaconda-devel-list mailing list
Joel Andres Granados
Red Hat / Brno Czech Republic
Anaconda-devel-list mailing list