watchdog: new option to skip ping in close

If a daemon has /dev/watchdog open, and exits (say by crashing or sigkill), watchdog_release() generates a new watchdog_ping(), extending the life of the machine. However, the daemon may require control over whether or not the watchdog is pinged, and the ping generated by close compromises this control.

I'm using the watchdog to reset machines a cluster when their shared leases expire. Machine A will not ping its watchdog after one of its leases expires, intending that it be reset by its watchdog. Machine B can then assume that 60 seconds after the lease expiration, machine A will be reset, and it can safely acquire A's leases. The problem is that if the daemon on machine A exits sometime during the 60 seconds prior to the watchdog firing, watchdog_close() generates a new ping, extending the the life of A by a new 60 seconds. To account for this, B must wait 120 seconds instead of 60 seconds to account for the additional ping the kernel may have inserted.

Signed-off-by David Teigland <[email protected]>

---
  drivers/watchdog/watchdog_dev.c |    9 ++++++++-
  include/linux/watchdog.h        |    2 ++
  2 files changed, 10 insertions(+), 1 deletion(-)

drivers/watchdog/watchdog_dev.c +0 -0

index ef8edec..b77442a 100644
@@ -339,6 +339,11 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
  	unsigned int val;

drivers/watchdog/watchdog_dev.c +3 -1

@@ -480,7 +485,9 @@ static int watchdog_release(struct inode *inode, struct file *file)
  		if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
  			dev_crit(wdd->dev, "watchdog did not stop!\n");
  		mutex_unlock(&wdd->lock);
-		watchdog_ping(wdd);
+
+		if (!test_bit(WDOG_NO_CLOSE_PING, &wdd->status))
+			watchdog_ping(wdd);
  	}
 
  	/* Allow the owner module to be unloaded again */

include/linux/watchdog.h +1 -0

index da70f0f..4fa10ff 100644
@@ -31,6 +31,7 @@ struct watchdog_info {
  #define	WDIOC_SETPRETIMEOUT	_IOWR(WATCHDOG_IOCTL_BASE, 8, int)
  #define	WDIOC_GETPRETIMEOUT	_IOR(WATCHDOG_IOCTL_BASE, 9, int)
  #define	WDIOC_GETTIMELEFT	_IOR(WATCHDOG_IOCTL_BASE, 10, int)
+#define	WDIOC_NOCLOSEPING	_IOR(WATCHDOG_IOCTL_BASE, 11, int)
 
  #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
  #define	WDIOS_UNKNOWN		-1	/* Unknown status error */

Why on earth are you putting that if block there and not simply putting the command in the switch-case where it belongs? If you were afraid watchdog_ioctl_op could cause issues, then maybe you should have studied the code?

That would have thought you that it will always return -ENOIOCTLCMD for commands it does not know how to handle, so it won't interfere with adding new standard commands.

Adding this if block there is completely unnecessary and quite ugly IMHO.

Quoted 7 lines · Hans de Goede

Why on earth are you putting that if block there and not simply putting the command in the switch-case where it belongs? If you were afraid watchdog_ioctl_op could cause issues, then maybe you should have studied the code?

That would have thought you that it will always return -ENOIOCTLCMD for commands it does not know how to handle, so it won't interfere with adding new standard commands.

Quoted 1 line

Adding this if block there is completely unnecessary and quite ugly IMHO.

It is ugly, but it's also necessary for a couple reasons that you may have missed at first glance. First, this ioctl is addressing behavior that exists in the common framework layer, not in individual drivers, so it doesn't make too much sense to pass down to the drivers.

That being said, we could do so if ENOIOCTLCMD worked as you suggest, but it doesn't quite. Drivers with an ioctl op will return ENOTTY,

in which
case the switch statement would work, but drivers with no ioctl op (e.g. softdog) cause watchdog_ioctl_op to return ENOIOCTLCMD, causing the switch statement to be skipped.

In the end, we need WDIOC_NOCLOSEPING to be processed by the core, independent of what drivers do, since as I explained this is targeting the behavior of the core framework, not the drivers.

Quoted 3 lines

In the end, we need WDIOC_NOCLOSEPING to be processed by the core, independent of what drivers do, since as I explained this is targeting the behavior of the core framework, not the drivers.

Right, and it will be processed by the core if it is in the switch-case because:

watchdog_ioctl_op will return ENOIOCTLCMD for non driver specific custom ioctls, causing watchdog_ioctl() to continue into the switch case for standard ioctls!

Quoted 8 lines

Why on earth are you putting that if block there and not simply putting the command in the switch-case where it belongs? If you were afraid watchdog_ioctl_op could cause issues, then maybe you should have studied the code?

That would have thought you that it will always return -ENOIOCTLCMD for commands it does not know how to handle, so it won't interfere with adding new standard commands.

Adding this if block there is completely unnecessary and quite ugly IMHO.

It is ugly, but it's also necessary for a couple reasons that you may have missed at first glance. First, this ioctl is addressing behavior that exists in the common framework layer, not in individual drivers, so it doesn't make too much sense to pass down to the drivers.

That being said, we could do so if ENOIOCTLCMD worked as you suggest, but it doesn't quite. Drivers with an ioctl op will return ENOTTY, in which case the switch statement would work, but drivers with no ioctl op (e.g. softdog) cause watchdog_ioctl_op to return ENOIOCTLCMD, causing the switch statement to be skipped.

In the end, we need WDIOC_NOCLOSEPING to be processed by the core, independent of what drivers do, since as I explained this is targeting the behavior of the core framework, not the drivers.

For those reasons, I placed the code where it is, but I would be satisfied with a different approach that also works.

Quoted 5 lines

That being said, we could do so if ENOIOCTLCMD worked as you suggest, but it doesn't quite. Drivers with an ioctl op will return ENOTTY, in which case the switch statement would work, but drivers with no ioctl op (e.g. softdog) cause watchdog_ioctl_op to return ENOIOCTLCMD, causing the switch statement to be skipped.

at this moment all drivers that are allready converted to the general watchdog infrastructure/API don't need and thus don't have an ioctl operation in place. They all use the common code. Before we added the WDIOC_GETTIMELEFT to the framework, I had the converted iTCO_wdt driver using an ioctl operations call that used the ENOIOCTLCMD without any issue.

I'm going to be honoust: I don't understand the use case. I have the impression that you want to solve High Availability issues with watchdog resets...

And that's like replacing a car with a new car when you are out of gasoline...

So, I need to think more about this and I wonder if other people have the same issue as you have with the extra ping.

The code for the complete system would be translated as a module or kernel parameter and not as an extra ioctl.
If it would be needed for specific drivers then we need a more generic approach that will also let us tweak other items in the future so that we don't need an ioctl for every change of behaviour.