[lvm-devel] [PATCH] automatic snapshot extension with dmeventd (BZ 427298)

[Petr Rockai]

Zdenek Kabelac <zkabelac at redhat.com> writes:

> I don't like these hard coded numbers - maybe it should be connected to
> PATH_MAX * xyz + abc, or maybe just use dm_asprintf ?

Unlimited allocations are not allowed in dmeventd anyway, so you have to
cap it somehow. PATH_MAX is probably too big. As long as the number is
in just one place, I don't see the problem (the rest is referring to
sizeof of the array instead).

>> --- old-snapshot-monitoring/tools/lvresize.c	2010-10-05 19:42:32.000000000 +0200
>> +++ new-snapshot-monitoring/tools/lvresize.c	2010-10-05 19:42:32.000000000 +0200
>> @@ -196,34 +197,41 @@ static int _lvresize_params(struct cmd_c
> ...
>> +
>> +		if (arg_count(cmd, extents_ARG)) {
>> +			lp->extents = arg_uint_value(cmd, extents_ARG, 0);
>> +			lp->sign = arg_sign_value(cmd, extents_ARG, SIGN_NONE);
>> +			lp->percent = arg_percent_value(cmd, extents_ARG, PERCENT_NONE);
>> +		}
>> +
>> +		/* Size returned in kilobyte units; held in sectors */
>> +		if (arg_count(cmd, size_ARG)) {
>> +			lp->size = arg_uint64_value(cmd, size_ARG, UINT64_C(0));
>
> I know it's just code move - but we could strip this unneeded UINT64_C.
Ok. (The attached patch fixes this.)

>> +static int _adjust_policy_params(struct cmd_context *cmd,
>> +				 struct logical_volume *lv, struct lvresize_params *lp)
>> +{
>> +	float percent;
>> +	percent_range_t range;
>> +	int policy_threshold, policy_amount;
>> +
>> +	policy_threshold =
>> +		find_config_tree_int(cmd, "activation/snapshot_autoextend_threshold",
>> +				     DEFAULT_SNAPSHOT_AUTOEXTEND_THRESHOLD);
>> +	policy_amount =
>> +		find_config_tree_int(cmd, "activation/snapshot_autoextend_percent",
>> +				     DEFAULT_SNAPSHOT_AUTOEXTEND_PERCENT);
>> +
>> +	if (policy_threshold >= 100)
>> +		return 1; /* nothing to do */
>
> For some really large sizes - even 1% could be a huge amount of disk space -
> so maybe some users would like to see floating number support here?

I'd open a separate bug for that, if you really think so. I tend to
disagree. (Btw. there's no single float config option in LVM today --
even though there is theoretical support for that, it's probably not
tested at all.)

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmeventd-snapshot-extend.diff
Type: text/x-diff
Size: 15258 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20101015/0bde6fa3/attachment.bin>