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

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

[Petr Rockai]

Hi,

the attached patch adds support for automatic snapshot extension. It
uses 2 configuration variables (in lvm.conf):

    # 'snapshot_extend_threshold' and 'snapshot_extend_amount' define how to
    # handle automatic snapshot extension. The former defines when the snapshot
    # should be extended: when its space usage exceeds this many percent. The
    # latter defines how much extra space should be allocated for the snapshot,
    # in percent of its current size.
    #
    # For example, if you set snapshot_extend_threshold to 70 and
    # snapshot_extend_amount to 20, whenever a snapshot exceeds 70% usage, it
    # will be extended by another 20%. For a 1G snapshot, using 700M usage will
    # trigger a resize to 1.2G. When the usage exceeds 840M, the snapshot will
    # be extended to 1.44G, and so on.
    #
    # Setting snapshot_extend_threshold to 100 disables automatic extensions.

    snapshot_extend_threshold = 100
    snapshot_extend_amount = 20

It works by calling lvextend --use-policies every time that the snapshot
use grows by a 5% step, starting at 50% (i.e. it checks at 50, 55, 60,
... 95). The implementation of lvextend --use-policies checks the
configuration options and if the threshold is hit, it extends the
volume. (On a second thought, this 50% should be either relaxed (to 5)
or documented in lvm.conf.)

The patch comes with two automated tests, one testing lvextend
--use-policies directly (test/t-lvextend-snapshot-policy.sh), another
for the dmeventd integration (test/t-lvextend-snapshot-dmeventd.sh).

The latter is somewhat suboptimal, since dmeventd only checks the
snapshot every 10 seconds, so it takes a bit longish. This can be fixed
later though, shouldn't be a real problem for anything.

Yours,
   Petr.

PS: I'll look at the related BZ 189462 next (umount when the snapshot is
invalidated). Shouldn't be too hard, it just needs to find the right
mount point, presumably by consulting /proc/mounts, and use umount2 with
MNT_FORCE and MNT_DETACH. It's probably better not to touch (nor trust)
/etc/mtab.

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

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

[Petr Rockai]

Attaching a revised patch, addressing couple of points raised on IRC by
Alasdair. The setting names have changed, I have cleaned up defaults to
use the standard mechanism and fixed couple of coding style points.

The tests still pass.

Yours,
   Petr.

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

[PATCH] let dmeventd unmount invalid snapshots (BZ 189462)

[Petr Rockai]

Hi,

this is the second part of the snapshot monitoring stuff for
dmeventd. This one will remove mounts of a snapshot when it is
invalidated.

There are some trade-offs involved. I have opted for using an umount
syscall directly since it's simpler and more robust. The downside is
that /etc/mtab will go out of sync and people may actually think that
the snapshot is still mounted (mount(8) just prints mtab without
thinking twice about it). We could just fork off umount(8) instead, but
that can fail in new and interesting ways.

The other possible issue with the patch is that I am not sure whether
it's actually safe to read /proc/mounts incrementally, with possibly
intervening mount/umount calls. It *seems* to be, but a confirmation
would be nice.

The code comes (as usual) with an automated test:
test/t-snapshot-autoumount-dmeventd.sh

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmeventd-snapshot-umount.diff
Type: text/x-diff
Size: 4941 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20101007/2686b3ab/attachment.bin>

[PATCH] let dmeventd unmount invalid snapshots (BZ 189462)

[Alasdair G Kergon]

On Thu, Oct 07, 2010 at 03:10:43PM +0200, Peter Rockai wrote:
> There are some trade-offs involved. I have opted for using an umount
> syscall directly since it's simpler and more robust. The downside is
> that /etc/mtab will go out of sync and people may actually think that

I'm not keen on leaving /etc/mtab out-of-date, or missing out anything
else that /bin/umount might actually do (with an errored-out device).
Would this patch need an update to the selinux context to allow umount?

> the snapshot is still mounted (mount(8) just prints mtab without
> thinking twice about it). We could just fork off umount(8) instead, but
> that can fail in new and interesting ways.

Yes, even if moved outside the locking.  But even umount2() within
the lock might have deadlock possibilities, as might syslog() as
you point out.

