Bug 2100608

Devel finished with 

https://listman.redhat.com/archives/lvm-devel/2022-May/024180.html

backported to RH8.7 branch.

If vdo_block_map_period is an alias for block_map_era_length, then shouldn't it work with lvs to get the value as well? Why have this alias at all when vdo_block_map_era_length is already a valid interface?

[root@hayes-01 ~]# lvs -a -o +vdo_block_map_period vdo_sanity/vdo_pool
  [...]
  Unrecognised field: vdo_block_map_period

[root@hayes-01 ~]# lvs -a -o +vdo_block_map_era_length vdo_sanity/vdo_pool
  LV       VG         Attr       LSize Pool Origin Data%  Meta%  Move Log Cpy%Sync Convert VDOBlockMapEraLength
  vdo_pool vdo_sanity dwi------- 5.00g             60.03                                                  16380

Why does vdo_block_map_cache_size not report an int like all the other properties listed?

[root@hayes-01 ~]#  lvs -o vdo_block_map_cache_size vdo_sanity/vdo_lv
  VDOBlockMapCacheSize
               128.00m


And, if it reports a non int, why doesn't it take a non int for the alteration cmd?

[root@hayes-01 ~]# lvchange --vdosettings vdo_block_map_period=256.00m /dev/vdo_sanity/vdo_lv
  Parse error at byte 29 (line 1): unexpected token

[root@hayes-01 ~]# lvchange --vdosettings vdo_block_map_period=256.00 /dev/vdo_sanity/vdo_lv
  Invalid argument for VDO setting "vdo_block_map_period".

[root@hayes-01 ~]# lvchange --vdosettings vdo_block_map_period=256 /dev/vdo_sanity/vdo_lv
  Logical volume vdo_sanity/vdo_lv changed.


Please post the devel test results for these options.

Doesn't seem to change anything anyways:

[root@hayes-01 ~]# lvs -o vdo_block_map_cache_size vdo_sanity/vdo_lv
  VDOBlockMapCacheSize
               128.00m

[root@hayes-01 ~]# lvchange --vdosettings vdo_block_map_period=256 /dev/vdo_sanity/vdo_lv
  Logical volume vdo_sanity/vdo_lv changed.

[root@hayes-01 ~]#  lvs -o vdo_block_map_cache_size vdo_sanity/vdo_lv
  VDOBlockMapCacheSize
               128.00m

For this moment -  we only support aliasing  for 'vdosettings' input as a syntax sugar (maybe user should be notified about proper lvm2 option name).
We may eventually add same kind of similar support for 'lvs' (in some future iteration).

Related to comment #7
As with 'size input' for some settings - this setting parser is the same as with 'configuration' parser - so input value have same limitation and user can use 'just' plain integer value with all such settings.

I can see this to be a candidatre for RFE to improve some parametric input - but ATM all --config, --cachesettings, --vdosettings, --metadataprofile, --profile have simple parser accepting only simple values without any decimal points or SI unit suffixes.

As for  comment #7

block_map_period is an alias for block_map_era_length - so not related  vdo_block_map_cache_size.
So it's correct the vdo_block_map_cache_size stays with 128M size.

So to print with lvs:  -o+vdo_block_map_era_length

So ATM all works as it's been designed for current release.

Still, vdo_block_map_cache_size is listed as one of the properties that is now supported to be changed, and it still appears there is not way to do that.

[root@hayes-01 ~]# lvs -o vdo_block_map_cache_size vdo_sanity/vdo_lv
  VDOBlockMapCacheSize
               128.00m

[root@hayes-01 ~]# lvchange --vdosettings 'vdo_block_map_cache_size=256.00m' /dev/vdo_sanity/vdo_lv
  Parse error at byte 33 (line 1): unexpected token

[root@hayes-01 ~]# lvchange --vdosettings 'vdo_block_map_cache_size=256' /dev/vdo_sanity/vdo_lv
  Unknown VDO setting "vdo_block_map_cache_size".

[root@hayes-01 ~]# lvs -o vdo_block_map_cache_size vdo_sanity/vdo_lv
  VDOBlockMapCacheSize
               128.00m


Sure, "Parse error at byte 33 (line 1): unexpected token" is a bug that can be fixed later and not a part of this, but the fact that the lvchange fails with "Unknown VDO setting "vdo_block_map_cache_size"." implies that this doesn't work as advertised.

vdo_block_map_era_length doesn't appear to work either. Now, I realize im not exactly sure what a valid "vdo_block_map_era_length" alteration would be, but having the lvchange cmd report that it's now "corrupted" is another bug in this feature.


[root@hayes-01 ~]#  lvs -o vdo_block_map_era_length vdo_sanity/vdo_lv
  VDOBlockMapEraLength
                 16380

[root@hayes-01 ~]# lvchange --vdosettings vdo_block_map_era_length=16381 /dev/vdo_sanity/vdo_lv
  Logical volume vdo_sanity/vdo_lv changed.
  LV vdo_sanity/vdo_pool, segment 1 invalid: sets unsupported VDO block map era length for vdo-pool segment.
  Internal error: LV segments corrupted in vdo_pool.

