Avoid recursive calls from dmeventd to itself
PATCH 1/1 Avoid recursive calls from dmeventd to itself
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 --------------
Signature · 1 line
id' Ash = Ash; id' Dust = Dust; id' _ = undefined
PATCH 1/1 Avoid recursive calls from dmeventd to itself
Review
Quoted 2 lines · Peter Rockai
(quoted diff) +1
-1- 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 1/1 Avoid recursive calls from dmeventd to itself
Quoted 8 lines · Alasdair G Kergon
Review
Quoted 2 lines · Peter Rockai
(quoted diff) +1
-1- 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 :\).
Quoted 6 lines
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
voidlvm2_disable_dmeventd_monitoring(void *handle)
after handle initialisation to setDMEVENTD_MONITOR_DISABLEDand 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
Signature · 1 line
id' Ash = Ash; id' Dust = Dust; id' _ = undefined
PATCH 1/1 Avoid recursive calls from dmeventd to itself
Quoted 11 lines · Petr Rockai
Quoted 6 lines
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
voidlvm2_disable_dmeventd_monitoring(void *handle)
after handle initialisation to setDMEVENTD_MONITOR_DISABLEDand 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 --------------
Signature · 1 line
id' Ash = Ash; id' Dust = Dust; id' _ = undefined
PATCH 1/1 Avoid recursive calls from dmeventd to itself
Quoted 9 lines · Petr Rockai
Quoted 7 lines · Petr Rockai
Quoted 6 lines
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
voidlvm2_disable_dmeventd_monitoring(void *handle)
after handle initialisation to setDMEVENTD_MONITOR_DISABLEDand have that take precedence over any later attempt to enable monitoring.
Quoted 7 lines
Quoted 4 lines
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