Created attachment 51198 [details] [review]
File module patch

I'm going to attach a few files below, they go in gnome-vfs/modules.

Created attachment 51199 [details]
inotify helper source

Created attachment 51200 [details]
inotify helper header

Created attachment 51201 [details]
local_inotify_syscalls.h

I'd appreciate a review of my patch.

YES!

A few notes:

Gnome-vfs will use inotify if it is available (at compile time), and use FAM
otherwise. Do people have a problem with it not being detected at runtime? By
making it a compile time option, we avoid the overhead of linking against libfam.

With gnome-vfs using inotify directly there is a big memory usage drop. Compared
to FAM/gamin+dnotify you should save approx 4MB of memory and compared to
gamin+inotify you should save about 2MB of memory. I'm talking about real memory
used by the daemon, not including shared libraries. Plus you will probably save
a bit more by not having to load libfam at all.

Thanks for your efforts!

While I'm unable to review your patch for the lack of knowledge, just a nitpick:
I wonder why you check for HAVE_LINUX_INOTIFY_H in the code instead of HAVE_INOTIFY

Christian, HAVE_INOTIFY is an automake conditional and HAVE_LINUX_INOTIFY_H is a
#define produced from AC_CHECK_HEADERS.

Doesn't gamin include code that delays notifications when there's thousands per
second? I remember Daniel Veillard talking about this. He may have something to
say on the topic. 

That said I'm not super familiar with any of the *notify code, so feel free to
set me straight...

gamin+dnotify has flow control. That is, when the rate of events for a directory
is high it disables dnotify and falls back to straight polling. The downside to
this is the high memory usage from keeping full stat() trees of whatever
directories you are watching. 

gamin+inotify tries to reduce duplicate events. It does this in two places, the
first is in the kernel and the second is in the per FAM-connection event queue.
This works just as well (better depending on the criteria) based on my
performance testing. To clarify, looking for duplicate events means: looking at
the tail of the event queue and only queueing the new event if it is different
than the tail.

What I do to test performance is run a script that creates a file, moves it
twice, and deletes the file. It does this for 1000 files. At the same time I run
 dd if=/dev/zero of=testfile. So there are two fast event streams. 

gamin+dnotify uses a lot of CPU until it switches on flow control, then it uses
a minor amount. Memory usage grows from all directory tree caching that takes place.

gamin+inotify never uses much CPU. Some times the CPU spikes to 15% briefly.
Memory usage is static.

My test case is very extreme, dd fills up the file much faster than anyone could
download it. And because the move script is running at the same time, the
kernel/gamin+inotify won't encounter many duplicate events.

I consider memory usage to be more important than CPU usage. Also, I think there
will be an overlap between people with fast CPUs and lots of disk access. So the
 minor CPU hit is worth it when considering how much memory we can save.

Now, back to gnome-vfs. Gnome-vfs provides a per-monitor duplicate event check.
 It is roughly the same as the per-fam-connection duplicate event checker used
in gamin+inotify. It is probably more effective because it is more fine grained
(being per monitor, not per connection). 

In conclusion, gnome-vfs+inotify will use much less memory, and even under
extreme cases use a relatively small amount of CPU.

New version. Fixes a nasty race condition in the previous version.

Created attachment 51287 [details] [review]
File module patch take 2

Created attachment 51288 [details]
inotify-helper.c

Created attachment 51289 [details]
inotify-helper.h

Created attachment 51290 [details]
local_inotify_syscalls.h

Interesting, thanks for the run down. 

FWIW, nautilus does seem to currently have problems with too many notification
events. Try building a source tree while the directory is open in nautilus. But
that's another bug for another day. 

Not having looked to deeply at the code i think this makes perfect sense.
However, I think by default the inotify/fam switch should be runtime, perhaps
with a configure switch to disable fam support. We're still not in a place where
we can guarantee that most kernels have inotify support.

Also, i think this is to late for gnome 2.12 at this point.

Alexander, I'm in complete agreement. I'm attaching an updated patch that
accomplishes runtime switching (preference going to inotify) and includes a
--disable-fam configure switch.

Created attachment 51384 [details] [review]
File module patch take 3

Created attachment 51773 [details] [review]
rawr

I discussed with John earlier that I was also working on a patch to address
this bug.

Here's what I have so far.