More issues wrt to the online options use_compression, use_deduplication:

1. I assumed this was a typo, and that you meant vdo_compression and vdo_deduplication since lvs and lvchange both fail if you use use_* or vdo_use_*, but then I saw this in the warning "Invalid argument for VDO setting "vdo_use_compression"." So this appears broken as well.

[root@hayes-01 ~]# lvchange --vdosettings vdo_compression=enabled /dev/vdo_sanity/vdo_lv
  Invalid argument for VDO setting "vdo_use_compression".


[root@hayes-01 ~]# lvs -o vdo_deduplication vdo_sanity/vdo_lv
  VDODeduplication
                  
[root@hayes-01 ~]# lvs -o vdo_compression vdo_sanity/vdo_lv
  VDOCompression



2. Why are we adding and supporting another way to change the compression/dedupe when we already have this easier interface?

[root@hayes-01 ~]# lvchange --compression n vdo_sanity/vdo_lv
  Logical volume vdo_sanity/vdo_lv changed.
[root@hayes-01 ~]# lvchange --compression y vdo_sanity/vdo_lv
  Logical volume vdo_sanity/vdo_lv changed.

So now we have to either remember "vdo_compression=1" or "--compression y" depending on which interface we're using.

[root@hayes-01 ~]# lvchange --vdosettings vdo_compression=1 /dev/vdo_sanity/vdo_lv
  Logical volume vdo_sanity/vdo_lv changed.
[root@hayes-01 ~]# lvchange --vdosettings vdo_compression=0 /dev/vdo_sanity/vdo_lv
  Logical volume vdo_sanity/vdo_lv changed.
[root@hayes-01 ~]# lvchange --vdosettings vdo_compression=1 /dev/vdo_sanity/vdo_lv
  Logical volume vdo_sanity/vdo_lv changed.

[root@hayes-01 ~]# lvs -o vdo_compression vdo_sanity/vdo_lv
  VDOCompression
         enabled

Another issue:

None of the "VDOPOOL NO Lvchange  (only lvcreate/lvconvert)" options appear to work either.



[root@hayes-02 ~]# lvcreate --yes --type linear -n vdo_pool  -L 5G vdo_sanity  
  Logical volume "vdo_pool" created.

[root@hayes-02 ~]# lvconvert --yes --type vdo-pool -n vdo_lv --vdosettings 'vdo_index_memory_size=256' -V 20G vdo_sanity/vdo_pool
lvconvert: unrecognized option '--vdosettings'
  Error during parsing of command line.

[root@hayes-02 ~]# lvconvert --yes --type vdo-pool -n vdo_lv --vdosettings 'vdo_check_point_frequency=1' -V 20G vdo_sanity/vdo_pool
lvconvert: unrecognized option '--vdosettings'
  Error during parsing of command line.

[root@hayes-02 ~]# lvconvert --yes --type vdo-pool -n vdo_lv -V 20G vdo_sanity/vdo_pool
  WARNING: Converting logical volume vdo_sanity/vdo_pool to VDO pool volume with formating.
  THIS WILL DESTROY CONTENT OF LOGICAL VOLUME (filesystem etc.)
    The VDO volume can address 2 GB in 1 data slab.
    It can grow to address at most 16 TB of physical storage in 8192 slabs.
    If a larger maximum size might be needed, use bigger slabs.
  Logical volume "vdo_lv" created.
  Converted vdo_sanity/vdo_pool to VDO pool volume and created virtual vdo_sanity/vdo_lv VDO volume.

[root@hayes-02 ~]# lvconvert --vdosettings 'vdo_check_point_frequency=1' vdo_sanity/vdo_pool
lvconvert: unrecognized option '--vdosettings'
  Error during parsing of command line.

[root@hayes-02 ~]# lvconvert --vdosettings 'vdo_index_memory_size=256' vdo_sanity/vdo_pool
lvconvert: unrecognized option '--vdosettings'
  Error during parsing of command line.

Let's go by comments: 

Comment #12

