Re: [Xen-devel] [PATCH] Switch to poll() in cxenstored's IO loop

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-01-22 at 14:50 +0000, Wei Liu wrote:
> > > > > +                 !(reopen_log_pipe0_pollfd->revents & 
> > > > > ~(POLLIN|POLLOUT|POLLPRI)) &&
> > > > 
> > > > This represents an error case I think? Is there anything we can do about
> > > > it? If this sort of error occurs and we do nothing will we end up just
> > > > spinning because every subsequent poll will just return straight away?
> > > > 
> > > > 
> > > 
> > > Yes, this represents an error. This indicates the pipe used to trigger
> > > reopen_log is broken. What's the motivation of reopening log file?
> > 
> > Rotation I should imagine.
> > 
> > >  I
> > > have no idea about the use case. But simply ignoring this error instead
> > > of crashing the process is better choice IMHO.
> > 
> > Agreed, but my concern was that the daemon would spin consuming 100%
> > CPU, which is nearly as bad as crashing.
> > 
> > > The whole pollfd set is reinitialized for every loop. So it is also
> > > possible that for the next loop it will success even previous poll
> > > fails?
> > 
> > I suppose it depends on the reason it failed. I would expect most of
> > them would be pretty final rather than temporary but I haven't looked at
> > the pipe docs.
> > 
> 
> We can reinitialize the pipe if error occurs, because this pipe is only
> used within xenstored itself to notify log reload.

But if you get POLLPRI or POLLOUT you don't do this with the code as it
currently stands (from the v2 patch):
+                       if (reopen_log_pipe0_pollfd->revents & 
~(POLLIN|POLLOUT|POLLPRI)) {
+                               close(reopen_log_pipe[0]);
+                               close(reopen_log_pipe[1]);
+                               init_pipe(reopen_log_pipe);
+                       } else if (reopen_log_pipe0_pollfd->revents & POLLIN) {
+                               char c;
+                               if (read(reopen_log_pipe[0], &c, 1) != 1)
+                                       barf_perror("read failed");
+                               reopen_log();
+                       }

So on POLLRDHUP or POLLERR etc you reopen the log, on POLLIN you act as
expected but for POLLPRI and POLLOUT you don't do anything.

If these two events should never occur then you should include the in
the error handling part of the above. If they can occur then you need to
do something to handle them, otherwise they will just keep reoccuring on
every iteration.

The same goes for the other instances of this pattern.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel