args: use arg parsing function for region size

Consolidate the validation of the region size arg
in a new arg parsing function.

diff --git a/lib/config/config_settings.h b/lib/config/config_settings.h
index 1d9603fb5ef06534f79474f3f1c02cd375ccb1dd..2b76526e3b4d72bdf65f39210241538a7d924461 100644 (file)
--- a/lib/config/config_settings.h
+++ b/lib/config/config_settings.h
@@ -1221,14 +1221,15 @@ cfg_array(activation_read_only_volume_list_CFG, "read_only_volume_list", activat
        "read_only_volume_list = [ \"vg1\", \"vg2/lvol1\", \"@tag1\", \"@*\" ]\n"
        "#\n")
 
-cfg(activation_mirror_region_size_CFG, "mirror_region_size", activation_CFG_SECTION, 0, CFG_TYPE_INT, DEFAULT_RAID_REGION_SIZE, vsn(1, 0, 0), NULL, vsn(2, 2, 99),
+ cfg(activation_mirror_region_size_CFG, "mirror_region_size", activation_CFG_SECTION, 0, CFG_TYPE_INT, DEFAULT_RAID_REGION_SIZE, vsn(1, 0, 0), NULL, vsn(2, 2, 99),
        "This has been replaced by the activation/raid_region_size setting.\n",
-        "Size in KiB of each copy operation when mirroring.\n")
+       "Size in KiB of each raid or mirror synchronization region.\n")
 
 cfg(activation_raid_region_size_CFG, "raid_region_size", activation_CFG_SECTION, 0, CFG_TYPE_INT, DEFAULT_RAID_REGION_SIZE, vsn(2, 2, 99), NULL, 0, NULL,
        "Size in KiB of each raid or mirror synchronization region.\n"
-       "For raid or mirror segment types, this is the amount of data that is\n"
-       "copied at once when initializing, or moved at once by pvmove.\n")
+       "The clean/dirty state of data is tracked for each region.\n"
+       "The value is rounded down to a power of two if necessary, and\n"
+       "is ignored if it is not a multiple of the machine memory page size.\n")
 
 cfg(activation_error_when_full_CFG, "error_when_full", activation_CFG_SECTION, CFG_DEFAULT_COMMENTED, CFG_TYPE_BOOL, DEFAULT_ERROR_WHEN_FULL, vsn(2, 2, 115), NULL, 0, NULL,
        "Return errors if a thin pool runs out of space.\n"

diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c
index bc599deb4177d7f9c8d2f865044e6dec6470cb0b..d8f869d7b1f3ce7c0b66e1b134a8e065a512bcf4 100644 (file)
--- a/lib/metadata/lv_manip.c
+++ b/lib/metadata/lv_manip.c
@@ -712,6 +712,7 @@ static int _round_down_pow2(int r)
 
 int get_default_region_size(struct cmd_context *cmd)
 {
+       int pagesize = lvm_getpagesize();
        int region_size = _get_default_region_size(cmd);
 
        if (!is_power_of_2(region_size)) {
@@ -720,6 +721,12 @@ int get_default_region_size(struct cmd_context *cmd)
                            region_size / 2);
        }
 
+       if (region_size % (pagesize >> SECTOR_SHIFT)) {
+               region_size = DEFAULT_RAID_REGION_SIZE * 2;
+               log_verbose("Using default region size %u kiB (multiple of page size).",
+                           region_size / 2);
+       }
+
        return region_size;
 }
 

