----- "Radek Vykydal" <rvykydal@xxxxxxxxxx> wrote: > 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. 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 > 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. 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 > 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 > 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. Regards. > > > _______________________________________________ > 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
