Re: [RFC/PATCH] inotify -- a dnotify replacement

[John McCutchan <ttb@tentacle.dhs.org>]

On Tue, May 11, 2004 at 01:46:47PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Tue, May 11, 2004 at 08:20:01AM -0400, John McCutchan wrote:
>  
> > Inotify will support watching a hierarchy. The reason it was not
> > implemented yet is because the one app that I really care about is
> > nautilus and the maintainer of it says he doesn't care. 
> 
> How are you going to implement that?

>From a quick glance at someone elses implementation of it, I plan on
walking up the dentries and checking at each level if a watcher on that
level is interested in events from subdirectories. Is this good practice in
the kernel?

> > The big feature that inotify is trying to provide is not having to keep
> > a file open (So that unmounting is not affected). I asked for some
> > guidance from people more familiar with the kernel so that I can
> > implement this feature, it requires changes made to the inode cache, and
> > how unmounting is done.
> 
> Bzzert.  First of all, on quite a few filesystems inumbers are stable
> only when object is pinned down.  What's more, if it's not pinned down
> you've got nothing even remotely resembling a reliable way to tell if
> two events had happened to the same object - inumbers can be reused.

The inode will be pinned down, I haven't implemented this yet but I am
going to change the inode cache (is this the right place? )
so that if inode->watcher_count > 0 the inode stays pinned. Then when
the filesystem is unmounted, we will kick off all the watchers on
each inode.

> 
> Besides, your "doesn't pin down" is racy as hell - not to mention the
> way you manage the lists, pretty much every function is an exploitable
> hole.  Hell, just take a look at your "find inode" stuff - you grab
> superblock, find an inode by inumber (great idea, that - especially
> since on a bunch of filesystems it will get you BUG() or equivalent)
> then drop refernce to superblock (at which point it can be destroyed by
> umount()) _and_ do iput() (which will do lovely, lovely things if that
> umount did happen).  Moreover, you return a pointer to inode, even
> though there's nothing to hold it alive anymore.  And dereference that
> pointer later on, not caring if it had been freed/reused/whatever.

Like I said above,as long as an inode has a watcher it will be pinned. As
for the races, I plan on implementing locking around all of the list operations.
Perhaps I wasn't very clear that this is very much a WIP and lots of work is
needed.

> 
> Overall: hopeless crap.  And that's a direct result of your main feature -
> it's really broken by design.

Having directory event notification without needing to keep a file open on the
device is not broken by design. It is the only reasonable solution to
a problem that needs fixing. You can't simply say that a file manager
needing to be notified when directories change is broken. How would
you solve this problem?

John