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

[Joel Granados <jgranado redhat com>]

----- "Radek Vykydal" <rvykydal redhat com> wrote:

> 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
> >> 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 redhat com
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-- 
Joel Andres Granados
Red Hat / Brno Czech Republic