Correct name of the setting here is: vdo_block_map_cache_size_mb
(can be shortened to blockmapcachesizemb - but suffix mb still needs to be there as is not option - but it could be eventually another sort of syntax sugar added later - not sure if it's worth yet..)

Comment #13

era length cannot be changed at all (not being listed as changable) - the reported error message is rather ulgy - current upstream does handle this case in nicer way already:

# lvchange --vdosettings 'vdo_check_point_frequency=1' vg/vpool0
  Cannot change VDO setting "vdo_check_point_frequency" in existing VDO pool.

But this can likely wait for next release (assuming it will hit RH9.1)

Comment #14

vdo_use_compression is there just for completeness - --vdosettings is just a 'nicer' way to handle rather ugly '--config ...'  interface which has lot of internal drawbacks inside lvm2 code whenever user is using this --config option - so since  this compression & deduplication can be already said by lvm.conf or any profiles - it's natural to have it as one of '--vdosettings' option.
And yes some of those option happen to have it's own dedicated lvm2 option --compression  && --deduplication -  as these do have potential to be possibly used with some other targets in the future - but with tons of other vdo setting options - it's possibly more easier to handle these changes with '--vdosettings' option  (but never say never - if there would be a big user request to add more of those as direct lvm2 option - it may happen - so far I do more believe users will like usage of some vdo profiles more for various workloads - and upstream made some progress in this direction already.
Fact  the syntax parses for  'config' settings is less advanced and just want  0/1  instead of 'syntax sugar advanced lvm2 option has been already commented - and if we figure out some 'compatible' parsing mechanism - it might be enhanced in future (but's highly non-trivial task)

Comment #15

'vdo_check_point_frequency' is problematic - it's 'gone' from current version of vdoformat tooling - so here lvm2 will need to 'ignore' any changes to this setting other then value 0  - so this is current bug in lvm2 handling that needs a fix.

vdo_index_memory_size again requires  _mb to be three in the option name -> vdo_index_memory_size_mb   should work.

Overlooked  another point from Comment #13    - which is now also handle with better more understandable error message in upstream code base:


# lvchange --vdosettings vdo_block_map_era_length=16381  vg/lvol0
  VDO block map era length 16381 is out of range [1..16380].

So error gives a user better understanding why it's failing -  as 16380 is max value for this setting.
Failing of lvchange in existing tested code base is correct - just the reported error was not ideal. It will be fixed with next release - unsure if the patches enhancing error message will get into this cycle ATM.

Re: Comment #12 and Comment #15:

I filed bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=2108227
https://bugzilla.redhat.com/show_bug.cgi?id=2108233

Re: Comment #13:

I filed bug:
https://bugzilla.redhat.com/show_bug.cgi?id=2108239

Re: Comment #14:

I filed bug:
https://bugzilla.redhat.com/show_bug.cgi?id=2108254i


"vdo_use_compression is there just for completeness"

The reality though, is that these don't fully work. Only the original way of setting this (vdo_compression) works, in that it can be viewed and set through the same argument.


# vdo_use_compression:  can NOT be viewed, can NOT be altered
[root@hayes-01 ~]# lvs  -o vdo_use_compression vdo_sanity/vdo_lv
   [...]
  Unrecognised field: vdo_use_compression

[root@hayes-01 ~]# lvchange --vdosettings 'vdo_use_compression = 1' vdo_sanity/vdo_pool 
  Unknown VDO setting "vdo_use_compression".


# use_compression:  can NOT be viewed, CAN be altered
[root@hayes-01 ~]# lvs  -o use_compression vdo_sanity/vdo_lv
   [...]
  Unrecognised field: use_compression

[root@hayes-01 ~]# lvchange --vdosettings 'use_compression = 1' vdo_sanity/vdo_pool 
  Logical volume vdo_sanity/vdo_pool changed.



# vdo_compression: CAN be viewed, CAN be altered
[root@hayes-01 ~]# lvs  -o vdo_compression vdo_sanity/vdo_lv
  VDOCompression
         enabled

[root@hayes-01 ~]# lvchange --vdosettings 'vdo_compression = 1' vdo_sanity/vdo_pool 
  Logical volume vdo_sanity/vdo_pool changed.

None of the many issue above, alone, are enough to pull this feature from 8.7/9.1, but I think all of them together is enough. I think documenting the release notes for this will be difficult given that many of these work only to set, or only to view, and that only a subset work both ways like advertised in comment #0 "So here is the current list of implemented support:" section.

That said, I will go with whatever devel decides with the list of provided caveats.

Yep - some logic can be nontrivial to get at first - so filled bugs will be somehow handled to improve the situation.

Just to put some 'other perspective' let's just explain this i.e. the property of VDO is really named 'vdo_block_map_cache_size'  - and with the suffix _mb it is used for configuration to make it clearly different from sizes in some other units (typical  sector 512b) used in those lvm conf files - so the user does not need to recalculate this large size himself and can use the  'MiB' units as more human readable number.

'lvs' does not 'suffer' from this  'config parser' limitation  (aka using only plain integer number) and can express/report the size in any configurable --unit number.

But for the completeness & user happiness we will add accompanying 'vdo_..._mb' lvs options showing the size as the integer number in MiB - as it might be in fact usable for scripting - so  it's a valid  RFE.

Similar with i.e.  'vdo_use_compression' -  which is then reported by  'vdo_compression'  reported state -
but it's not a big deal to add more 'aliases' - just 'lvs -o help' might need some more thinking how to make this listing more easily readable.

Saying that - I do not see it currently as a high prio issue for 8.7 that makes VDO filled BZ reports will not be all eventually fixed for this release - as the VDO got a missing functionality working - just some 'syntax sugar' completeness is lacking (while some of reported BZ are already fixed in upstream) - so we will see how much it will fit into 8.7 build.

Another issue to add to the list (use_sparse_index):
https://bugzilla.redhat.com/show_bug.cgi?id=2108326

Current upstream functionality should be matching documentation from comment 4.
(lvs needs to follow documented full names of vdo settings).

Depends to which level we want to backport this upgraded functionality to RHEL8 branch - may require larger patch series.

The bar for inclusion into rhel8.9 is quite high and I don't think this qualifies for the final rhel8 release. We can fix this in the rhel9 clone (bug 2070777).