[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] make sure that we calculate a vg size that is less than the total parition size.

Joel Granados wrote:
----- "Radek Vykydal" <rvykydal redhat com> 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
(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
appear in another corner case when the partition size is clamped only

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.)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]