diff --git a/tools/args.h b/tools/args.h
index a481bc5230eae4ed57e76cc19c92445db562bc31..b33df9af29fbd1ad487fbbf3bf61acffdc431b54 100644 (file)
--- a/tools/args.h
+++ b/tools/args.h
@@ -1212,10 +1212,8 @@ arg(resizefs_ARG, 'r', "resizefs", 0, 0, 0,
 /* Not used */
 arg(reset_ARG, 'R', "reset", 0, 0, 0, NULL)
 
-arg(regionsize_ARG, 'R', "regionsize", sizemb_VAL, 0, 0,
-    "A mirror is divided into regions of this size, and the mirror log\n"
-    "uses this granularity to track which regions are in sync.\n"
-    "(This can be used with the \"mirror\" type, not the \"raid1\" type.)\n")
+arg(regionsize_ARG, 'R', "regionsize", regionsize_VAL, 0, 0,
+    "Size of each raid or mirror synchronization region.\n")
 
 arg(physicalextentsize_ARG, 's', "physicalextentsize", sizemb_VAL, 0, 0,
     "#vgcreate\n"

diff --git a/tools/command-lines.in b/tools/command-lines.in
index 6743b798944fa07299e9c3c4f9ce77740b07d16b..1a9a796bf0e953319377f87df93befcdc2acf0f8 100644 (file)
--- a/tools/command-lines.in
+++ b/tools/command-lines.in
@@ -317,7 +317,7 @@ RULE: all not LV_thinpool LV_cachepool
 ---
 
 OO_LVCONVERT_RAID: --mirrors SNumber, --stripes_long Number,
---stripesize SizeKB, --regionsize SizeMB, --interval Number
+--stripesize SizeKB, --regionsize RegionSize, --interval Number
 
 OO_LVCONVERT_POOL: --poolmetadata LV, --poolmetadatasize SizeMB,
 --poolmetadataspare Bool, --readahead Readahead, --chunksize SizeKB
@@ -364,7 +364,7 @@ ID: lvconvert_raid_types
 DESC: Convert LV to raid1 or mirror, or change number of mirror images.
 RULE: all not lv_is_locked lv_is_pvmove
 
-lvconvert --regionsize SizeMB LV_raid
+lvconvert --regionsize RegionSize LV_raid
 OO: OO_LVCONVERT
 ID: lvconvert_change_region_size
 DESC: Change the region size of an LV.
@@ -655,7 +655,7 @@ OO_LVCREATE_POOL: --poolmetadatasize SizeMB, --poolmetadataspare Bool, --chunksi
 OO_LVCREATE_THIN: --discards Discards, --errorwhenfull Bool
 
 OO_LVCREATE_RAID: --mirrors SNumber, --stripes Number, --stripesize SizeKB,
---regionsize SizeMB, --minrecoveryrate SizeKB, --maxrecoveryrate SizeKB
+--regionsize RegionSize, --minrecoveryrate SizeKB, --maxrecoveryrate SizeKB
 
 ---
 
@@ -712,7 +712,7 @@ DESC: Create a striped LV (infers --type striped).
 ---
 
 lvcreate --type mirror --size SizeMB VG
-OO: --mirrors SNumber, --mirrorlog MirrorLog, --regionsize SizeMB, --stripes Number, OO_LVCREATE
+OO: --mirrors SNumber, --mirrorlog MirrorLog, --regionsize RegionSize, --stripes Number, OO_LVCREATE
 OP: PV ...
 ID: lvcreate_mirror
 DESC: Create a mirror LV (also see --type raid1).

diff --git a/tools/command.c b/tools/command.c
index 59c9ea0072a3b486a5fb9e1583f5e022fb780b67..68566fafc3818efb48475fefc39a544aa1d2191f 100644 (file)
--- a/tools/command.c
+++ b/tools/command.c
@@ -75,6 +75,7 @@ static inline int segtype_arg(struct cmd_context *cmd, struct arg_values *av) {
 static inline int alloc_arg(struct cmd_context *cmd, struct arg_values *av) { return 0; }
 static inline int locktype_arg(struct cmd_context *cmd, struct arg_values *av) { return 0; }
 static inline int readahead_arg(struct cmd_context *cmd, struct arg_values *av) { return 0; }
+static inline int regionsize_arg(struct cmd_context *cmd, struct arg_values *av) { return 0; }
 static inline int vgmetadatacopies_arg(struct cmd_context *cmd __attribute__((unused)), struct arg_values *av) { return 0; }
 static inline int pvmetadatacopies_arg(struct cmd_context *cmd __attribute__((unused)), struct arg_values *av) { return 0; }
 static inline int metadatacopies_arg(struct cmd_context *cmd __attribute__((unused)), struct arg_values *av) { return 0; }

diff --git a/tools/lvconvert.c b/tools/lvconvert.c
index 4d6b1c9b1d1460f3b5a79a9a5ba89b4642e850c9..502d614e786798e477cd3e9d91c9334cc5fa5ba9 100644 (file)
--- a/tools/lvconvert.c
+++ b/tools/lvconvert.c
@@ -146,8 +146,6 @@ static int _read_conversion_type(struct cmd_context *cmd,
 static int _read_params(struct cmd_context *cmd, struct lvconvert_params *lp)
 {
        const char *vg_name = NULL;
-       int region_size;
-       int pagesize = lvm_getpagesize();
 
        if (!_read_conversion_type(cmd, lp))
                return_0;
@@ -249,45 +247,10 @@ static int _read_params(struct cmd_context *cmd, struct lvconvert_params *lp)
                                return 0;
                        }
 
-                       /*
-                        * --regionsize is only valid if converting an LV into a mirror.
-                        * Checked when we know the state of the LV being converted.
-                        */
-                       if (arg_is_set(cmd, regionsize_ARG)) {
-                               if (arg_sign_value(cmd, regionsize_ARG, SIGN_NONE) ==
-                                           SIGN_MINUS) {
-                                       log_error("Negative regionsize is invalid.");
-                                       return 0;
-                               }
+                       if (arg_is_set(cmd, regionsize_ARG))
                                lp->region_size = arg_uint_value(cmd, regionsize_ARG, 0);
-                       } else {
-                               region_size = get_default_region_size(cmd);
-                               if (region_size < 0) {
-                                       log_error("Negative regionsize in "
-                                                 "configuration file is invalid.");
-                                       return 0;
-                               }
-                               lp->region_size = region_size;
-                       }
-
-                       if (lp->region_size % (pagesize >> SECTOR_SHIFT)) {
-                               log_error("Region size (%" PRIu32 ") must be "
-                                         "a multiple of machine memory "
-                                         "page size (%d).",
-                                         lp->region_size, pagesize >> SECTOR_SHIFT);
-                               return 0;
-                       }
-
-                       if (!is_power_of_2(lp->region_size)) {
-                               log_error("Region size (%" PRIu32
-                                         ") must be a power of 2.", lp->region_size);
-                               return 0;
-                       }
-
-                       if (!lp->region_size) {
-                               log_error("Non-zero region size must be supplied.");
-                               return 0;
-                       }
+                       else
+                               lp->region_size = get_default_region_size(cmd);
 
                        /* FIXME man page says in one place that --type and --mirrors can't be mixed */
                        if (lp->mirrors_supplied && !lp->mirrors)

diff --git a/tools/lvcreate.c b/tools/lvcreate.c
index 051258535c3c05e6f67574a441564fbe446e15d5..cd31c996919707a8fe44b3e934a863b654d1142b 100644 (file)
--- a/tools/lvcreate.c
+++ b/tools/lvcreate.c
@@ -545,7 +545,6 @@ static int _read_raid_params(struct cmd_context *cmd,
 static int _read_mirror_and_raid_params(struct cmd_context *cmd,
                                        struct lvcreate_params *lp)
 {
-       int pagesize = lvm_getpagesize();
        unsigned max_images;
 
        if (seg_is_raid(lp)) {
@@ -616,19 +615,6 @@ static int _read_mirror_and_raid_params(struct cmd_context *cmd,
                return 0;
        }
 
-       if (!is_power_of_2(lp->region_size)) {
-               log_error("Region size (%" PRIu32 ") must be a power of 2",
-                         lp->region_size);
-               return 0;
-       }
-
-       if (lp->region_size % (pagesize >> SECTOR_SHIFT)) {
-               log_error("Region size (%" PRIu32 ") must be a multiple of "
-                         "machine memory page size (%d)",
-                         lp->region_size, pagesize >> SECTOR_SHIFT);
-               return 0;
-       }
-
        if (seg_is_mirror(lp) && !_read_mirror_params(cmd, lp))
                 return_0;
 

diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c
index 4fda356061cb3e6f5ef19245dc68999531ecb83d..58d1a443c70e540fedceb4162864ffacba024fa9 100644 (file)
--- a/tools/lvmcmdline.c
+++ b/tools/lvmcmdline.c
@@ -797,6 +797,45 @@ int readahead_arg(struct cmd_context *cmd __attribute__((unused)), struct arg_va
        return 1;
 }
 
+int regionsize_arg(struct cmd_context *cmd, struct arg_values *av)
+{
+       int pagesize = lvm_getpagesize();
+       uint32_t num;
+
+       if (!_size_arg(cmd, av, 2048, 0))
+               return 0;
+
+       if (av->sign == SIGN_MINUS) {
+               log_error("Region size may not be negative.");
+               return 0;
+       }
+
+       if (av->ui64_value > UINT32_MAX) {
+               log_error("Region size is too big (max %u).", UINT32_MAX);
+               return 0;
+       }
+
+       num = av->ui_value;
+
+       if (!num) {
+               log_error("Region size may not be zero.");
+               return 0;
+       }
+
+       if (num % (pagesize >> SECTOR_SHIFT)) {
+               log_error("Region size must be a multiple of machine memory page size (%d bytes).",
+                         pagesize);
+               return 0;
+       }
+
+       if (!is_power_of_2(num)) {
+               log_error("Region size must be a power of 2.");
+               return 0;
+       }
+
+       return 1;
+}
+
 /*
  * Non-zero, positive integer, "all", or "unmanaged"
  */

diff --git a/tools/tools.h b/tools/tools.h
index 00476d106dcd8ee02618ed36fa245783e80ef763..b3858a080786a6f506f7ae861e25dce42c282b8b 100644 (file)
--- a/tools/tools.h
+++ b/tools/tools.h
@@ -200,6 +200,7 @@ int segtype_arg(struct cmd_context *cmd, struct arg_values *av);
 int alloc_arg(struct cmd_context *cmd, struct arg_values *av);
 int locktype_arg(struct cmd_context *cmd, struct arg_values *av);
 int readahead_arg(struct cmd_context *cmd, struct arg_values *av);
+int regionsize_arg(struct cmd_context *cmd, struct arg_values *av);
 int vgmetadatacopies_arg(struct cmd_context *cmd __attribute__((unused)), struct arg_values *av);
 int pvmetadatacopies_arg(struct cmd_context *cmd __attribute__((unused)), struct arg_values *av);
 int metadatacopies_arg(struct cmd_context *cmd __attribute__((unused)), struct arg_values *av);

diff --git a/tools/vals.h b/tools/vals.h
index 12d315f80facd2485ebc6446dda2df4e2fea4874..38029aadc133eeae320f7cdfef4f3a39b4de9817 100644 (file)
--- a/tools/vals.h
+++ b/tools/vals.h
@@ -92,6 +92,10 @@
  * --size and other option args treat upper/lower letters
  * the same, all as 1024 SI base.  For this reason, we
  * should avoid suggesting the upper case letters.
+ *
+ * FIXME: negative numbers should be automatically rejected
+ * for anything but int_arg_with_sign(), e.g.
+ * size_mb_arg() should reject a negative number.
  */
 
 val(none_VAL, NULL, "None", "ERR")             /* unused, for enum value 0 */
@@ -113,6 +117,7 @@ val(discards_VAL, discards_arg, "Discards", "passdown|nopassdown|ignore")
 val(mirrorlog_VAL, mirrorlog_arg, "MirrorLog", "core|disk")
 val(sizekb_VAL, size_kb_arg, "SizeKB", "Number[k|unit]")
 val(sizemb_VAL, size_mb_arg, "SizeMB", "Number[m|unit]")
+val(regionsize_VAL, regionsize_arg, "RegionSize", "Number[m|unit]")
 val(numsigned_VAL, int_arg_with_sign, "SNumber", "[+|-]Number")
 val(numsignedper_VAL, int_arg_with_sign_and_percent, "SNumberP", "[+|-]Number[%VG|%PVS|%FREE]")
 val(permission_VAL, permission_arg, "Permission", "rw|r")