This is in absolutely no way proposed for inclusion as it's still very much a
work in progress but I thought I should throw it out there.


I wrote the patch in a fairly generic way such that the inotify-*.[ch] files
don't know about gnome-vfs's existence.

A question that I'd like feedback on:  maybe it would be more appropriate to
have a libinotify (or similar) that presents a nice interface to userspace
applications?  gnome-vfs (and others, like beagle) could then use this unified
library which would do all the dirty work.

This carries all of the normal advantages of many users of a single piece of
code making the finding and repair of bugs a lot easier (compared with the
current situation where if gamin or gnome-vfs or beagle or [...] find and
repair a bug in their inotify implementations then nobody else benefits).

Ryan, why are you re-implementing the inotify backend? So far your backend is
missing important features. You aren't attempting to re-activate inotify after
deletion. You aren't pairing move events. My code handles these cases and more.
It's also heavily tested. Could you please provide concrete problems with my
approach? You mentioned two in our irc conversation: 1) You thought I was
converting events from inotify-> gamin -> gnome-vfs. My code wasn't doing that.
2) You mentioned the leak in gam_server. The leak may or may not be in the
inotify code, and I encourage you to point out where in my patch to gnome-vfs we
could leak anything. I have run lots of applications when using my gnome-vfs
patch, and I can't find any leaks in that are related to the inotify backend code. 

So, the gnome-vfs inotify backend may have a leak, but it is complete. Fixing
the leak will be trivial once it's found. Fixing a complete re-write of the
inotify backend will require much more effort. As I have said in the gam_server
bug, I have gone through this inotify code, and accounted for every allocate /
deallocate.

I considered going the libinotify course, and was encouraged not to bother by
the beagle maintainers. I'm not saying don't do it, but I would suggest talking
with these parties before going ahead.

I'm not attempting to reactivate after deletion because I haven't looked into
everything that the GNOME VFS monitoring system is supposed to do (Christian
mentioned this, though as well as monitoring files that don't yet exist but
might be created -- something else my patch fails to do).

As for the event pairing, as far as I can see, it's not required.  Gnome VFS has
no concept of reporting 'moves' as far as I can tell, so I just report
MOVED_FROM as a delete and MOVED_TO as a create.

I should probably also have a talk with the beagle guys and see what their
thoughts on the matter are.  I did go ahead and put libinotify in CVS but nobody
has to use it if they don't want to (I haven't even put a particularly high
amount of effort into the code that is in CVS at this time -- it was more a dump
of what I had produced from my own experimenting with inotify).  Your opinions
on what is currently 'libinotify' are being actively solicited though -- please
come on IRC :)

I would like to point out that the only portion of inotify-helper.c that isn't a
requirement is the link (ln) handling. It is racey, and was only implemented
because of FAMs link policy. I would recommend it being removed, and gnome-vfs
adopting a different link policy. FWIW, FAM never properly implemented it's link
policy, so we won't be missing anything. I would just like to re-iterate that I
don't see any reason to re-implement this. There are a couple warts on my code,
but it's much easier to clean them off then re-implement everything from scratch.

Ahh.. yes watching files that don't exist yet is another item that is
implemented in my code already, and tested. It is a requirement for gnome-vfs
monitoring. You just recieve a CREATE event when the file shows back up again.

Yes, Gnome VFS _currently_ has no concept of reporting moves. This among other
event types is a sign of gnome-vfs-monitor's immaturity. The point is, my
inotify code does pair them, so when the much needed overhaul of
gnome-vfs-monitor takes place, we can start providing these events right away.

I have reviewed your libinotify code, as you said, it's very basic and doesn't
provide many features (yet). Having libinotify depend on glib will make it much
less interesting to applications outside of gnome. Gnome applications are alread
y going to be using gnome-vfs-monitor, so why would they be motivated to use
libinotify? Also, gnome-vfs puts constraints on behaviour that applications like
beagle don't want (monitoring of non-existant files, monitoring for files to
re-appear). 

The beagle guy to talk to is Robert Love, my co-conspirator. We discussed this
(libinotify) a couple weeks ago, and he really didn't see a use for it. I don't
want to tell you not to do it, because no doubt, some people will find use in
your code. But I have code that is tested and works for gamin/mono/gnome-vfs and
Robert has code for beagle. I really don't see what market your library is going
to fill. 

You are obviously keen on having this fixed, and understand inotify. Why not put
your energy into "fixing" my patch instead of re-implementing everything?

Sorry, but I don't have time for irc right now. I just got back from the
cottage, and I have to move into school tomorrow. Coincidently, we seem to atten
d the same school.

The leak in gam_server was found, and it was not in the inotify code. So, my
patch  can now be considered solid.

It would be nice if the inotify backend could notify of move events (where it
can, for moves within a partition with rename), and the gnome-vfs calls itself
"faking a move" when gnome_vfs_move is used.

My patch can notify of move events. Inotify sends out a MOVED_FROM & MOVED_TO,
if you are watching both the source & destination directories of the moved file,
you will get both events. The attached patch does the work of pairing the
MOVED_FROM & MOVED_TO events together when possible. But there is no gnome-vfs
move event defined. So, it just sends out a DELETE & CREATE event. 

Nod. Rhythmbox could do with extended monitoring events, the moved_to and
moved_from in particular, to keep track of the files on disk being moved to
different locations, while keeping the associated metadata.

So after serveral flames and requests I am going to comment on this bug here. I
will no using John McCutchan's inotify patch for gnome-vfs in favour for Ryan
Lortie's libinotify approach. The major two reasons are:

1) I don't like the idea of having gam_* code ripped out of gam and then bend it
so that it fits into gnome-vfs. I favor a clean new approach here that can
address all the problems the current FAM based code has.

  Some examples are,
    - we should catch renames of the target we are watching
       (try to do a gnomevfs-monitor /home/nobody/bar and rename bar to foo
        and see what happens)
    - we might have to actually watch all the parents of a given uri (see
       nautilus' new folder-tree-tab-location-thingy)  
    - we don't wanna poll for non-existant files
    - ..., etc, ...

2) I like the idea of having the libinotify code outside of gnome-vfs because I
don't have to maintain it then. While people might flame me for being too lazy
here it is more then that. Maintaining GnomeVFS is not easy and adding more and
more code makes it even more complicated. Especially if it is that complex
itself. (I still have the files-disappear issue on my ubuntu box sometimes).

While I am sure that one could fix all the technial problems in John's patch
(and also the comsetic ones like rename all the gam_* functions) that still
doesn't solve problem two. 

I am totally aware of the fact that throwing away such a big peace of code
(although initially not written for gnome-vfs) is totally sad and I really wanna
thank John for all the effort he has been putting into it - it is nothing
personal here. 

I myself have written a > 700 lines patch 3 month ago to try to get inotify
directly into gnome-vfs which I am going to trash now, so I know that it hurts.
I am really sorry for the inconvenience and I hope you might join efforts with
Ryan and libinotify.

On a more technical side I am preparing a patch for using libinotify but its not
complete. I just wanna point to it now so people see that it's already worked
on. http://www.gnome.org/~gicmo/patches/inotify.patch is the url. It's not
working atm. Don't use it. 
The other thing I looked into is extening the GnomeVFSMonitorType enumeration to
have a GNOME_VFS_MONITOR_EXTENDET memeber. With this flag passed to
gnome_vfs_monitor_add () one could get MOVE events, etc. Although client code
must be prepared to get a GNOME_VFS_ERROR_NOT_SUPPORTED when the
GNOME_VFS_MONITOR_EXTENDET is passed and inotify is not used since we can only
provide that info if we are using inotify. 
Comments?

Christian, could you please enlighten me to the problems in my patch? AFAIK
there is none, aside from the superficial problems like the function names. Yes,
this code was originally written for gamin, but almost all of it is generic,
which is why it was so trivial to port it to gnome-vfs. You list three concerns:

  - we should catch renames of the target we are watching
       (try to do a gnomevfs-monitor /home/nobody/bar and rename bar to foo
        and see what happens)

My patch already handles this, but there is no way for gnome-vfs to report move
or rename events currently, so I just report a delete followed by a create
event. (Libinotify doesn't support sending move/rename events)

     - we might have to actually watch all the parents of a given uri (see
       nautilus' new folder-tree-tab-location-thingy)  

I don't even understand why this is a problem. And "all the parents"? A uri only
has one parent. What is stopping you from watching an arbitrary directory with
my patch?

    - we don't wanna poll for non-existant files

Yes you do, there is a finite limit of inotify watches, (currently 8192 per
user) and you shouldn't be wasting them just for the sake of not polling. It is
very easy for a user to hit this limit when they are running beagle, and then
you are totally screwed. 

Finally,

   - ..., etc, ...

What are the other concerns? Why haven't you spoken with me about the other
concerns you seem to have. Why haven't you listed them on this bug? You seem to
have made up your mind in private, without discussing them with me. I just took
a look at the libinotify code, and it doesn't offer much compared to my patch,
other than this percieved benefit of it being a seperate library. In fact it
lacks many features that my code offers and it is totally untested. I guess
working code isn't as useful as yet to be written code that promises to be
"better" and "cleaner".

You mention this "files disappear bug", but there isn't a bug in gnome or
ubuntu's bugzilla. No one else seems to be having this problem. 

In general, I don't see any benefit to re-implementing code that works, and has
been well tested. And you have no valid technical arguments to backup your
position. The implementation you are advocating we use is immature, and untested. 

I just found your "files disappear bug"
http://bugzilla.ubuntu.com/show_bug.cgi?id=14967, if this is a valid bug report,
it is a kernel bug not a user space bug. So, yet another perceived problem with
my patch has nothing to do with it. I am going to investigate the possible
kernel bug.

Hey John,

as I said, I am pretty sure you are a damned skilled programmer and I never
doubted the quality of your code. I am also very intersted in the inotify
details like e.g. the maximum amounts of watches and all that's stuff BUT that
exaclty leads me to point two of my concern. I don't even wanna spent time
looking if my file disappear bug is inotify client code or if its a kernel bug
(I wouldnt be suprised anyway). I just wanna link against a library and if
somebody has issues with files disappearing I will tell to go to you or Ryan.
It's mainly me at the moment trying to maintain gnome-vfs (reviewing patches and
arguing with people ;) and I already have to understand so many different
concepts, protocols and my ToDo is huge. Could you for on moment consider my POV
and just accept that I don't want this code in the gnome-vfs codebase but live
outside, just like fam!
Why can't you and Ryan not join efforts and just make libinotify be
uber-rocking? I am pretty sure Ryan is open for a discussion about all the
various details of inotify and accepts patches and stuff. 
I am flying out of California back home tomorrow so dont expect any comments
here soon, it's not me being arrogant or something. 

And I totally appreciate all the efforts you put in here (including the long
bugzilla statements). Seriously. Thanks!

(Btw, you patch really supports sending out DELETE, CREATE for rename of watch
targets? How come I never see them with gnome-vfs+gamin+inotfy?)

I don't see how having the code in a c file included in gnome-vfs, maintained by
me, is any more of a burdan than the path you insist on going down. Since you
already made up your mind without even discussing these issues with me, I'm not
going to bother pushing my patch anymore, you obviously have a bias against it. 

Reinventing the wheel is fun!

Its clear that this code wasn't designed for gnome-vfs (e.g. the gam_* names,
and the unnecessary inotify_data_t/inotify_sub split), however we need to get
some inotify support in, and depending on an external not-api/abi stable library
won't work. So, for now i think using johns code is the right thing. We might
possibky want to use libinotify later if it turns out to be stable and useful,
thats a later question.

I made some changes to johns last patch, mostly making it threadsafe but also
changing some small details.

John: There is an issue with the way you handle symlinks though. I don't really
understand the need to all the link special handling, but if you monitor as a
directory a symlink that points to a directory you won't ever get any changes
from in that directory reported. I think this is a regression from fam/gamin.

Also, john, would you be interested in maintaining this code in gnome-vfs? I
guess you already maintain similar code in gamin, so it might not be much more
work for you.

I reported the symlink-to-dir issue as bug 322348.

I have factored out the kernel glue code into inotify_kernel.[ch].

inotify_kernel handles:
*) Reading of events from inotify instance fd
*) Pairing moves together

It only deals with wds and not paths. 

I have extended the event structure to look like this:

typedef struct {
    guint32 wd;
    guint32 mask;
    guint32 cookie;
    guint32 len;
    char *  name;
    gboolean paired;
    guint32 pair_wd;
    guint32 pair_mask;
    guint32 pair_len;
    char *  pair_name;
} ik_event_t;

Paired will be true if we were able to pair a MOVED_FROM and MOVED_TO. 
You only get 1 event for a move. 
If we couldn't pair the FROM and TO, the event masks are changed to DELETE and
CREATE.

It's currently callback based. I'm interested in comments about the interface.
This is mainly the same code, so it should be stable. 

A simple app would look like this:

void ik_callback(ik_event_t *event);

ik_startup (ik_callback);

ik_watch (...)

Then when events are ready ik_callback will be called.

Next I'm planning on porting the backend to use this new interface. After that I
will look at the symlink bug.

PS, should an inotify component be created in bugzilla?

Created attachment 55345 [details]
inotify_kernel.h

Created attachment 55346 [details]
inotify_kernel.c

Another point that needs discussion is timeouts in the code.

PROCESS_EVENTS_TIME 

How often we should process the event queue trying to pair moves.

Currently 33 milliseconds.

DEFAULT_HOLD_UNTIL_TIME 

How long should we hold an event in the event queue.

Currently 1 millisecond

MOVE_HOLD_UNTIL_TIME

How long should we hold a MOVED_FROM event while we are trying to pair with a
MOVED_TO.

Currently 5 milliseconds

---The above are timers, so the process won't actually be put to sleep.---


MAX_PENDING_COUNT

How many times should we wait for more events to build up in the kernel.

Currently 5

PENDING_PAUSE_MICROSECONDS

How long should the process sleep between each try.

Currently 8 milliseconds

So the process could be put to sleep for 40 milliseconds.

PENDING_PAUSE_MICROSECONDS came from gam_server. 

These last two definitely need to be changed now that this will be per-application.

I want to explain the motivation behind the timer and the timeout. 

Inotify sends seperate MOVED_FROM and MOVED_TO events and because we have a
multi-tasking OS, a unrelated event can sneak in between them. Also, the
MOVED_TO might never come if you aren't watching the destination of the move.

Applications that want to pair the FROMs and TOs together need to "hold" FROM
events until the TO comes or you have held the event for enough time that you
feel confident that the TO will never come. So, when a MOVED_FROM shows up, we
hold it for as long as MOVE_HOLD_UNTIL_TIME. Because the events are ordered, we
have to hold all events behind this FROM. 

The tradeoff here is between not matching MOVED_FROM to a MOVED_TO and holding
up the event stream waiting for the possible MOVED_TO. 

PROCESS_EVENTS_TIME is how often we process the events in this queue.

That covers the timer, now for the timeout.

Applications wait on inotify for events. Technically the process is waiting for
the inotify fd to become readable. When the kernel queues an event, the fd
becomes readable and the process is woken up. Typically the application will
wake up, read the event in, do something, and go back to sleep waiting for
inotify. The kernel will then queue the next event, and this cycle will repeat.
This kills performance. inotify_kernel checks how many events are queued, and
decides if it should sleep a bit longer, letting more events pile up inside the
kernel. This is repeat up to MAX_PENDING_COUNT times. Each time the process
sleeps for PENDING_PAUSE_MICROSECONDS.

The trade off here is IO performance vs. wasted application time.

The current numbers were just pulled from air and used in gamin. They seemed to
work well in the daemon.

Last night I committed an updated version of the backend. It fixes almost all of
the complains against the old one. It is much simpler, and was designed with
gnome-vfs in mind – not gamin. The backend can now map multiple paths to a
single wd, which lets symbolic links work like they should. Also, it watches a
files parent directory instead of the file. This is needed because my changes in
2.6.13 affect the way delete events are delivered. There is a race issue in how
files are handled that also applies to the symbolic link case. I will discuss it
in bug 322348.

The only big item left to implement is watching all the parents of a directory
and delivering MOVE/DELETE events to subfolders.

People interested in not having to run gamin/fam with gnome 2.14 should take a look at bug #314854. 

Quick overview: During the last gnome release, gnome-menus changed from using gnome-vfs-monitor to using FAM/gamin directly, so even though the rest of your desktop will be using inotify, you will still be running the FAM/gamin daemon. I have provided a patch to revert the changes and use gnome-vfs-monitor that was shipped with SUSE 10.

Closing bug as FIXED because according to comment 45, the patch just lacks the parent watching feature, which is covered by bug 331087.