Bug 1796677

(In reply to Corey Marthaler from comment #0)
> Description of problem:
>    dm-writecache settings
> 
>        Tunable parameters can be passed to the dm-writecache kernel module
> using the --cachesettings option when caching is started, e.g.
> 
>        · high_watermark = <count>
>          Start writeback when the number of used blocks reach this watermark
> 
>        · low_watermark = <count>
>          Stop writeback when the number of used blocks drops below this
> watermark
> 

Not sure who documented this.  Or if lvm2 meant to present a different interface than the kernel interface?  but the kernel code expects percentages (0-100):

                } else if (!strcasecmp(string, "high_watermark") && opt_params >= 1) {
                        string = dm_shift_arg(&as), opt_params--;
                        if (sscanf(string, "%d%c", &high_wm_percent, &dummy) != 1)
                                goto invalid_optional;
                        if (high_wm_percent < 0 || high_wm_percent > 100)
                                goto invalid_optional;
                        wc->high_wm_percent_set = true;
                } else if (!strcasecmp(string, "low_watermark") && opt_params >= 1) {
                        string = dm_shift_arg(&as), opt_params--;
                        if (sscanf(string, "%d%c", &low_wm_percent, &dummy) != 1)
                                goto invalid_optional;
                        if (low_wm_percent < 0 || low_wm_percent > 100)
                                goto invalid_optional;
                        wc->low_wm_percent_set = true;
                }


> # HIGH
> [root@hayes-02 ~]# lvconvert --yes --type writecache --cachesettings
> 'high_watermark=1470' --cachevol writecache_sanity/pool
> writecache_sanity/display_writecache
...
> # LOW
> [root@hayes-02 ~]# lvconvert --yes --type writecache --cachesettings
> 'low_watermark=1470' --cachevol writecache_sanity/pool
> writecache_sanity/display_writecache

provided value needs to be >= 0 and <= 100.  Again, they are percentages.

I documented it incorrectly, man page fixed here:
https://sourceware.org/git/?p=lvm2.git;a=commit;h=64a82a1c79a4aaef5359150f960932aee4df1982

Just to be clear, is QA to verify the man page change only? I see that LVM still doesn't check of the values are between 0-100.

Man page is fixed:

lvmcache(7)
       
       Tunable options are:

       · high_watermark = <percent>

         Start writeback when the writecache usage reaches this percent (0-100).

       · low_watermark = <percent>

         Stop writeback when the writecache usage reaches this percent (0-100)

verifying values between 0 and 100:
https://sourceware.org/git/?p=lvm2.git;a=commit;h=84eab461c843e2018b6ff73a9af8140e27cde391

# lvconvert --type writecache --cachevol fast --cachesettings high_watermark=101 test/main
  Invalid setting: high_watermark
  Invalid writecache settings.

# lvconvert --type writecache --cachevol fast --cachesettings low_watermark=-1000 test/main
  Invalid setting: low_watermark
  Invalid writecache settings.

Marian, 