The options/problems here need a bit more investigation.
 
> The other possible issue with the patch is that I am not sure whether
> it's actually safe to read /proc/mounts incrementally, with possibly
 
kabi found problems like that with another /proc file.

Alasdair

[PATCH] let dmeventd unmount invalid snapshots (BZ 189462)

[Petr Rockai]

Hi,

Alasdair G Kergon <agk@redhat.com> writes:
> On Thu, Oct 07, 2010 at 03:10:43PM +0200, Peter Rockai wrote:
>> There are some trade-offs involved. I have opted for using an umount
>> syscall directly since it's simpler and more robust. The downside is
>> that /etc/mtab will go out of sync and people may actually think that
>
> I'm not keen on leaving /etc/mtab out-of-date, or missing out anything
> else that /bin/umount might actually do (with an errored-out device).
> Would this patch need an update to the selinux context to allow umount?

I don't know about selinux. Anyhow, I think my plan is to fork off
/bin/umount asynchronously (i.e. outside of the dmeventd lock). That's
probably the safest possible approach I can think of, at least with our
current design. It will need a bit of thinking, but it should be doable.

>> the snapshot is still mounted (mount(8) just prints mtab without
>> thinking twice about it). We could just fork off umount(8) instead, but
>> that can fail in new and interesting ways.
>
> Yes, even if moved outside the locking.  But even umount2() within
> the lock might have deadlock possibilities, as might syslog() as
> you point out.

I will tackle that (syslog) separately.

>> The other possible issue with the patch is that I am not sure whether
>> it's actually safe to read /proc/mounts incrementally, with possibly
>
> kabi found problems like that with another /proc file.

Ok, that arguably sucks. Nevertheless, from reading the kernel code for
that, it seems that while the contents may change in between reads, the
results are still well defined: adding lines to the buffer is done
atomically in the kernel, so we should never get a garbled line. What
could happen is that we encounter the same line twice, or that we miss
something that was there when we were opening the file but was no longer
there when we got to that point in it. Neither should be a serious
problem.

That behaviour depends on the proc file implementation using the
seq_file mechanism, which may not be the case with all files. It is used
for all the mount-related files though (mounts, self/mounts,
self/mountinfo).

Yours,
   Petr.

[PATCH] let dmeventd unmount invalid snapshots (BZ 189462)

[Petr Rockai]

Hi again,

the somewhat delayed re-cast of the auto-umount patch is
attached. Changes since the last time:

- the umount itself is now outside the dmeventd lvm2 lock, which should
  mitigate the deadlock opportunities introduced by the umount
- we fork & exec real /bin/umount now, meaning that /etc/mtab is updated
  (well, if umount manages to update it, that is)

Yours,
   Petr.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmeventd-snapshot-umount.diff
Type: text/x-diff
Size: 6614 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20101029/94d4053b/attachment.bin>

[PATCH] let dmeventd unmount invalid snapshots (BZ 189462)

[Petr Rockai]

Petr Rockai <me@mornfall.net> writes:
> the somewhat delayed re-cast of the auto-umount patch is attached.
... and now a version that addresses Alasdair's IRC comments.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmeventd-snapshot-umount.diff
Type: text/x-diff
Size: 6905 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20101029/bbf49bc8/attachment.bin>

[PATCH] let dmeventd unmount invalid snapshots (BZ 189462)

[Zdenek Kabelac]

Dne 29.10.2010 13:49, Petr Rockai napsal(a):
> Hi again,
> 
> the somewhat delayed re-cast of the auto-umount patch is
> attached. Changes since the last time:
> 
> - the umount itself is now outside the dmeventd lvm2 lock, which should
>   mitigate the deadlock opportunities introduced by the umount
> - we fork & exec real /bin/umount now, meaning that /etc/mtab is updated
>   (well, if umount manages to update it, that is)
> 

Looks like a bit of problem for application which locks itself into memory, to
proceed all the time. Though currently I do not have good solution for this
problem in mind - so it's probably to have at least something in this moment.

Zdenek

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

[Zdenek Kabelac]

Dne 5.10.2010 19:44, Petr Rockai napsal(a):
> Attaching a revised patch, addressing couple of points raised on IRC by
> Alasdair. The setting names have changed, I have cleaned up defaults to
> use the standard mechanism and fixed couple of coding style points.
> 
> The tests still pass.
> 
> Yours,
>    Petr.
> 
> 
> 
> dmeventd-snapshot-extend.diff
> 
> 
> diff -rN -u -p old-snapshot-monitoring/daemons/dmeventd/plugins/snapshot/dmeventd_snapshot.c new-snapshot-monitoring/daemons/dmeventd/plugins/snapshot/dmeventd_snapshot.c
> --- old-snapshot-monitoring/daemons/dmeventd/plugins/snapshot/dmeventd_snapshot.c	2010-10-05 19:42:32.000000000 +0200
> +++ new-snapshot-monitoring/daemons/dmeventd/plugins/snapshot/dmeventd_snapshot.c	2010-10-05 19:42:32.000000000 +0200
> @@ -26,8 +26,10 @@
>  
>  /* First warning when snapshot is 80% full. */
>  #define WARNING_THRESH 80
> -/* Further warnings at 85%, 90% and 95% fullness. */
> -#define WARNING_STEP 5
> +/* Run a check every 5%. */
> +#define CHECK_STEP 5
> +/* Do not bother checking snapshots less than 50% full. */
> +#define CHECK_MINIMUM 50
>  
>  struct snap_status {
>  	int invalid;
> @@ -69,6 +71,28 @@ static void _parse_snapshot_params(char 
>  	status->max = atoi(p);
>  }
>  
> +static int _extend(const char *device)
> +{
> +	char *vg = NULL, *lv = NULL, *layer = NULL;
> +	char cmd_str[1024];

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


> +	int r = 0;
> +
> +	if (!dm_split_lvm_name(dmeventd_lvm2_pool(), device, &vg, &lv, &layer)) {
> +		syslog(LOG_ERR, "Unable to determine VG name from %s.", device);
> +		return 0;
> +	}
> +	if (sizeof(cmd_str) <= snprintf(cmd_str, sizeof(cmd_str),
> +					"lvextend --use-policies %s/%s", vg, lv)) {
> +		syslog(LOG_ERR, "Unable to form LVM command: Device name too long.");
> +		return 0;
> +	}
> +




> --- 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.


> +			lp->sign = arg_sign_value(cmd, size_ARG, SIGN_NONE);
> +			lp->percent = PERCENT_NONE;
> +		}
>  	}
>  
>  	if (lp->resize == LV_EXTEND && lp->sign == SIGN_MINUS) {
> @@ -269,6 +277,33 @@ static int _lvresize_params(struct cmd_c
>  	return 1;
>  }
>  
> +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'll do some functionality tests later - patch looks good to me so far.

Zdenek

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

[Petr Rockai]

Zdenek Kabelac <zkabelac@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>

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

[Zdenek Kabelac]

Dne 15.10.2010 13:57, Petr Rockai napsal(a):
> Zdenek Kabelac <zkabelac@redhat.com> writes:
> 

>>> +	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.)
> 

Yes - we do not currently use floats in lvm.conf. Another version could be, to
allow setting also in some absolute sizes - i.e.

snapshot_autoextend_free = 100MB
snapshot_autoextend_size = 200MB


Ok - I've tested the patch - and it seems to be working.

So ACK from my side.

What I see as a problem for the future is how to resolve settings per each
snapshot as current  'global' version - one setting for all snapshots is IMHO
quite limiting.


Zdenek

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

[Alasdair G Kergon]

On Fri, Oct 15, 2010 at 02:55:37PM +0200, Zdenek Kabelac wrote:
> What I see as a problem for the future is how to resolve settings per each
> snapshot as current  'global' version - one setting for all snapshots is IMHO
> quite limiting.
 
When we discussed this I suggested storing the information directly against each LV
just as we do for other properties.  So you'd set it with lvcreate/lvchange or
inherit setting from vgcreate/vgchange or use default from lvm.conf or fallback
to builtin default.

That builds upon this patch, which is the first step.

Alasdair