Joel Granados wrote:
----- "Radek Vykydal" <rvykydal@xxxxxxxxxx> wrote:
Joel Granados Moreno wrote:
Hi list:
Here is my second wack at solving this issue (468944). It is VERY
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. This
means that when you clamp the pv size in the vg size calculation there
will be no difference between the unclamped and the clamped value.
This means that we are calculating the vg available size as the sum of
the total sizes of all the pvs (partitions) that compose the vg, which
is not accurate. So to make sure that the vg size is less than the
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 used
for
metadata in.
Most of the time, clamping will protect us.
Moreover, in Fedora subtracting of one PE after clamping protects us
for
sure
(though it is probably overkill). This subtracting was patched in rhel
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 the
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 only
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 self.origvgrequest.preexist_size:
+ 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
clamping)
wouldn't break 217913 as well as your conditional subtracting of PE doesn't.
size less then is needed
for metadata - then we wouldn't subtract the PE. I think subtracting
some reserve
(but what size?) for metadata from partition size before clamping, is
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 whatever)
should be added to VolumeGroupRequestSpec.getActualSize which is used in
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,
size for LV --grow can be computed wrongly).
But I agree it is just another (maybe even less probable) corner case.
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 argument.
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 only
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 hit
other corner cases in future.)
Radek
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