Can we get the above mentioned fix included in the official build for this bug (as it's not yet in lvm2-2.03.08-2.el8), Thanks.

[root@hayes-02 ~]# lvs
  LV              VG                Attr       LSize Pool Origin Data%  Meta%  Move Log Cpy%Sync Convert
  cworigin        writecache_sanity -wi------- 4.00g                                                    
  test_writecache writecache_sanity -wi------- 2.00g
                                                    
[root@hayes-02 ~]# lvconvert --test --yes --type writecache --cachesettings 'high_watermark=101' --cachevol writecache_sanity/test_writecache writecache_sanity/cworigin
  TEST MODE: Metadata will NOT be updated and volumes will not be (de)activated.
  Logical volume writecache_sanity/cworigin now has write cache.
[root@hayes-02 ~]# lvconvert --yes --type writecache --cachesettings 'high_watermark=101' --cachevol writecache_sanity/test_writecache writecache_sanity/cworigin
  Logical volume writecache_sanity/cworigin now has write cache.

[root@hayes-02 ~]# lvchange -ay writecache_sanity/cworigin
  device-mapper: reload ioctl on  (253:2) failed: Invalid argument

Feb 25 11:02:06 hayes-02 kernel: device-mapper: table: 253:2: writecache: Invalid optional argument
Feb 25 11:02:06 hayes-02 kernel: device-mapper: ioctl: error adding target to table

(In reply to David Teigland from comment #5)
> verifying values between 0 and 100:
> https://sourceware.org/git/?p=lvm2.git;a=commit;
> h=84eab461c843e2018b6ff73a9af8140e27cde391
> 
> # lvconvert --type writecache --cachevol fast --cachesettings
> high_watermark=101 test/main
>   Invalid setting: high_watermark
>   Invalid writecache settings.
> 
> # lvconvert --type writecache --cachevol fast --cachesettings
> low_watermark=-1000 test/main
>   Invalid setting: low_watermark
>   Invalid writecache settings.

I didn't see an accompanying test suite addition for this... could we get one?

(In reply to Corey Marthaler from comment #6)
> Marian, 
> 
> Can we get the above mentioned fix included in the official build for this
> bug (as it's not yet in lvm2-2.03.08-2.el8), Thanks.

No problem, but we do need an exception now, and I am asking for one.

The fix itself is a no risk usability improvement, just rejecting invalid values.

Fix verified in the latest rpms.


kernel-4.18.0-211.el8    BUILT: Thu Jun  4 03:33:39 CDT 2020
lvm2-2.03.09-2.el8    BUILT: Fri May 29 11:29:58 CDT 2020
lvm2-libs-2.03.09-2.el8    BUILT: Fri May 29 11:29:58 CDT 2020
lvm2-dbusd-2.03.09-2.el8    BUILT: Fri May 29 11:32:49 CDT 2020
lvm2-lockd-2.03.09-2.el8    BUILT: Fri May 29 11:29:58 CDT 2020
boom-boot-1.1-1.el8    BUILT: Thu May 14 08:11:56 CDT 2020
device-mapper-1.02.171-2.el8    BUILT: Fri May 29 11:29:58 CDT 2020
device-mapper-libs-1.02.171-2.el8    BUILT: Fri May 29 11:29:58 CDT 2020
device-mapper-event-1.02.171-2.el8    BUILT: Fri May 29 11:29:58 CDT 2020
device-mapper-event-libs-1.02.171-2.el8    BUILT: Fri May 29 11:29:58 CDT 2020



1B. Test that a highwater mark needs to be higher than low watermark
lvconvert --test --yes --type writecache --cachesettings 'low_watermark=56 high_watermark=3' --cachevol writecache_sanity/test_writecache writecache_sanity/cworigin
  TEST MODE: Metadata will NOT be updated and volumes will not be (de)activated.
  High watermark must be greater than low watermark.
  Invalid writecache settings.
lvconvert --test --yes --type writecache --cachesettings 'low_watermark=56 high_watermark=56' --cachevol writecache_sanity/test_writecache writecache_sanity/cworigin
  TEST MODE: Metadata will NOT be updated and volumes will not be (de)activated.
  High watermark must be greater than low watermark.
  Invalid writecache settings.
lvconvert --test --yes --type writecache --cachesettings 'low_watermark=56 high_watermark=57' --cachevol writecache_sanity/test_writecache writecache_sanity/cworigin
  TEST MODE: Metadata will NOT be updated and volumes will not be (de)activated.

1C. Test that a low water mark shouldnt be allowed if over 100%
lvconvert --test --yes --type writecache --cachesettings 'low_watermark=156' --cachevol writecache_sanity/test_writecache writecache_sanity/cworigin
  TEST MODE: Metadata will NOT be updated and volumes will not be (de)activated.
  Invalid setting: low_watermark
  Invalid writecache settings.

1D. Test that a low water mark is numeric
lvconvert --test --yes --type writecache --cachesettings 'low_watermark=abc' --cachevol writecache_sanity/test_writecache writecache_sanity/cworigin
  TEST MODE: Metadata will NOT be updated and volumes will not be (de)activated.
  Invalid setting: low_watermark
  Invalid writecache settings.

1E. Test that a high water mark shouldnt be allowed if over 100%
lvconvert --test --yes --type writecache --cachesettings 'high_watermark=345' --cachevol writecache_sanity/test_writecache writecache_sanity/cworigin
  TEST MODE: Metadata will NOT be updated and volumes will not be (de)activated.
  Invalid setting: high_watermark
  Invalid writecache settings.

1F. Test that a high water mark is numeric
lvconvert --test --yes --type writecache --cachesettings 'high_watermark=DEF' --cachevol writecache_sanity/test_writecache writecache_sanity/cworigin
  TEST MODE: Metadata will NOT be updated and volumes will not be (de)activated.
  Invalid setting: high_watermark
  Invalid writecache settings.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (lvm2 bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2020:4546

The needinfo request[s] on this closed bug have been removed as they have been unresolved for 500 days