mdev -d can (silently) die with "read: no buffer space available"

[Jan Klötzke]

On Sun, Dec 15, 2019 at 08:44:14AM +1000, Alexander Zangerl wrote:
> On Sat, 14 Dec 2019 14:49:20 +0100, Jan Klötzke writes:
> >Just to double check: did you increase BUFFER_SIZE or RCVBUF? I'm
> >asking because the error that you observed (ENOBUFS) is returned by the
> >kernel if the socket ran out of buffer space, that is mdev did not read
> >the socket fast enough. OTOH if BUFFER_SIZE would be too small then the
> >event would just be silently truncated.
> 
> i only increased BUFFER_SIZE.
> 
> --- a/util-linux/mdev.c
> +++ b/util-linux/mdev.c
> @@ -1150,7 +1150,7 @@ static void initial_scan(char *temp)
>  #if ENABLE_FEATURE_MDEV_DAEMON
>  
>  /* uevent applet uses 16k buffer, and mmaps it before every read */
> -# define BUFFER_SIZE (2 * 1024)
> +# define BUFFER_SIZE (64 * 1024)
>  # define RCVBUF (2 * 1024 * 1024)
>  # define MAX_ENV 32
> 
> 2 mb for the socket buffer seems ample, but
> reading possibly that much in bites of only 2 kb feels a bit disproportional.
> maybe reading only one such nibble per cycle is what causes the
> buffer to grow faster than mdev can consume it? i'm not sure.

This is strange. The read() call will always return a single event.
Otherwise the logic would haven been broken already. So except for
possibly truncated events the BUFFER_SIZE does not make a difference on
how fast mdev can process events.

OTOH udevd seems to use a whooping 128MiB for the netlink socket receive
buffer. And the ENOBUFS error is exactly what should be returned if the
receive buffer overflows.

> >This has always been this way. I think it makes sense for a plain "mdev
> >-s" which is a synchronous invocation. But for "mdev -d" it might indeed
> >be better to write everything to mdev.log, including the initial scan.
> 
> i concur.
> 
> >> i think open_mdev_log() should be called in daemon_loop, not just
> >> after initial_scan() and bb_daemonize_or_rexec().
> >
> >That would incur a considerable syscall overhead for the regular case
> >where mdev.log does not exist. Then mdev would try to open mdev.log for
> >every event it receives.
> 
> i meant 'guarded by a stat() and inode comparison'.
> that shouldn't be too expensive i think. how about this? the patch works
> for any number of instances of mdev.log being created and removed later on,
> and costs one stat().
> 

> --- a/util-linux/mdev.c
> +++ b/util-linux/mdev.c
> @@ -1156,6 +1156,9 @@ static void initial_scan(char *temp)
>  
>  static void daemon_loop(char *temp, int fd)
>  {
> +	 struct stat logfilestat;
> +	 ino_t logfileino;
> +
>  	for (;;) {
>  		char netbuf[BUFFER_SIZE];
>  		char *env[MAX_ENV];
> @@ -1180,6 +1183,14 @@ static void daemon_loop(char *temp, int
>  			s += strlen(s) + 1;
>  		}
>  
> +		/* check if the logfile has (dis)appeared */
> +		if (stat("/dev/mdev.log", &logfilestat) == 0
> +				&& S_ISREG(logfilestat.st_mode)
> +				&& logfilestat.st_ino != logfileino)
> +		{
> +			 open_mdev_log(NULL ,getpid());
> +			 logfileino = logfilestat.st_ino;
> +		}
>  		process_action(temp, 0);
>  
>  		while (idx)

Hmm, adding a stat() for each event that is processed does not seem
right. In the normal case there will never be a mdev.log...

> 
> >Maybe adding SIGHUP handler that
> >re-reads mdev.conf and SIGUSR1 to (re-)open mdev.log is a good idea...
> 
> that sounds great but maybe a bit more complex than necessary?
> 
> having to restart mdev to activate a new mdev.conf feels reasonable to me,
> as the daemon doesn't have any internal state to keep long-term.

As a user that's what I would expect from a daemon: reload the
configuration on SIGHUP and do a log-rotation on SIGUSR1. I think the
patch won't be big to implement that.

> also, (re)opening the log only via signal would mean that there's a
> near-zero chance of getting debug output for the initial scan.

No. In case you want to debug the initial scan the mdev.log should
simply be touched in advance. SIGUSR1 should be there to let mdev know
that you added/removed/replaced mdev.log and that mdev should re-open
it.

/Jan