Re: [libvirt] [PATCH v3 4/4] logical: Display thin lv's found in a libvirt managed volume group

On 02/02/2016 08:30 AM, Pavel Hrdina wrote: > On Tue, Feb 02, 2016 at 09:11:51AM +0100, Ján Tomko wrote: >> On Mon, Feb 01, 2016 at 03:29:53PM -0500, John Ferlan wrote: >>> Modify the regex for the 'devices' (a/k/a 'extents') from "(\\S+)" >>> (e.g., 1 or more) to "(\\S*)" (e.g., zero or more). >>> >>> Then for any "thin" lv's found, mark the volume as a sparse volume so >>> that the volume wipe algorithm doesn't work. >>> >>> Since a "thin" segtype has no devices, this will result in any "thin" >>> lv part of some thin-pool within a volume group used as a libvirt pool >>> to be displayed as a possible volume to use. >>> >> >> A thin pool is another layer on top of some of the LVs in the VG. >> I think it deserves a separate pool type. > > I must agree with Jan, and also already wrote the same for v2 patch. The thin > pool and thin LV shouldn't be mixed with normal logical pool even they share the > same VG. > If LVM allows it, then who are we to say it cannot or shouldn't work or how it should be configured?

Thin pools were introduced in LVM2 (2.02.89) according to what I've found. That's a few releases behind the 2.02.132 I have on my f23 laptop. Nothing in the description creating a thin-pool requires it's own volume group although sure some "best practices" indicate creating vg's as the container for a thin-pool. I can certainly understand and agree in principle to creating a "strong preference" to have a thin-pool be a libvirt pool because it simplifies things such as allocated/capacity management and physical device source listing.

So should I assume then the desire from the review viewpoint is to not support thin lv's at this point in time?

And that would be because we want them to be configured a specific way? FWIW: if someone did that - we still won't be able to support it without this patch.

Any thoughts on the VIR_STORAGE_VOL_LOGICAL*_REGEX's? (depending on the answer to the prior question with the obvious change of using S+ instead of S* for devices)?