Re: [gamin] gam-poll.c rewrite

[Daniel Veillard <veillard redhat com>]

On Fri, Jul 01, 2005 at 02:58:34PM +0100, Neal H. Walfield wrote:
> At Fri, 1 Jul 2005 08:45:04 -0400,
> Daniel Veillard wrote:
> >   That is a huge change that I didn't see discussed at all !
> > It maybe the right thing but IMHO changes a lot ! For example
> > suddenly monitoring a resource may lead to monitoring a very 
> > busy directory, or put a monitor onto a kind of filesystem you
> > really don't want to match. Trivial example
> > 
> >    monitor /tmp/app-$user/lock
> > 
> > which is likely to be done by a number of applications.
> > suddently you force to monitor /tmp, which is one of the busiest
> > directory on any system. Do that on a machine with 100 users
> > and I'm sure to get a flood of bug reports from pissed-off customers
> > because there is hundred of gam_servers eating all the CPU on the
> > system.
> 
> Busy resources automatically go into polling mode.  Is this somehow
> insufficient?

  Might be.
  You still increase the number of watched resources and potentially
a lot. This really can have a large impact and it's very hard to assess
how many things will break. Things do break on changes, no matter if
this is right or wrong to do the change, and gamin potentially affects
a lot of desktop applications and expected user behaviour. Adding monitors
to the full hierarchy is taking a lot of risks IMHO.

> > > even though you only need to watch the parent if the
> > > directory in question is empty.  However, that is non-trivial and may
> > > involve some races (if the last file is unlinked and then the
> > > directory as well we might miss the chance to set up the monitor for
> > > the parent) so I deferred.
> > 
> >   If a file or directory is monitored, it gets removed, then I would
> > expect a kernel event, then the stat() associated will show the resource
> > had disapeared.
> 
> dnotify only emits events on directories.  The current approach is to
> use dnotify to monitor the parent of a node and scan its contents for
> changes.  I think, although I could be wrong, when a directory is
> unlinked, its parent gets an event, not it.  Thus the need to monitor a
> directory's parent for its removal.

  So it's really needed only if we have a watch on resource which is
a directory. It would not be needed in the general case. 

> > > Where do you want to go from here?
> > 
> >   I want to see discussions of features *before* you code them !
> 
> To explain my past behavior, I've found that the FLOSS mentality is
> generally "show me the code."  Knowing that this is your attitude,
> I'll try to have more discussion before I do work.

  Okay, thanks, code is important but as the project grows and affects
users you have to operate differently. If the code wasn't widely deployed
and relied upon, I would not care that much :-) . 
  My view about this is "Open Source is a process", that process means to
operate as a team, in an open fashion, and pushing big changes like the
recursive monitoring should never be done silently. If you do so, you break
the knowledge of every other people in that team, reducing the value of the
code in the end, even if intrinsically the code may be better after that.

> > You are doing radical design changes, and there is not a single exchange
> > of mails about them before hand. You can't work in isolation, I admit
> > that the mail-list being down for 2 days (but that was documented/planned)
> > doesn't help, but it's a process problem.
> > 
> >   How to go from there:
> >     - I don't think I will accept a patch monitoring all parent directories.
> >       That mean we can't even run reliable regression tests anymore, and
> >       while I like the work you are doing, being able to make regression
> >       tests is even more crucial.
> 
> I am confused by your logic: how does not running regressions follow
> from monitoring parent directories?  We can still run the regression
> tests.

   A number of those regressions monitor the way dnotify is used. It's key
to find out breakages when code changes are introduced (as a proof I would not
have detected the recursive watch behaviour otherwise it would have gone
silently in). I want to monitor what's happening there. In the current version
with a complete recursive monitoring of the ancestors there is little chance
me running the test and you running the test are gonna match depending where
our checkouts are installed.

> >     - If doing just the parent directory monitoring, then it would be
> >       possible to fix the dnotify*.py and flood*.py regression tests
> 
> The debugging API exposes implementation details.  If the

  Those implementation details are crucially important. If I get a Changed
even because I watched using dnotify versus because watching with poll it
is very different, this may meen the busy detection code in the server just
failed. I also want to monitor that there is no leak of kernel monitors
from the regression tests because that means leaks of filedescriptors and
potentially filling hard drives with unreachables but still open files.

> implementation changes then of course the debugging events will as
> well.  I have listed all the other regressions that I saw in the Gamin
> public interface.

  Yes, thansk for that, but I want to monitor those internals as part of
the regression tests too. Watching the parent directory when a directory
is being watched sounds an acceptable change, this will lead to some changes
to dnotify*.py and flood*.py, but the function of those tests will still
be available which is what I care about.

> >       That's a patch I may accept after checking behaviour on the overall
> >       desktop on a couple of OS versions, but that would be a gamin 0.2xx
> >       new release, I will not consider this an incremental release of 0.1
> 
> Just to make it clear, I have not been pushing for an immediate
> release with these changes.

  Yes but keeping them out of CVS means you accumulates changes on your side
which is not good and applying them to CVS means we are ready to integrate
them and in that case this dscussion is needed.

   thanks,

Daniel

-- 
Daniel Veillard      | Red Hat Desktop team http://redhat.com/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/