[PATCH] Avoid recursive calls from dmeventd to itself

[PATCH] Avoid recursive calls from dmeventd to itself

[Petr Rockai]

Hi,

the attached patch keeps the LVM-based dmeventd plugins from trying to
manipulate the dmeventd monitoring state of the logical volumes they are
currently acting on.

Until now, every time a logical volume has been changed by a dmeventd
plugin, this plugin would have called back to dmeventd through the
external FIFO mechanism. I am fairly sure this is superfluous,
inefficient and possibly even dangerous.

It is probably related to RHBZ 707056 and I wouldn't be surprised if
this sometimes caused mirror repair issues. With the patch, the
incidence of "No longer monitoring..." messages while repairing mirrors
drops dramatically.

Yours,
   Petr

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmeventd_norecurse.diff
Type: text/x-diff
Size: 1806 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20110901/b65fdb7c/attachment.bin>
-------------- next part --------------


-- 
id' Ash = Ash; id' Dust = Dust; id' _ = undefined

[PATCH] Avoid recursive calls from dmeventd to itself

[Alasdair G Kergon]

On Thu, Sep 01, 2011 at 03:08:03AM +0200, Peter Rockai wrote:
> -		lvm2_run(_lvm_handle, "_memlock_dec");
> +		lvm2_run(_lvm_handle, "_dmeventd_leave");

I'm not keen on overloading it like that.

If it's being used with cmdlib and lvm2_run, use of existing cmdline parameters
ought to be sufficient to achieve this.

It might be a little more code, but I'd rather see it a property of the handle
never to perform dmeventd monitoring calls.  We never fixed handle init to 
take multiple settings, so maybe call
  void lvm2_disable_dmeventd_monitoring(void *handle)
after handle initialisation to set DMEVENTD_MONITOR_DISABLED and have that take
precedence over any later attempt to enable monitoring.

Alasdair

[PATCH] Avoid recursive calls from dmeventd to itself

[Petr Rockai]

Alasdair G Kergon <agk@redhat.com> writes:

> On Thu, Sep 01, 2011 at 03:08:03AM +0200, Peter Rockai wrote:
>> -		lvm2_run(_lvm_handle, "_memlock_dec");
>> +		lvm2_run(_lvm_handle, "_dmeventd_leave");
>
> I'm not keen on overloading it like that.
>
> If it's being used with cmdlib and lvm2_run, use of existing cmdline parameters
> ought to be sufficient to achieve this.

While that is true, I doubt it is a good idea. If we say that dmeventd
is not supposed to call itself recursively (which I believe is the
correct approach), we should enforce that globally. Asking every call
site to take care of this individually is error prone and bound to
introduce the bug sooner or later (especially with the rampant
copy-and-paste programming found in LVM :\).

> It might be a little more code, but I'd rather see it a property of the handle
> never to perform dmeventd monitoring calls.  We never fixed handle init to 
> take multiple settings, so maybe call
>   void lvm2_disable_dmeventd_monitoring(void *handle)
> after handle initialisation to set DMEVENTD_MONITOR_DISABLED and have that take
> precedence over any later attempt to enable monitoring.

This sounds like a better option. Now that raises the question why we
aren't doing the same thing for memlock_{inc,dec}? We should certainly
unify these two things. I'll have a look, and unless I run into a
stumbling block will submit a patch to do that.

(Of course, it would be much better if we could instead move dmeventd
plugins over to using liblvm2api (replacing liblvm2cmd), but I suspect
there's a lot of stuff to resolve before we can hit that mark.)

Petr

-- 
id' Ash = Ash; id' Dust = Dust; id' _ = undefined

[PATCH] Avoid recursive calls from dmeventd to itself

[Petr Rockai]

Petr Rockai <prockai@redhat.com> writes:

>> It might be a little more code, but I'd rather see it a property of the handle
>> never to perform dmeventd monitoring calls.  We never fixed handle init to 
>> take multiple settings, so maybe call
>>   void lvm2_disable_dmeventd_monitoring(void *handle)
>> after handle initialisation to set DMEVENTD_MONITOR_DISABLED and have that take
>> precedence over any later attempt to enable monitoring.
>
> This sounds like a better option. Now that raises the question why we
> aren't doing the same thing for memlock_{inc,dec}? We should certainly
> unify these two things. I'll have a look, and unless I run into a
> stumbling block will submit a patch to do that.

The patch doing that is attached. Seems straightforward enough to me. We
can change the memlock interface to be the same, too.

Yours,
   Petr

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dmeventd_norecurse_2.diff
Type: text/x-diff
Size: 1939 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20111010/269e3b36/attachment.bin>
-------------- next part --------------

-- 
id' Ash = Ash; id' Dust = Dust; id' _ = undefined

[PATCH] Avoid recursive calls from dmeventd to itself

[Milan Broz]

On 10/10/2011 04:33 PM, Petr Rockai wrote:
> Petr Rockai <prockai@redhat.com> writes:
> 
>>> It might be a little more code, but I'd rather see it a property of the handle
>>> never to perform dmeventd monitoring calls.  We never fixed handle init to 
>>> take multiple settings, so maybe call
>>>   void lvm2_disable_dmeventd_monitoring(void *handle)
>>> after handle initialisation to set DMEVENTD_MONITOR_DISABLED and have that take
>>> precedence over any later attempt to enable monitoring.
>>
>> This sounds like a better option. Now that raises the question why we
>> aren't doing the same thing for memlock_{inc,dec}? We should certainly
>> unify these two things. I'll have a look, and unless I run into a
>> stumbling block will submit a patch to do that.
> 
> The patch doing that is attached. Seems straightforward enough to me. We
> can change the memlock interface to be the same, too.

ack.

Milan