inotify.
Subsystems (7)
PATCH 1/1 inotify.
Below find an updated inotify, with a doubt the world's great inotify release, against 2.6.12-rc6.
The only change from the last release is a bug fix in the unmount code, fixed by Anton Altaparmakov, that was triggered by NTFS unmount.
Lot's of programs are using inotify and there is a lot of sample code available. A sampling:
Beagle, an Inotify-based Search Infrastructure:
http://beaglewiki.org/
Inotify support for Muine, a music player:
http://www.snorp.net/files/muine-inotify.tar.gz
Gamin, a FAM replacement, supports inotify:
http://www.gnome.org/~veillard/gamin/
Inotify Python bindings:
http://www.amautacorp.com/staff/Rudd-O/projects/pages/se-python-inotify
Test app and Glib bindings:
http://www.kernel.org/pub/linux/kernel/people/rml/
Best,
Robert Love
inotify!
inotify is intended to correct the deficiencies of dnotify, particularly its inability to scale and its terrible user interface:
* dnotify requires the opening of one fd per each directory that you intend to watch. This quickly results in too many open files and pins removable media, preventing unmount.
* dnotify is directory-based. You only learn about changes to directories. Sure, a change to a file in a directory affects the directory, but you are then forced to keep a cache of stat structures.
* dnotify's interface to user-space is awful. Signals?
inotify provides a more usable, simple, powerful solution to file change notification:
* inotify's interface is a device node, not SIGIO. You open a single fd to the device node, which is select()-able.
* inotify has an event that says "the filesystem that the item you were watching is on was unmounted."
* inotify can watch directories or files.
Inotify is currently used by Beagle (a desktop search infrastructure), Gamin (a FAM replacement), and other projects.
See Documentation/filesystems/inotify.txt.
Documentation/filesystems/inotify.txt | 123 ++++ fs/Kconfig | 13 fs/Makefile | 1 fs/attr.c | 33 - fs/compat.c | 12 fs/file_table.c | 3 fs/inode.c | 9 fs/inotify.c | 992 ++++++++++++++++++++++++++++++++++ fs/namei.c | 30 - fs/nfsd/vfs.c | 6 fs/open.c | 4 fs/read_write.c | 15 fs/sysfs/file.c | 7 fs/xattr.c | 5 include/linux/fs.h | 7 include/linux/fsnotify.h | 257 ++++++++ include/linux/inotify.h | 124 ++++ include/linux/sched.h | 4 kernel/user.c | 4 19 files changed, 1585 insertions(+), 64 deletions(-)
Documentation/filesystems/inotify.txt +123 -0
@@ -0,0 +1,123 @@ + inotify + a powerful yet simple file change notification system + + + +Document started 15 Mar 2005 by Robert Love <[email protected]> + +(i) User Interface + +Inotify is controlled by a device node, /dev/inotify. If you do not use udev, +this device may need to be created manually. First step, open it + + int dev_fd = open ("/dev/inotify", O_RDONLY); + +Change events are managed by "watches". A watch is an (object,mask) pair where +the object is a file or directory and the mask is a bitmask of one or more +inotify events that the application wishes to receive. See <linux/inotify.h> +for valid events. A watch is referenced by a watch descriptor, or wd. + +Watches are added via a file descriptor. + +Watches on a directory will return events on any files inside of the directory. + +Adding a watch is simple, + + /* 'wd' represents the watch on fd with mask */ + struct inotify_request req = { fd, mask }; + int wd = ioctl (dev_fd, INOTIFY_WATCH, &req); + +You can add a large number of files via something like + + for each file to watch { + struct inotify_request req; + int file_fd; + + file_fd = open (file, O_RDONLY); + if (fd < 0) { + perror ("open"); + break; + } + + req.fd = file_fd; + req.mask = mask; + + wd = ioctl (dev_fd, INOTIFY_WATCH, &req); + + close (fd); + } + +You can update an existing watch in the same manner, by passing in a new mask. + +An existing watch is removed via the INOTIFY_IGNORE ioctl, for example + + ioctl (dev_fd, INOTIFY_IGNORE, wd); + +Events are provided in the form of an inotify_event structure that is read(2) +from /dev/inotify. The filename is of dynamic length and follows the struct. +It is of size len. The filename is padded with null bytes to ensure proper +alignment. This padding is reflected in len. + +You can slurp multiple events by passing a large buffer, for example + + size_t len = read (fd, buf, BUF_LEN); + +Will return as many events as are available and fit in BUF_LEN. + +/dev/inotify is also select() and poll() able. + +You can find the size of the current event queue via the FIONREAD ioctl. + +All watches are destroyed and cleaned up on close. + + +(ii) Internal Kernel Implementation + +Each open inotify device is associated with an inotify_device structure. + +Each watch is associated with an inotify_watch structure. Watches are chained +off of each associated device and each associated inode. + +See fs/inotify.c for the locking and lifetime rules. + + +(iii) Rationale + +Q: What is the design decision behind not tying the watch to the +open fd of the watched object? + +A: Watches are associated with an open inotify device, not an +open file. This solves the primary problem with dnotify: +keeping the file open pins the file and thus, worse, pins the +mount. Dnotify is therefore infeasible for use on a desktop +system with removable media as the media cannot be unmounted. + +Q: What is the design decision behind using an-fd-per-device as +opposed to an fd-per-watch? + +A: An fd-per-watch quickly consumes more file descriptors than +are allowed, more fd's than are feasible to manage, and more +fd's than are ideally select()-able. Yes, root can bump the +per-process fd limit and yes, users can use epoll, but requiring +both is silly and an extraneous requirement. A watch consumes +less memory than an open file, separating the number spaces is +thus sensible. The current design is what user-space developers +want: Users open the device, once, and add n watches, requiring +but one fd and no twiddling with fd limits. +Opening /dev/inotify two thousand times is silly. If we can +implement user-space's preferences cleanly--and we can, the idr +layer makes stuff like this trivial--then we should. + +Q: Why a device node? + +A: The second biggest problem with dnotify is that the user +interface sucks ass. Signals are a terrible, terrible interface +for file notification. Or for anything, for that matter. The +idea solution, from all perspectives, is a file descriptor based +one that allows basic file I/O and poll/select. Obtaining the +fd and managing the watches could of been done either via a +device file or a family of new system calls. We decided to +implement a device file because adding three or four new system +calls that mirrored open, close, and ioctl seemed silly. A +character device makes sense from user-space and was easy to +implement inside of the kernel.
fs/attr.c +4 -29
@@ -10,7 +10,7 @@#include <linux/mm.h> #include <linux/string.h> #include <linux/smp_lock.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/fcntl.h> #include <linux/quotaops.h> #include <linux/security.h>
@@ -107,31 +107,8 @@ out:return error; } -EXPORT_SYMBOL (inode_setattr); -int setattr_mask (unsigned int ia_valid) -{ -unsigned long dn_mask =0 ; - -if (ia_valid &ATTR_UID ) - dn_mask |=DN_ATTRIB ; -if (ia_valid &ATTR_GID ) - dn_mask |=DN_ATTRIB ; -if (ia_valid &ATTR_SIZE ) - dn_mask |=DN_MODIFY ; -/* both times implies a utime(s) call */ -if ((ia_valid & (ATTR_ATIME |ATTR_MTIME )) == (ATTR_ATIME |ATTR_MTIME )) - dn_mask |=DN_ATTRIB ; -else if (ia_valid &ATTR_ATIME ) - dn_mask |=DN_ACCESS ; -else if (ia_valid &ATTR_MTIME ) - dn_mask |=DN_MODIFY ; -if (ia_valid &ATTR_MODE ) - dn_mask |=DN_ATTRIB ; -return dn_mask; -} -int notify_change (struct dentry * dentry,struct iattr * attr) {struct inode *inode = dentry->d_inode;
@@ -197,11 +174,9 @@if (ia_valid &ATTR_SIZE )up_write (&dentry->d_inode->i_alloc_sem); -if (!error) { -unsigned long dn_mask =setattr_mask (ia_valid); -if (dn_mask) -dnotify_parent (dentry, dn_mask); - } +if (!error) +fsnotify_change (dentry, ia_valid); +return error; }
fs/compat.c +8 -4
@@ -37,7 +37,7 @@#include <linux/ctype.h> #include <linux/module.h> #include <linux/dirent.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/highuid.h> #include <linux/sunrpc/svc.h> #include <linux/nfsd/nfsd.h>
@@ -1307,9 +1307,13 @@ out:if (iov != iovstack)kfree (iov); -if ((ret + (type ==READ )) >0 ) -dnotify_parent (file->f_dentry, - (type ==READ ) ?DN_ACCESS :DN_MODIFY ); +if ((ret + (type ==READ )) >0 ) { +struct dentry *dentry = file->f_dentry; +if (type ==READ ) +fsnotify_access (dentry); +else +fsnotify_modify (dentry); + }return ret; }
fs/file_table.c +3 -0
@@ -16,6 +16,7 @@#include <linux/eventpoll.h> #include <linux/mount.h> #include <linux/cdev.h> +#include <linux/fsnotify.h> /* sysctl tunables... */ struct files_stat_struct files_stat = {
@@ -123,6 +124,8 @@struct inode *inode = dentry->d_inode;might_sleep (); + +fsnotify_close (file);/* * The function eventpoll_release() should be the first called * in the file cleanup chain.
fs/inode.c +9 -0
@@ -21,6 +21,7 @@#include <linux/pagemap.h> #include <linux/cdev.h> #include <linux/bootmem.h> +#include <linux/inotify.h> /* * This is needed for the following functions:
@@ -128,6 +129,9 @@#ifdef CONFIG_QUOTA memset (&inode->i_dquot,0 ,sizeof (inode->i_dquot));#endif +#ifdef CONFIG_INOTIFY +atomic_set (&inode->inotify_watch_count,0 ); +#endif inode->i_pipe =NULL ; inode->i_bdev =NULL ; inode->i_cdev =NULL ;
@@ -202,6 +206,10 @@INIT_LIST_HEAD (&inode->i_data.i_mmap_nonlinear);spin_lock_init (&inode->i_lock);i_size_ordered_init (inode); +#ifdef CONFIG_INOTIFY +INIT_LIST_HEAD (&inode->inotify_watches); +sema_init (&inode->inotify_sem,1 ); +#endif }EXPORT_SYMBOL (inode_init_once);
@@ -346,6 +354,7 @@down (&iprune_sem);spin_lock (&inode_lock); +inotify_unmount_inodes (&sb->s_inodes); busy =invalidate_list (&sb->s_inodes, &throw_away);spin_unlock (&inode_lock);
fs/inotify.c +992 -0
992 lines not on this page · raw message
fs/Kconfig +13 -0
@@ -339,6 +339,19 @@ If you don't know whether you need it, then you don't need it: answer N. +config INOTIFY + bool "Inotify file change notification support" + default y + ---help--- + Say Y here to enable inotify support and the /dev/inotify character + device. Inotify is a file change notification system and a + replacement for dnotify. Inotify fixes numerous shortcomings in + dnotify and introduces several new features. It allows monitoring + of both files and directories via a single open fd. Multiple file + events are supported. + + If unsure, say Y. + config QUOTA bool "Quota support" help
fs/Makefile +1 -0
@@ -11,6 +11,7 @@ attr.o bad_inode.o file.o filesystems.o namespace.o aio.o \ seq_file.o xattr.o libfs.o fs-writeback.o mpage.o direct-io.o \ +obj-$(CONFIG_INOTIFY) += inotify.o obj-$(CONFIG_EPOLL) += eventpoll.o obj-$(CONFIG_COMPAT) += compat.o
fs/namei.c +16 -14
@@ -21,7 +21,7 @@#include <linux/namei.h> #include <linux/quotaops.h> #include <linux/pagemap.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/smp_lock.h> #include <linux/personality.h> #include <linux/security.h>
@@ -1296,7 +1296,7 @@DQUOT_INIT (dir); error = dir->i_op->create (dir, dentry, mode, nd);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, dentry->d_name.name);security_inode_post_create (dir, dentry, mode); }return error;
@@ -1602,7 +1602,7 @@DQUOT_INIT (dir); error = dir->i_op->mknod (dir, dentry, mode, dev);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, dentry->d_name.name);security_inode_post_mknod (dir, dentry, mode, dev); }return error;
@@ -1675,7 +1675,7 @@DQUOT_INIT (dir); error = dir->i_op->mkdir (dir, dentry, mode);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_mkdir (dir, dentry->d_name.name);security_inode_post_mkdir (dir,dentry, mode); }return error;
@@ -1766,7 +1766,7 @@ }up (&dentry->d_inode->i_sem);if (!error) { -inode_dir_notify (dir,DN_DELETE ); +fsnotify_rmdir (dentry, dentry->d_inode, dir);d_delete (dentry); }dput (dentry);
@@ -1839,9 +1839,10 @@/* We don't d_delete() NFS sillyrenamed files--they still exist. */ if (!error && !(dentry->d_flags &DCACHE_NFSFS_RENAMED )) { +fsnotify_unlink (dentry, dir);d_delete (dentry); -inode_dir_notify (dir,DN_DELETE ); } +return error; }
@@ -1915,7 +1916,7 @@DQUOT_INIT (dir); error = dir->i_op->symlink (dir, dentry, oldname);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, dentry->d_name.name);security_inode_post_symlink (dir, dentry, oldname); }return error;
@@ -1988,7 +1989,7 @@ error = dir->i_op->link (old_dentry, dir, new_dentry);up (&old_dentry->d_inode->i_sem);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, new_dentry->d_name.name);security_inode_post_link (old_dentry, dir, new_dentry); }return error;
@@ -2152,6 +2153,7 @@ {int error;int is_dir =S_ISDIR (old_dentry->d_inode->i_mode); +const char *old_name;if (old_dentry->d_inode == new_dentry->d_inode)return 0 ;
@@ -2173,18 +2175,18 @@DQUOT_INIT (old_dir);DQUOT_INIT (new_dir); + old_name =fsnotify_oldname_init (old_dentry->d_name.name); +if (is_dir) error =vfs_rename_dir (old_dir,old_dentry,new_dir,new_dentry);else error =vfs_rename_other (old_dir,old_dentry,new_dir,new_dentry);if (!error) { -if (old_dir == new_dir) -inode_dir_notify (old_dir,DN_RENAME ); -else { -inode_dir_notify (old_dir,DN_DELETE ); -inode_dir_notify (new_dir,DN_CREATE ); - } +const char *new_name = old_dentry->d_name.name; +fsnotify_move (old_dir, new_dir, old_name, new_name, is_dir); } +fsnotify_oldname_free (old_name); +return error; }
fs/nfsd/vfs.c +3 -3
@@ -45,7 +45,7 @@#endif /* CONFIG_NFSD_V3 */ #include <linux/nfsd/nfsfh.h> #include <linux/quotaops.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #ifdef CONFIG_NFSD_V4 #include <linux/posix_acl.h> #include <linux/posix_acl_xattr.h>
@@ -861,7 +861,7 @@ nfsdstats.io_read += err; *count = err; err =0 ; -dnotify_parent (file->f_dentry,DN_ACCESS ); +fsnotify_access (file->f_dentry); }else err =nfserrno (err); out:
@@ -917,7 +917,7 @@set_fs (oldfs);if (err >=0 ) { nfsdstats.io_write += cnt; -dnotify_parent (file->f_dentry,DN_MODIFY ); +fsnotify_modify (file->f_dentry); }/* clear setuid/setgid flag after write */
fs/open.c +3 -1
@@ -10,7 +10,7 @@#include <linux/file.h> #include <linux/smp_lock.h> #include <linux/quotaops.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/module.h> #include <linux/slab.h> #include <linux/tty.h>
@@ -944,9 +944,11 @@ fd =get_unused_fd ();if (fd >=0 ) {struct file *f =filp_open (tmp, flags, mode); + error =PTR_ERR (f);if (IS_ERR (f)) goto out_error; +fsnotify_open (f->f_dentry);fd_install (fd, f); } out:
fs/read_write.c +9 -6
@@ -10,7 +10,7 @@#include <linux/file.h> #include <linux/uio.h> #include <linux/smp_lock.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/security.h> #include <linux/module.h> #include <linux/syscalls.h>
@@ -239,7 +239,7 @@else ret =do_sync_read (file, buf, count, pos);if (ret >0 ) { -dnotify_parent (file->f_dentry,DN_ACCESS ); +fsnotify_access (file->f_dentry); current->rchar += ret; } current->syscr++;
@@ -287,7 +287,7 @@else ret =do_sync_write (file, buf, count, pos);if (ret >0 ) { -dnotify_parent (file->f_dentry,DN_MODIFY ); +fsnotify_modify (file->f_dentry); current->wchar += ret; } current->syscw++;
@@ -523,9 +523,12 @@ out:if (iov != iovstack)kfree (iov); -if ((ret + (type ==READ )) >0 ) -dnotify_parent (file->f_dentry, - (type ==READ ) ?DN_ACCESS :DN_MODIFY ); +if ((ret + (type ==READ )) >0 ) { +if (type ==READ ) +fsnotify_access (file->f_dentry); +else +fsnotify_modify (file->f_dentry); + }return ret; Efault: ret = -EFAULT ;
fs/sysfs/file.c +2 -5
@@ -3,7 +3,7 @@*/ #include <linux/module.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/kobject.h> #include <asm/uaccess.h> #include <asm/semaphore.h>
@@ -389,9 +389,6 @@* sysfs_update_file - update the modified timestamp on an object attribute. * @kobj: object we're acting for. * @attr: attribute descriptor. -* -* Also call dnotify for the dentry, which lots of userspace programs -* use. */ int sysfs_update_file (struct kobject * kobj,const struct attribute * attr) {
@@ -406,7 +403,7 @@if (victim->d_inode && (victim->d_parent->d_inode == dir->d_inode)) { victim->d_inode->i_mtime =CURRENT_TIME ; -dnotify_parent (victim,DN_MODIFY ); +fsnotify_modify (victim);/** * Drop reference from initial sysfs_get_dentry().
fs/xattr.c +4 -1
@@ -16,6 +16,7 @@#include <linux/security.h> #include <linux/syscalls.h> #include <linux/module.h> +#include <linux/fsnotify.h> #include <asm/uaccess.h> /*
@@ -57,8 +58,10 @@if (error) goto out; error = d->d_inode->i_op->setxattr (d, kname, kvalue, size, flags); -if (!error) +if (!error) { +fsnotify_xattr (d);security_inode_post_setxattr (d, kname, kvalue, size, flags); + } out:up (&d->d_inode->i_sem); }
include/linux/fs.h +6 -1
@@ -471,6 +471,12 @@struct dnotify_struct *i_dnotify;/* for directory notifications */ #endif +#ifdef CONFIG_INOTIFY +atomic_t inotify_watch_count;/* number of watches */ +struct list_head inotify_watches;/* watches on this inode */ +struct semaphore inotify_sem;/* protects the watches list */ +#endif +unsigned long i_state;unsigned long dirtied_when;/* jiffies of first dirtying */
@@ -1369,7 +1375,6 @@extern int do_remount_sb (struct super_block *sb,int flags,void *data,int force);extern sector_t bmap (struct inode *,sector_t ); -extern int setattr_mask (unsigned int );extern int notify_change (struct dentry *,struct iattr *);extern int permission (struct inode *,int ,struct nameidata *); extern int generic_permission(struct inode *, int,
include/linux/fsnotify.h +257 -0
257 lines not on this page · raw message
include/linux/inotify.h +124 -0
124 lines not on this page · raw message
include/linux/sched.h +4 -0
@@ -404,6 +404,10 @@atomic_t processes;/* How many processes does this user have? */ atomic_t files;/* How many open files does this user have? */ atomic_t sigpending;/* How many pending signals does this user have? */ +#ifdef CONFIG_INOTIFY +atomic_t inotify_watches;/* How many inotify watches does this user have? */ +atomic_t inotify_devs;/* How many inotify devs does this user have opened? */ +#endif /* protected by mq_lock */ unsigned long mq_bytes;/* How many bytes can be allocated to mqueue? */ unsigned long locked_shm;/* How many pages of mlocked shm ? */
kernel/user.c +4 -0
@@ -120,6 +120,10 @@atomic_set (&new->processes,0 );atomic_set (&new->files,0 );atomic_set (&new->sigpending,0 ); +#ifdef CONFIG_INOTIFY +atomic_set (&new->inotify_watches,0 ); +atomic_set (&new->inotify_devs,0 ); +#endif new->mq_bytes =0 ; new->locked_shm =0 ;
Quoted 2 lines · Robert Love
Below find an updated inotify, with a doubt the world's great inotify release, against 2.6.12-rc6.
some quick comments, nothing earth shattering..
Review
Quoted 1 line
(quoted diff) +0
-0@@ -0,0 +1,123 @@
diff -p, please.
fs/inotify.c line 94
Quoted 1 line
fs/inotify.c +1
-0+* struct inotify_kernel_event - An intofiy event, originating from a watch and
intofiy!
Quoted 7 lines
fs/inotify.c +7
-0+/* +* inotify_dev_is_watching_inode - is this device watching this inode? +* +* Callers must hold dev->sem. +*/ +static inline int inotify_dev_is_watching_inode (struct inotify_device *dev, +struct inode *inode)
fs/inotify.c line 459
Quoted 10 lines
fs/inotify.c +10
-0+{ +struct inotify_watch *watch; + +list_for_each_entry (watch, &dev->watches, d_list) { +if (watch->inode == inode) +return 1 ; + } + +return 0 ; +}
Am I missing where this is used?
fs/inotify.c line 508
Quoted 2 lines
fs/inotify.c +2
-0+if (likely (!atomic_read (&inode->inotify_watch_count))) +return ;
Are there still platforms that implement atomic_read() with locks? I wonder if there isn't a cheaper way for inodes to find out that they're not involved in inotify.. maybe an inode function pointer that is only set to queue_event when watchers are around?
fs/inotify.c line 517
Quoted 8 lines
fs/inotify.c +8
-0+down (&inode->inotify_sem); +list_for_each_entry_safe (watch, next, &inode->inotify_watches, i_list) { +u32 watch_mask = watch->mask; +if (watch_mask & mask) { +struct inotify_device *dev = watch->dev; +get_inotify_watch (watch); +down (&dev->sem); +inotify_dev_queue_event (dev, watch, mask, cookie, name);
I wonder if its worth walking the watches as they're added and removed to calculate a total mask of what is being watched so that uninteresting events can be immediately ignored without walking the watches.
I also wonder about adding references to an event from each device that is watching the inode rather than queueing events on all the devices. If it only ever copies the full event to user space anyway, you'd just need a series of pointers on the dev that hold references to a refcounted event. It looks like events are only ever dequeued from the device as they're walked from the head of the list so it doesn't seem like the ability to remove an event from the middle would be missed.
Too aggressive for a situation that rarely happens anyway?
Quoted 17 lines
fs/inotify.c +17
-0+while (1 ) { +int events; + +prepare_to_wait (&dev->wq, &wait,TASK_INTERRUPTIBLE ); + +down (&dev->sem); + events = !list_empty (&dev->events); +up (&dev->sem); +if (events) { + ret =0 ; +break ; + } + +if (file->f_flags &O_NONBLOCK ) { + ret = -EAGAIN ; +break ; + }
fs/inotify.c line 688
Quoted 10 lines
fs/inotify.c +10
-0+ +if (signal_pending (current)) { + ret = -EINTR ; +break ; + } + +schedule (); + } + +finish_wait (&dev->wq, &wait);
It'd be nice to get some wait_event_interruptible() love to replace the hand-rolled _wait()s and schedule(). It could probably be folded into the second loop, I imagine. I suspect that the signal_pending() case wants to return ERESTARTSYS like w_e_i() does.
fs/inotify.c line 762
Quoted 1 line
fs/inotify.c +1
-0+nonseekable_open (inode, file);
While it doesn't return errors today, it might be nice to test anyway.
include/linux/fsnotify.h line 26
Quoted 8 lines
include/linux/fsnotify.h +8
-0+/* +* fsnotify_move - file old_name at old_dir was moved to new_name at new_dir +*/ +static inline void fsnotify_move (struct inode *old_dir,struct inode *new_dir, +const char *old_name,const char *new_name, +int isdir) +{ +u32 cookie =inotify_get_cookie ();
How about deferring this atomic_inc() until it's known if the event is going to be queued or not so that renames that have nothing to do with inotify don't bounce the cache line around? If it's *only* used to assciate two different events with a single rename maybe it'd be better to roll some compound rename event.
I hope this helps. I, for one, would be thrilled to see dnotify deprecated.
- z
Quoted 1 line · Zach Brown
some quick comments, nothing earth shattering..
Thanks, zab!
Quoted 3 lines
Quoted 1 line
+ * struct
inotify_kernel_event- An intofiy event, originating from a watch andintofiy!
Fixed.
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+/* + * inotify_dev_is_watching_inode - is this device watching this inode? + * + * Callers must hold dev->sem. + */ +static inline int inotify_dev_is_watching_inode(struct inotify_device *dev, + struct inode *inode) +{ + struct inotify_watch *watch; + + list_for_each_entry(watch, &dev->watches, d_list) {
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ if (watch->inode == inode) + return 1; + } + + return 0; +}Am I missing where this is used?
Nope. I am not using it anymore. Removed.
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (likely(!atomic_read(&inode->inotify_watch_count))) + return;Are there still platforms that implement
atomic_read()with locks? I wonder if there isn't a cheaper way for inodes to find out that they're not involved in inotify.. maybe an inode function pointer that is only set toqueue_eventwhen watchers are around?
I don't know what esoteric architectures are doing, but the solution needs to be atomic (or we need to say "we don't care about races"--but its hard not to care about a pointer race). On x86, at least, an atomic_read() is trivial.
I actually would not mind being racey (in a safe way) or finding a cheaper solution, especially if we could remove inode->inotify_watch_count altogether (and not replace it with anything).
But the overhead here is not biting us (we just went through some off-list benchmarking that led to the inclusion of this check, in fact).
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+ down(&inode->inotify_sem); + list_for_each_entry_safe(watch, next, &inode->inotify_watches, i_list) { + u32 watch_mask = watch->mask; + if (watch_mask & mask) {
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ struct inotify_device *dev = watch->dev; + get_inotify_watch(watch); + down(&dev->sem); + inotify_dev_queue_event(dev, watch, mask, cookie, name);I wonder if its worth walking the watches as they're added and removed to calculate a total mask of what is being watched so that uninteresting events can be immediately ignored without walking the watches.
We used to do this. I found it wasn't really worth it, performance-wise. And--I forget the details now--there was a locking concern in caching the value. Like, where I usually grab the locks A->B, caching the mask led me to a B->A situation. Or something.
Quoted 9 lines
I also wonder about adding references to an event from each device that is watching the inode rather than queueing events on all the devices. If it only ever copies the full event to user space anyway, you'd just need a series of pointers on the dev that hold references to a refcounted event. It looks like events are only ever dequeued from the device as they're walked from the head of the list so it doesn't seem like the ability to remove an event from the middle would be missed.
Too aggressive for a situation that rarely happens anyway?
_Probably_ way too aggressive for this situation, although I would not rule it out. This is always an optimization that we can do later, if something proves expensive, but I don't see it right now.
Quoted 4 lines
It'd be nice to get some
wait_event_interruptible()love to replace the hand-rolled _wait()s andschedule(). It could probably be folded into the second loop, I imagine. I suspect that thesignal_pending()case wants to return ERESTARTSYS likew_e_i()does.
Yah, agreed.
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ nonseekable_open(inode, file);While it doesn't return errors today, it might be nice to test anyway.
Sure. Done.
Quoted 6 lines
Quoted 6 lines
(quoted diff) +6
-0+/* + * fsnotify_move - file old_name at old_dir was moved to new_name at new_dir + */ +static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir, + const char *old_name, const char *new_name, + int isdir)
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+{ + u32 cookie = inotify_get_cookie();How about deferring this
atomic_inc()until it's known if the event is going to be queued or not so that renames that have nothing to do with inotify don't bounce the cache line around? If it's *only* used to assciate two different events with a single rename maybe it'd be better to roll some compound rename event.
It is hard, because we don't know if the user is watching the src and dst until we actually get there and see that they are--we'd have to walk every watch to find if they match and then, if so, increment the cookie and send it out to those two watches.
Quoted 1 line
I hope this helps. I, for one, would be thrilled to see dnotify deprecated.
It helps a lot. Thank you very much for the pair of eyes, Zach.
I appreciate it.
Updated patch below!
Best,
Robert Love
inotify!
inotify is intended to correct the deficiencies of dnotify, particularly its inability to scale and its terrible user interface:
* dnotify requires the opening of one fd per each directory that you intend to watch. This quickly results in too many open files and pins removable media, preventing unmount.
* dnotify is directory-based. You only learn about changes to directories. Sure, a change to a file in a directory affects the directory, but you are then forced to keep a cache of stat structures.
* dnotify's interface to user-space is awful. Signals?
inotify provides a more usable, simple, powerful solution to file change notification:
* inotify's interface is a device node, not SIGIO. You open a single fd to the device node, which is select()-able.
* inotify has an event that says "the filesystem that the item you were watching is on was unmounted."
* inotify can watch directories or files.
Inotify is currently used by Beagle (a desktop search infrastructure), Gamin (a FAM replacement), and other projects.
See Documentation/filesystems/inotify.txt.
Documentation/filesystems/inotify.txt | 123 ++++ fs/Kconfig | 13 fs/Makefile | 1 fs/attr.c | 33 - fs/compat.c | 12 fs/file_table.c | 3 fs/inode.c | 9 fs/inotify.c | 976 ++++++++++++++++++++++++++++++++++ fs/namei.c | 30 - fs/nfsd/vfs.c | 6 fs/open.c | 4 fs/read_write.c | 15 fs/sysfs/file.c | 7 fs/xattr.c | 5 include/linux/fs.h | 7 include/linux/fsnotify.h | 257 ++++++++ include/linux/inotify.h | 124 ++++ include/linux/sched.h | 4 kernel/user.c | 4 19 files changed, 1569 insertions(+), 64 deletions(-)
Documentation/filesystems/inotify.txt +123 -0
@@ -0,0 +1,123 @@ + inotify + a powerful yet simple file change notification system + + + +Document started 15 Mar 2005 by Robert Love <[email protected]> + +(i) User Interface + +Inotify is controlled by a device node, /dev/inotify. If you do not use udev, +this device may need to be created manually. First step, open it + + int dev_fd = open ("/dev/inotify", O_RDONLY); + +Change events are managed by "watches". A watch is an (object,mask) pair where +the object is a file or directory and the mask is a bitmask of one or more +inotify events that the application wishes to receive. See <linux/inotify.h> +for valid events. A watch is referenced by a watch descriptor, or wd. + +Watches are added via a file descriptor. + +Watches on a directory will return events on any files inside of the directory. + +Adding a watch is simple, + + /* 'wd' represents the watch on fd with mask */ + struct inotify_request req = { fd, mask }; + int wd = ioctl (dev_fd, INOTIFY_WATCH, &req); + +You can add a large number of files via something like + + for each file to watch { + struct inotify_request req; + int file_fd; + + file_fd = open (file, O_RDONLY); + if (fd < 0) { + perror ("open"); + break; + } + + req.fd = file_fd; + req.mask = mask; + + wd = ioctl (dev_fd, INOTIFY_WATCH, &req); + + close (fd); + } + +You can update an existing watch in the same manner, by passing in a new mask. + +An existing watch is removed via the INOTIFY_IGNORE ioctl, for example + + ioctl (dev_fd, INOTIFY_IGNORE, wd); + +Events are provided in the form of an inotify_event structure that is read(2) +from /dev/inotify. The filename is of dynamic length and follows the struct. +It is of size len. The filename is padded with null bytes to ensure proper +alignment. This padding is reflected in len. + +You can slurp multiple events by passing a large buffer, for example + + size_t len = read (fd, buf, BUF_LEN); + +Will return as many events as are available and fit in BUF_LEN. + +/dev/inotify is also select() and poll() able. + +You can find the size of the current event queue via the FIONREAD ioctl. + +All watches are destroyed and cleaned up on close. + + +(ii) Internal Kernel Implementation + +Each open inotify device is associated with an inotify_device structure. + +Each watch is associated with an inotify_watch structure. Watches are chained +off of each associated device and each associated inode. + +See fs/inotify.c for the locking and lifetime rules. + + +(iii) Rationale + +Q: What is the design decision behind not tying the watch to the +open fd of the watched object? + +A: Watches are associated with an open inotify device, not an +open file. This solves the primary problem with dnotify: +keeping the file open pins the file and thus, worse, pins the +mount. Dnotify is therefore infeasible for use on a desktop +system with removable media as the media cannot be unmounted. + +Q: What is the design decision behind using an-fd-per-device as +opposed to an fd-per-watch? + +A: An fd-per-watch quickly consumes more file descriptors than +are allowed, more fd's than are feasible to manage, and more +fd's than are ideally select()-able. Yes, root can bump the +per-process fd limit and yes, users can use epoll, but requiring +both is silly and an extraneous requirement. A watch consumes +less memory than an open file, separating the number spaces is +thus sensible. The current design is what user-space developers +want: Users open the device, once, and add n watches, requiring +but one fd and no twiddling with fd limits. +Opening /dev/inotify two thousand times is silly. If we can +implement user-space's preferences cleanly--and we can, the idr +layer makes stuff like this trivial--then we should. + +Q: Why a device node? + +A: The second biggest problem with dnotify is that the user +interface sucks ass. Signals are a terrible, terrible interface +for file notification. Or for anything, for that matter. The +idea solution, from all perspectives, is a file descriptor based +one that allows basic file I/O and poll/select. Obtaining the +fd and managing the watches could of been done either via a +device file or a family of new system calls. We decided to +implement a device file because adding three or four new system +calls that mirrored open, close, and ioctl seemed silly. A +character device makes sense from user-space and was easy to +implement inside of the kernel.
fs/attr.c +4 -29
@@ -10,7 +10,7 @@#include <linux/mm.h> #include <linux/string.h> #include <linux/smp_lock.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/fcntl.h> #include <linux/quotaops.h> #include <linux/security.h>
@@ -107,31 +107,8 @@ out:return error; } -EXPORT_SYMBOL (inode_setattr); -int setattr_mask (unsigned int ia_valid) -{ -unsigned long dn_mask =0 ; - -if (ia_valid &ATTR_UID ) - dn_mask |=DN_ATTRIB ; -if (ia_valid &ATTR_GID ) - dn_mask |=DN_ATTRIB ; -if (ia_valid &ATTR_SIZE ) - dn_mask |=DN_MODIFY ; -/* both times implies a utime(s) call */ -if ((ia_valid & (ATTR_ATIME |ATTR_MTIME )) == (ATTR_ATIME |ATTR_MTIME )) - dn_mask |=DN_ATTRIB ; -else if (ia_valid &ATTR_ATIME ) - dn_mask |=DN_ACCESS ; -else if (ia_valid &ATTR_MTIME ) - dn_mask |=DN_MODIFY ; -if (ia_valid &ATTR_MODE ) - dn_mask |=DN_ATTRIB ; -return dn_mask; -} -int notify_change (struct dentry * dentry,struct iattr * attr) {struct inode *inode = dentry->d_inode;
@@ -197,11 +174,9 @@if (ia_valid &ATTR_SIZE )up_write (&dentry->d_inode->i_alloc_sem); -if (!error) { -unsigned long dn_mask =setattr_mask (ia_valid); -if (dn_mask) -dnotify_parent (dentry, dn_mask); - } +if (!error) +fsnotify_change (dentry, ia_valid); +return error; }
fs/compat.c +8 -4
@@ -37,7 +37,7 @@#include <linux/ctype.h> #include <linux/module.h> #include <linux/dirent.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/highuid.h> #include <linux/sunrpc/svc.h> #include <linux/nfsd/nfsd.h>
@@ -1307,9 +1307,13 @@ out:if (iov != iovstack)kfree (iov); -if ((ret + (type ==READ )) >0 ) -dnotify_parent (file->f_dentry, - (type ==READ ) ?DN_ACCESS :DN_MODIFY ); +if ((ret + (type ==READ )) >0 ) { +struct dentry *dentry = file->f_dentry; +if (type ==READ ) +fsnotify_access (dentry); +else +fsnotify_modify (dentry); + }return ret; }
fs/file_table.c +3 -0
@@ -16,6 +16,7 @@#include <linux/eventpoll.h> #include <linux/mount.h> #include <linux/cdev.h> +#include <linux/fsnotify.h> /* sysctl tunables... */ struct files_stat_struct files_stat = {
@@ -123,6 +124,8 @@struct inode *inode = dentry->d_inode;might_sleep (); + +fsnotify_close (file);/* * The function eventpoll_release() should be the first called * in the file cleanup chain.
fs/inode.c +9 -0
@@ -21,6 +21,7 @@#include <linux/pagemap.h> #include <linux/cdev.h> #include <linux/bootmem.h> +#include <linux/inotify.h> /* * This is needed for the following functions:
@@ -128,6 +129,9 @@#ifdef CONFIG_QUOTA memset (&inode->i_dquot,0 ,sizeof (inode->i_dquot));#endif +#ifdef CONFIG_INOTIFY +atomic_set (&inode->inotify_watch_count,0 ); +#endif inode->i_pipe =NULL ; inode->i_bdev =NULL ; inode->i_cdev =NULL ;
@@ -202,6 +206,10 @@INIT_LIST_HEAD (&inode->i_data.i_mmap_nonlinear);spin_lock_init (&inode->i_lock);i_size_ordered_init (inode); +#ifdef CONFIG_INOTIFY +INIT_LIST_HEAD (&inode->inotify_watches); +sema_init (&inode->inotify_sem,1 ); +#endif }EXPORT_SYMBOL (inode_init_once);
@@ -346,6 +354,7 @@down (&iprune_sem);spin_lock (&inode_lock); +inotify_unmount_inodes (&sb->s_inodes); busy =invalidate_list (&sb->s_inodes, &throw_away);spin_unlock (&inode_lock);
fs/inotify.c +976 -0
976 lines not on this page · raw message
fs/Kconfig +13 -0
@@ -339,6 +339,19 @@ If you don't know whether you need it, then you don't need it: answer N. +config INOTIFY + bool "Inotify file change notification support" + default y + ---help--- + Say Y here to enable inotify support and the /dev/inotify character + device. Inotify is a file change notification system and a + replacement for dnotify. Inotify fixes numerous shortcomings in + dnotify and introduces several new features. It allows monitoring + of both files and directories via a single open fd. Multiple file + events are supported. + + If unsure, say Y. + config QUOTA bool "Quota support" help
fs/Makefile +1 -0
@@ -11,6 +11,7 @@ attr.o bad_inode.o file.o filesystems.o namespace.o aio.o \ seq_file.o xattr.o libfs.o fs-writeback.o mpage.o direct-io.o \ +obj-$(CONFIG_INOTIFY) += inotify.o obj-$(CONFIG_EPOLL) += eventpoll.o obj-$(CONFIG_COMPAT) += compat.o
fs/namei.c +16 -14
@@ -21,7 +21,7 @@#include <linux/namei.h> #include <linux/quotaops.h> #include <linux/pagemap.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/smp_lock.h> #include <linux/personality.h> #include <linux/security.h>
@@ -1296,7 +1296,7 @@DQUOT_INIT (dir); error = dir->i_op->create (dir, dentry, mode, nd);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, dentry->d_name.name);security_inode_post_create (dir, dentry, mode); }return error;
@@ -1602,7 +1602,7 @@DQUOT_INIT (dir); error = dir->i_op->mknod (dir, dentry, mode, dev);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, dentry->d_name.name);security_inode_post_mknod (dir, dentry, mode, dev); }return error;
@@ -1675,7 +1675,7 @@DQUOT_INIT (dir); error = dir->i_op->mkdir (dir, dentry, mode);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_mkdir (dir, dentry->d_name.name);security_inode_post_mkdir (dir,dentry, mode); }return error;
@@ -1766,7 +1766,7 @@ }up (&dentry->d_inode->i_sem);if (!error) { -inode_dir_notify (dir,DN_DELETE ); +fsnotify_rmdir (dentry, dentry->d_inode, dir);d_delete (dentry); }dput (dentry);
@@ -1839,9 +1839,10 @@/* We don't d_delete() NFS sillyrenamed files--they still exist. */ if (!error && !(dentry->d_flags &DCACHE_NFSFS_RENAMED )) { +fsnotify_unlink (dentry, dir);d_delete (dentry); -inode_dir_notify (dir,DN_DELETE ); } +return error; }
@@ -1915,7 +1916,7 @@DQUOT_INIT (dir); error = dir->i_op->symlink (dir, dentry, oldname);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, dentry->d_name.name);security_inode_post_symlink (dir, dentry, oldname); }return error;
@@ -1988,7 +1989,7 @@ error = dir->i_op->link (old_dentry, dir, new_dentry);up (&old_dentry->d_inode->i_sem);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, new_dentry->d_name.name);security_inode_post_link (old_dentry, dir, new_dentry); }return error;
@@ -2152,6 +2153,7 @@ {int error;int is_dir =S_ISDIR (old_dentry->d_inode->i_mode); +const char *old_name;if (old_dentry->d_inode == new_dentry->d_inode)return 0 ;
@@ -2173,18 +2175,18 @@DQUOT_INIT (old_dir);DQUOT_INIT (new_dir); + old_name =fsnotify_oldname_init (old_dentry->d_name.name); +if (is_dir) error =vfs_rename_dir (old_dir,old_dentry,new_dir,new_dentry);else error =vfs_rename_other (old_dir,old_dentry,new_dir,new_dentry);if (!error) { -if (old_dir == new_dir) -inode_dir_notify (old_dir,DN_RENAME ); -else { -inode_dir_notify (old_dir,DN_DELETE ); -inode_dir_notify (new_dir,DN_CREATE ); - } +const char *new_name = old_dentry->d_name.name; +fsnotify_move (old_dir, new_dir, old_name, new_name, is_dir); } +fsnotify_oldname_free (old_name); +return error; }
fs/nfsd/vfs.c +3 -3
@@ -45,7 +45,7 @@#endif /* CONFIG_NFSD_V3 */ #include <linux/nfsd/nfsfh.h> #include <linux/quotaops.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #ifdef CONFIG_NFSD_V4 #include <linux/posix_acl.h> #include <linux/posix_acl_xattr.h>
@@ -861,7 +861,7 @@ nfsdstats.io_read += err; *count = err; err =0 ; -dnotify_parent (file->f_dentry,DN_ACCESS ); +fsnotify_access (file->f_dentry); }else err =nfserrno (err); out:
@@ -917,7 +917,7 @@set_fs (oldfs);if (err >=0 ) { nfsdstats.io_write += cnt; -dnotify_parent (file->f_dentry,DN_MODIFY ); +fsnotify_modify (file->f_dentry); }/* clear setuid/setgid flag after write */
fs/open.c +3 -1
@@ -10,7 +10,7 @@#include <linux/file.h> #include <linux/smp_lock.h> #include <linux/quotaops.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/module.h> #include <linux/slab.h> #include <linux/tty.h>
@@ -944,9 +944,11 @@ fd =get_unused_fd ();if (fd >=0 ) {struct file *f =filp_open (tmp, flags, mode); + error =PTR_ERR (f);if (IS_ERR (f)) goto out_error; +fsnotify_open (f->f_dentry);fd_install (fd, f); } out:
fs/read_write.c +9 -6
@@ -10,7 +10,7 @@#include <linux/file.h> #include <linux/uio.h> #include <linux/smp_lock.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/security.h> #include <linux/module.h> #include <linux/syscalls.h>
@@ -239,7 +239,7 @@else ret =do_sync_read (file, buf, count, pos);if (ret >0 ) { -dnotify_parent (file->f_dentry,DN_ACCESS ); +fsnotify_access (file->f_dentry); current->rchar += ret; } current->syscr++;
@@ -287,7 +287,7 @@else ret =do_sync_write (file, buf, count, pos);if (ret >0 ) { -dnotify_parent (file->f_dentry,DN_MODIFY ); +fsnotify_modify (file->f_dentry); current->wchar += ret; } current->syscw++;
@@ -523,9 +523,12 @@ out:if (iov != iovstack)kfree (iov); -if ((ret + (type ==READ )) >0 ) -dnotify_parent (file->f_dentry, - (type ==READ ) ?DN_ACCESS :DN_MODIFY ); +if ((ret + (type ==READ )) >0 ) { +if (type ==READ ) +fsnotify_access (file->f_dentry); +else +fsnotify_modify (file->f_dentry); + }return ret; Efault: ret = -EFAULT ;
fs/sysfs/file.c +2 -5
@@ -3,7 +3,7 @@*/ #include <linux/module.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/kobject.h> #include <asm/uaccess.h> #include <asm/semaphore.h>
@@ -389,9 +389,6 @@* sysfs_update_file - update the modified timestamp on an object attribute. * @kobj: object we're acting for. * @attr: attribute descriptor. -* -* Also call dnotify for the dentry, which lots of userspace programs -* use. */ int sysfs_update_file (struct kobject * kobj,const struct attribute * attr) {
@@ -406,7 +403,7 @@if (victim->d_inode && (victim->d_parent->d_inode == dir->d_inode)) { victim->d_inode->i_mtime =CURRENT_TIME ; -dnotify_parent (victim,DN_MODIFY ); +fsnotify_modify (victim);/** * Drop reference from initial sysfs_get_dentry().
fs/xattr.c +4 -1
@@ -16,6 +16,7 @@#include <linux/security.h> #include <linux/syscalls.h> #include <linux/module.h> +#include <linux/fsnotify.h> #include <asm/uaccess.h> /*
@@ -57,8 +58,10 @@if (error) goto out; error = d->d_inode->i_op->setxattr (d, kname, kvalue, size, flags); -if (!error) +if (!error) { +fsnotify_xattr (d);security_inode_post_setxattr (d, kname, kvalue, size, flags); + } out:up (&d->d_inode->i_sem); }
include/linux/fs.h +6 -1
@@ -471,6 +471,12 @@struct dnotify_struct *i_dnotify;/* for directory notifications */ #endif +#ifdef CONFIG_INOTIFY +atomic_t inotify_watch_count;/* number of watches */ +struct list_head inotify_watches;/* watches on this inode */ +struct semaphore inotify_sem;/* protects the watches list */ +#endif +unsigned long i_state;unsigned long dirtied_when;/* jiffies of first dirtying */
@@ -1369,7 +1375,6 @@extern int do_remount_sb (struct super_block *sb,int flags,void *data,int force);extern sector_t bmap (struct inode *,sector_t ); -extern int setattr_mask (unsigned int );extern int notify_change (struct dentry *,struct iattr *);extern int permission (struct inode *,int ,struct nameidata *); extern int generic_permission(struct inode *, int,
include/linux/fsnotify.h +257 -0
257 lines not on this page · raw message
include/linux/inotify.h +124 -0
124 lines not on this page · raw message
include/linux/sched.h +4 -0
@@ -404,6 +404,10 @@atomic_t processes;/* How many processes does this user have? */ atomic_t files;/* How many open files does this user have? */ atomic_t sigpending;/* How many pending signals does this user have? */ +#ifdef CONFIG_INOTIFY +atomic_t inotify_watches;/* How many inotify watches does this user have? */ +atomic_t inotify_devs;/* How many inotify devs does this user have opened? */ +#endif /* protected by mq_lock */ unsigned long mq_bytes;/* How many bytes can be allocated to mqueue? */ unsigned long locked_shm;/* How many pages of mlocked shm ? */
kernel/user.c +4 -0
@@ -120,6 +120,10 @@atomic_set (&new->processes,0 );atomic_set (&new->files,0 );atomic_set (&new->sigpending,0 ); +#ifdef CONFIG_INOTIFY +atomic_set (&new->inotify_watches,0 ); +atomic_set (&new->inotify_devs,0 ); +#endif new->mq_bytes =0 ; new->locked_shm =0 ;
Quoted 16 lines · Robert Love
Quoted 7 lines · Zach Brown
fs/inotify.c line 490
Quoted 2 lines
fs/inotify.c +2
-0+if (likely (!atomic_read (&inode->inotify_watch_count))) +return ;Are there still platforms that implement
atomic_read()with locks? I wonder if there isn't a cheaper way for inodes to find out that they're not involved in inotify.. maybe an inode function pointer that is only set toqueue_eventwhen watchers are around?I don't know what esoteric architectures are doing, but the solution needs to be atomic (or we need to say "we don't care about races"--but its hard not to care about a pointer race). On x86, at least, an
atomic_read()is trivial.
Quoted 8 lines
I actually would not mind being racey (in a safe way) or finding a cheaper solution, especially if we could remove
inode->inotify_watch_countaltogether (and not replace it with anything).But the overhead here is not biting us (we just went through some off-list benchmarking that led to the inclusion of this check, in fact).
What we could do is just check list_empty(&inode->inotify_watchers) and remove the atomic count completely.
We don't actually care about getting an exact count at all, just whether or not it is empty, and in that case using list_empty is no more racy than checking an atomic count, both are done outside any locks.
It is basically just a lock avoidance heuristic. But I think count is superfluous - off with its head!
Signature · 3 lines
SUSE Labs, Novell Inc. Send instant messages to your online friends http://au.messenger.yahoo.com
Quoted 10 lines · Nick Piggin
What we could do is just check
list_empty(&inode->inotify_watchers)and remove the atomic count completely.We don't actually care about getting an exact count at all, just whether or not it is empty, and in that case using
list_emptyis no more racy than checking an atomic count, both are done outside any locks.It is basically just a lock avoidance heuristic. But I think count is superfluous - off with its head!
Yah. This is what I originally did. Because of the current implementation of list_empty(), the race is never dangerous (e.g., no segfault), we just get false positives/negatives, but that is no different than the atomic check, really, since we can race in the interim and then we go on and check the list anyhow.
So, let's do it. I'll cook up a patch and updated inotify tomorrow, if no one beats me to the punch.
Robert Love
PATCH 1/1 inotify, improved.
Quoted 8 lines · Robert Love
Yah. This is what I originally did. Because of the current implementation of
list_empty(), the race is never dangerous (e.g., no segfault), we just get false positives/negatives, but that is no different than the atomic check, really, since we can race in the interim and then we go on and check the list anyhow.So, let's do it. I'll cook up a patch and updated inotify tomorrow, if no one beats me to the punch.
Attached patch removes the atomics and does a lockless list check.
Robert Love
inotify!
inotify is intended to correct the deficiencies of dnotify, particularly its inability to scale and its terrible user interface:
* dnotify requires the opening of one fd per each directory that you intend to watch. This quickly results in too many open files and pins removable media, preventing unmount.
* dnotify is directory-based. You only learn about changes to directories. Sure, a change to a file in a directory affects the directory, but you are then forced to keep a cache of stat structures.
* dnotify's interface to user-space is awful. Signals?
inotify provides a more usable, simple, powerful solution to file change notification:
* inotify's interface is a device node, not SIGIO. You open a single fd to the device node, which is select()-able.
* inotify has an event that says "the filesystem that the item you were watching is on was unmounted."
* inotify can watch directories or files.
Inotify is currently used by Beagle (a desktop search infrastructure), Gamin (a FAM replacement), and other projects.
See Documentation/filesystems/inotify.txt.
Documentation/filesystems/inotify.txt | 123 ++++ fs/Kconfig | 13 fs/Makefile | 1 fs/attr.c | 33 - fs/compat.c | 12 fs/file_table.c | 3 fs/inode.c | 9 fs/inotify.c | 983 ++++++++++++++++++++++++++++++++++ fs/namei.c | 30 - fs/nfsd/vfs.c | 6 fs/open.c | 4 fs/read_write.c | 15 fs/sysfs/file.c | 7 fs/xattr.c | 5 include/linux/fs.h | 6 include/linux/fsnotify.h | 257 ++++++++ include/linux/inotify.h | 124 ++++ include/linux/sched.h | 4 kernel/user.c | 4 19 files changed, 1575 insertions(+), 64 deletions(-)
Documentation/filesystems/inotify.txt +123 -0
@@ -0,0 +1,123 @@ + inotify + a powerful yet simple file change notification system + + + +Document started 15 Mar 2005 by Robert Love <[email protected]> + +(i) User Interface + +Inotify is controlled by a device node, /dev/inotify. If you do not use udev, +this device may need to be created manually. First step, open it + + int dev_fd = open ("/dev/inotify", O_RDONLY); + +Change events are managed by "watches". A watch is an (object,mask) pair where +the object is a file or directory and the mask is a bitmask of one or more +inotify events that the application wishes to receive. See <linux/inotify.h> +for valid events. A watch is referenced by a watch descriptor, or wd. + +Watches are added via a file descriptor. + +Watches on a directory will return events on any files inside of the directory. + +Adding a watch is simple, + + /* 'wd' represents the watch on fd with mask */ + struct inotify_request req = { fd, mask }; + int wd = ioctl (dev_fd, INOTIFY_WATCH, &req); + +You can add a large number of files via something like + + for each file to watch { + struct inotify_request req; + int file_fd; + + file_fd = open (file, O_RDONLY); + if (fd < 0) { + perror ("open"); + break; + } + + req.fd = file_fd; + req.mask = mask; + + wd = ioctl (dev_fd, INOTIFY_WATCH, &req); + + close (fd); + } + +You can update an existing watch in the same manner, by passing in a new mask. + +An existing watch is removed via the INOTIFY_IGNORE ioctl, for example + + ioctl (dev_fd, INOTIFY_IGNORE, wd); + +Events are provided in the form of an inotify_event structure that is read(2) +from /dev/inotify. The filename is of dynamic length and follows the struct. +It is of size len. The filename is padded with null bytes to ensure proper +alignment. This padding is reflected in len. + +You can slurp multiple events by passing a large buffer, for example + + size_t len = read (fd, buf, BUF_LEN); + +Will return as many events as are available and fit in BUF_LEN. + +/dev/inotify is also select() and poll() able. + +You can find the size of the current event queue via the FIONREAD ioctl. + +All watches are destroyed and cleaned up on close. + + +(ii) Internal Kernel Implementation + +Each open inotify device is associated with an inotify_device structure. + +Each watch is associated with an inotify_watch structure. Watches are chained +off of each associated device and each associated inode. + +See fs/inotify.c for the locking and lifetime rules. + + +(iii) Rationale + +Q: What is the design decision behind not tying the watch to the +open fd of the watched object? + +A: Watches are associated with an open inotify device, not an +open file. This solves the primary problem with dnotify: +keeping the file open pins the file and thus, worse, pins the +mount. Dnotify is therefore infeasible for use on a desktop +system with removable media as the media cannot be unmounted. + +Q: What is the design decision behind using an-fd-per-device as +opposed to an fd-per-watch? + +A: An fd-per-watch quickly consumes more file descriptors than +are allowed, more fd's than are feasible to manage, and more +fd's than are ideally select()-able. Yes, root can bump the +per-process fd limit and yes, users can use epoll, but requiring +both is silly and an extraneous requirement. A watch consumes +less memory than an open file, separating the number spaces is +thus sensible. The current design is what user-space developers +want: Users open the device, once, and add n watches, requiring +but one fd and no twiddling with fd limits. +Opening /dev/inotify two thousand times is silly. If we can +implement user-space's preferences cleanly--and we can, the idr +layer makes stuff like this trivial--then we should. + +Q: Why a device node? + +A: The second biggest problem with dnotify is that the user +interface sucks ass. Signals are a terrible, terrible interface +for file notification. Or for anything, for that matter. The +idea solution, from all perspectives, is a file descriptor based +one that allows basic file I/O and poll/select. Obtaining the +fd and managing the watches could of been done either via a +device file or a family of new system calls. We decided to +implement a device file because adding three or four new system +calls that mirrored open, close, and ioctl seemed silly. A +character device makes sense from user-space and was easy to +implement inside of the kernel.
fs/attr.c +4 -29
@@ -10,7 +10,7 @@#include <linux/mm.h> #include <linux/string.h> #include <linux/smp_lock.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/fcntl.h> #include <linux/quotaops.h> #include <linux/security.h>
@@ -107,31 +107,8 @@ out:return error; } -EXPORT_SYMBOL (inode_setattr); -int setattr_mask (unsigned int ia_valid) -{ -unsigned long dn_mask =0 ; - -if (ia_valid &ATTR_UID ) - dn_mask |=DN_ATTRIB ; -if (ia_valid &ATTR_GID ) - dn_mask |=DN_ATTRIB ; -if (ia_valid &ATTR_SIZE ) - dn_mask |=DN_MODIFY ; -/* both times implies a utime(s) call */ -if ((ia_valid & (ATTR_ATIME |ATTR_MTIME )) == (ATTR_ATIME |ATTR_MTIME )) - dn_mask |=DN_ATTRIB ; -else if (ia_valid &ATTR_ATIME ) - dn_mask |=DN_ACCESS ; -else if (ia_valid &ATTR_MTIME ) - dn_mask |=DN_MODIFY ; -if (ia_valid &ATTR_MODE ) - dn_mask |=DN_ATTRIB ; -return dn_mask; -} -int notify_change (struct dentry * dentry,struct iattr * attr) {struct inode *inode = dentry->d_inode;
@@ -197,11 +174,9 @@if (ia_valid &ATTR_SIZE )up_write (&dentry->d_inode->i_alloc_sem); -if (!error) { -unsigned long dn_mask =setattr_mask (ia_valid); -if (dn_mask) -dnotify_parent (dentry, dn_mask); - } +if (!error) +fsnotify_change (dentry, ia_valid); +return error; }
fs/compat.c +8 -4
@@ -37,7 +37,7 @@#include <linux/ctype.h> #include <linux/module.h> #include <linux/dirent.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/highuid.h> #include <linux/sunrpc/svc.h> #include <linux/nfsd/nfsd.h>
@@ -1307,9 +1307,13 @@ out:if (iov != iovstack)kfree (iov); -if ((ret + (type ==READ )) >0 ) -dnotify_parent (file->f_dentry, - (type ==READ ) ?DN_ACCESS :DN_MODIFY ); +if ((ret + (type ==READ )) >0 ) { +struct dentry *dentry = file->f_dentry; +if (type ==READ ) +fsnotify_access (dentry); +else +fsnotify_modify (dentry); + }return ret; }
fs/file_table.c +3 -0
@@ -16,6 +16,7 @@#include <linux/eventpoll.h> #include <linux/mount.h> #include <linux/cdev.h> +#include <linux/fsnotify.h> /* sysctl tunables... */ struct files_stat_struct files_stat = {
@@ -123,6 +124,8 @@struct inode *inode = dentry->d_inode;might_sleep (); + +fsnotify_close (file);/* * The function eventpoll_release() should be the first called * in the file cleanup chain.
fs/inode.c +9 -0
@@ -21,6 +21,7 @@#include <linux/pagemap.h> #include <linux/cdev.h> #include <linux/bootmem.h> +#include <linux/inotify.h> /* * This is needed for the following functions:
@@ -128,6 +129,9 @@#ifdef CONFIG_QUOTA memset (&inode->i_dquot,0 ,sizeof (inode->i_dquot));#endif +#ifdef CONFIG_INOTIFY +atomic_set (&inode->inotify_watch_count,0 ); +#endif inode->i_pipe =NULL ; inode->i_bdev =NULL ; inode->i_cdev =NULL ;
@@ -202,6 +206,10 @@INIT_LIST_HEAD (&inode->i_data.i_mmap_nonlinear);spin_lock_init (&inode->i_lock);i_size_ordered_init (inode); +#ifdef CONFIG_INOTIFY +INIT_LIST_HEAD (&inode->inotify_watches); +sema_init (&inode->inotify_sem,1 ); +#endif }EXPORT_SYMBOL (inode_init_once);
@@ -346,6 +354,7 @@down (&iprune_sem);spin_lock (&inode_lock); +inotify_unmount_inodes (&sb->s_inodes); busy =invalidate_list (&sb->s_inodes, &throw_away);spin_unlock (&inode_lock);
fs/inotify.c +983 -0
983 lines not on this page · raw message
fs/Kconfig +13 -0
@@ -339,6 +339,19 @@ If you don't know whether you need it, then you don't need it: answer N. +config INOTIFY + bool "Inotify file change notification support" + default y + ---help--- + Say Y here to enable inotify support and the /dev/inotify character + device. Inotify is a file change notification system and a + replacement for dnotify. Inotify fixes numerous shortcomings in + dnotify and introduces several new features. It allows monitoring + of both files and directories via a single open fd. Multiple file + events are supported. + + If unsure, say Y. + config QUOTA bool "Quota support" help
fs/Makefile +1 -0
@@ -11,6 +11,7 @@ attr.o bad_inode.o file.o filesystems.o namespace.o aio.o \ seq_file.o xattr.o libfs.o fs-writeback.o mpage.o direct-io.o \ +obj-$(CONFIG_INOTIFY) += inotify.o obj-$(CONFIG_EPOLL) += eventpoll.o obj-$(CONFIG_COMPAT) += compat.o
fs/namei.c +16 -14
@@ -21,7 +21,7 @@#include <linux/namei.h> #include <linux/quotaops.h> #include <linux/pagemap.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/smp_lock.h> #include <linux/personality.h> #include <linux/security.h>
@@ -1296,7 +1296,7 @@DQUOT_INIT (dir); error = dir->i_op->create (dir, dentry, mode, nd);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, dentry->d_name.name);security_inode_post_create (dir, dentry, mode); }return error;
@@ -1602,7 +1602,7 @@DQUOT_INIT (dir); error = dir->i_op->mknod (dir, dentry, mode, dev);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, dentry->d_name.name);security_inode_post_mknod (dir, dentry, mode, dev); }return error;
@@ -1675,7 +1675,7 @@DQUOT_INIT (dir); error = dir->i_op->mkdir (dir, dentry, mode);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_mkdir (dir, dentry->d_name.name);security_inode_post_mkdir (dir,dentry, mode); }return error;
@@ -1766,7 +1766,7 @@ }up (&dentry->d_inode->i_sem);if (!error) { -inode_dir_notify (dir,DN_DELETE ); +fsnotify_rmdir (dentry, dentry->d_inode, dir);d_delete (dentry); }dput (dentry);
@@ -1839,9 +1839,10 @@/* We don't d_delete() NFS sillyrenamed files--they still exist. */ if (!error && !(dentry->d_flags &DCACHE_NFSFS_RENAMED )) { +fsnotify_unlink (dentry, dir);d_delete (dentry); -inode_dir_notify (dir,DN_DELETE ); } +return error; }
@@ -1915,7 +1916,7 @@DQUOT_INIT (dir); error = dir->i_op->symlink (dir, dentry, oldname);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, dentry->d_name.name);security_inode_post_symlink (dir, dentry, oldname); }return error;
@@ -1988,7 +1989,7 @@ error = dir->i_op->link (old_dentry, dir, new_dentry);up (&old_dentry->d_inode->i_sem);if (!error) { -inode_dir_notify (dir,DN_CREATE ); +fsnotify_create (dir, new_dentry->d_name.name);security_inode_post_link (old_dentry, dir, new_dentry); }return error;
@@ -2152,6 +2153,7 @@ {int error;int is_dir =S_ISDIR (old_dentry->d_inode->i_mode); +const char *old_name;if (old_dentry->d_inode == new_dentry->d_inode)return 0 ;
@@ -2173,18 +2175,18 @@DQUOT_INIT (old_dir);DQUOT_INIT (new_dir); + old_name =fsnotify_oldname_init (old_dentry->d_name.name); +if (is_dir) error =vfs_rename_dir (old_dir,old_dentry,new_dir,new_dentry);else error =vfs_rename_other (old_dir,old_dentry,new_dir,new_dentry);if (!error) { -if (old_dir == new_dir) -inode_dir_notify (old_dir,DN_RENAME ); -else { -inode_dir_notify (old_dir,DN_DELETE ); -inode_dir_notify (new_dir,DN_CREATE ); - } +const char *new_name = old_dentry->d_name.name; +fsnotify_move (old_dir, new_dir, old_name, new_name, is_dir); } +fsnotify_oldname_free (old_name); +return error; }
fs/nfsd/vfs.c +3 -3
@@ -45,7 +45,7 @@#endif /* CONFIG_NFSD_V3 */ #include <linux/nfsd/nfsfh.h> #include <linux/quotaops.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #ifdef CONFIG_NFSD_V4 #include <linux/posix_acl.h> #include <linux/posix_acl_xattr.h>
@@ -861,7 +861,7 @@ nfsdstats.io_read += err; *count = err; err =0 ; -dnotify_parent (file->f_dentry,DN_ACCESS ); +fsnotify_access (file->f_dentry); }else err =nfserrno (err); out:
@@ -917,7 +917,7 @@set_fs (oldfs);if (err >=0 ) { nfsdstats.io_write += cnt; -dnotify_parent (file->f_dentry,DN_MODIFY ); +fsnotify_modify (file->f_dentry); }/* clear setuid/setgid flag after write */
fs/open.c +3 -1
@@ -10,7 +10,7 @@#include <linux/file.h> #include <linux/smp_lock.h> #include <linux/quotaops.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/module.h> #include <linux/slab.h> #include <linux/tty.h>
@@ -944,9 +944,11 @@ fd =get_unused_fd ();if (fd >=0 ) {struct file *f =filp_open (tmp, flags, mode); + error =PTR_ERR (f);if (IS_ERR (f)) goto out_error; +fsnotify_open (f->f_dentry);fd_install (fd, f); } out:
fs/read_write.c +9 -6
@@ -10,7 +10,7 @@#include <linux/file.h> #include <linux/uio.h> #include <linux/smp_lock.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/security.h> #include <linux/module.h> #include <linux/syscalls.h>
@@ -239,7 +239,7 @@else ret =do_sync_read (file, buf, count, pos);if (ret >0 ) { -dnotify_parent (file->f_dentry,DN_ACCESS ); +fsnotify_access (file->f_dentry); current->rchar += ret; } current->syscr++;
@@ -287,7 +287,7 @@else ret =do_sync_write (file, buf, count, pos);if (ret >0 ) { -dnotify_parent (file->f_dentry,DN_MODIFY ); +fsnotify_modify (file->f_dentry); current->wchar += ret; } current->syscw++;
@@ -523,9 +523,12 @@ out:if (iov != iovstack)kfree (iov); -if ((ret + (type ==READ )) >0 ) -dnotify_parent (file->f_dentry, - (type ==READ ) ?DN_ACCESS :DN_MODIFY ); +if ((ret + (type ==READ )) >0 ) { +if (type ==READ ) +fsnotify_access (file->f_dentry); +else +fsnotify_modify (file->f_dentry); + }return ret; Efault: ret = -EFAULT ;
fs/sysfs/file.c +2 -5
@@ -3,7 +3,7 @@*/ #include <linux/module.h> -#include <linux/dnotify.h> +#include <linux/fsnotify.h> #include <linux/kobject.h> #include <asm/uaccess.h> #include <asm/semaphore.h>
@@ -389,9 +389,6 @@* sysfs_update_file - update the modified timestamp on an object attribute. * @kobj: object we're acting for. * @attr: attribute descriptor. -* -* Also call dnotify for the dentry, which lots of userspace programs -* use. */ int sysfs_update_file (struct kobject * kobj,const struct attribute * attr) {
@@ -406,7 +403,7 @@if (victim->d_inode && (victim->d_parent->d_inode == dir->d_inode)) { victim->d_inode->i_mtime =CURRENT_TIME ; -dnotify_parent (victim,DN_MODIFY ); +fsnotify_modify (victim);/** * Drop reference from initial sysfs_get_dentry().
fs/xattr.c +4 -1
@@ -16,6 +16,7 @@#include <linux/security.h> #include <linux/syscalls.h> #include <linux/module.h> +#include <linux/fsnotify.h> #include <asm/uaccess.h> /*
@@ -57,8 +58,10 @@if (error) goto out; error = d->d_inode->i_op->setxattr (d, kname, kvalue, size, flags); -if (!error) +if (!error) { +fsnotify_xattr (d);security_inode_post_setxattr (d, kname, kvalue, size, flags); + } out:up (&d->d_inode->i_sem); }
include/linux/fs.h +5 -1
@@ -471,6 +471,11 @@struct dnotify_struct *i_dnotify;/* for directory notifications */ #endif +#ifdef CONFIG_INOTIFY +struct list_head inotify_watches;/* watches on this inode */ +struct semaphore inotify_sem;/* protects the watches list */ +#endif +unsigned long i_state;unsigned long dirtied_when;/* jiffies of first dirtying */
@@ -1369,7 +1374,6 @@extern int do_remount_sb (struct super_block *sb,int flags,void *data,int force);extern sector_t bmap (struct inode *,sector_t ); -extern int setattr_mask (unsigned int );extern int notify_change (struct dentry *,struct iattr *);extern int permission (struct inode *,int ,struct nameidata *); extern int generic_permission(struct inode *, int,
include/linux/fsnotify.h +257 -0
257 lines not on this page · raw message
include/linux/inotify.h +124 -0
124 lines not on this page · raw message
include/linux/sched.h +4 -0
@@ -404,6 +404,10 @@atomic_t processes;/* How many processes does this user have? */ atomic_t files;/* How many open files does this user have? */ atomic_t sigpending;/* How many pending signals does this user have? */ +#ifdef CONFIG_INOTIFY +atomic_t inotify_watches;/* How many inotify watches does this user have? */ +atomic_t inotify_devs;/* How many inotify devs does this user have opened? */ +#endif /* protected by mq_lock */ unsigned long mq_bytes;/* How many bytes can be allocated to mqueue? */ unsigned long locked_shm;/* How many pages of mlocked shm ? */
kernel/user.c +4 -0
@@ -120,6 +120,10 @@atomic_set (&new->processes,0 );atomic_set (&new->files,0 );atomic_set (&new->sigpending,0 ); +#ifdef CONFIG_INOTIFY +atomic_set (&new->inotify_watches,0 ); +atomic_set (&new->inotify_devs,0 ); +#endif new->mq_bytes =0 ; new->locked_shm =0 ;
Re: [patch] inotify, improved.
Speaking of inotify improvements...
On a newsgroup someone was using inotify, but was asking if there was any way to also determine which process/user had caused the notification.
Is this something that would make sense (as an optional bit of information) in inotify?
Chris
Re: [patch] inotify, improved.
On Fri, 2005-06-17 at 09:37 -0600, Chris Friesen wrote:
Hi, Chris.
Quoted 5 lines
On a newsgroup someone was using inotify, but was asking if there was any way to also determine which process/user had caused the notification.
Is this something that would make sense (as an optional bit of information) in inotify?
It is definitely something that could be added, technically speaking.
I have been hesitant, though. I do not want feature creep to be a deterrent to acceptance into the Linux kernel. I also think that there could be arguments about security. Sending the event is one thing, telling which pid (and thus what user, etc.) caused the event is another. For example, we can make the argument that read rights on a file are tantamount to the right to receive a read event. But can we say that read rights are enough for a unprivileged user to know that root at pid 820 is writing the file? I don't know.
I'd add it if there were consensus. I don't know that it makes sense, though.
Robert Love
Re: [patch] inotify, improved.
Quoted 8 lines · Robert Love
I have been hesitant, though. I do not want feature creep to be a deterrent to acceptance into the Linux kernel. I also think that there could be arguments about security. Sending the event is one thing, telling which pid (and thus what user, etc.) caused the event is another. For example, we can make the argument that read rights on a file are tantamount to the right to receive a read event. But can we say that read rights are enough for a unprivileged user to know that root at pid 820 is writing the file? I don't know.
It's also racy as hell. By the time the inotify gets delivered to the userspace process, pid 820 may be long gone.....
Re: [patch] inotify, improved.
Quoted 2 lines · On Fri, 2005-06-17 at 12:11 -0400, [email protected] wrote:
It's also racy as hell. By the time the inotify gets delivered to the userspace process, pid 820 may be long gone.....
Yah, no one would expect otherwise. One nice feature of it would be able to see if the pid is equal to a known pid, say yourself or some worker process.
But I've also received requests to send the size and offset of a read/write, the user, etc. etc. I just say no.
Robert Love
Re: [patch] inotify, improved.
In reply to [email protected]Quoted 2 lines · [email protected] wrote:
It's also racy as hell. By the time the inotify gets delivered to the userspace process, pid 820 may be long gone.....
Yep. But I can see uses for people to want to log all activity on specific directory trees. Think audit trails, etc.
Imagine root getting a log like:
Date: Jan 1,2006: file /foo/evidence.txt changed by user blah, pid <666>, commandline: "vi evidence.txt"
Chris
Re: [patch] inotify, improved.
Quoted 7 lines · * Chris Friesen ([email protected]) wrote:
Quoted 2 lines · [email protected] wrote:
It's also racy as hell. By the time the inotify gets delivered to the userspace process, pid 820 may be long gone.....
Yep. But I can see uses for people to want to log all activity on specific directory trees. Think audit trails, etc.
This is already done by the audit patch. I don't think inotify has quite enough info to build a comprehensive audit record.
Re: [patch] inotify, improved.
In reply to Chris FriesenQuoted 7 lines · Chris Friesen
Quoted 2 lines · [email protected] wrote:
It's also racy as hell. By the time the inotify gets delivered to the userspace process, pid 820 may be long gone.....
Yep. But I can see uses for people to want to log all activity on specific directory trees. Think audit trails, etc.
If it's an audit trail, it should be provided by the audit subsystem, not just inotify.
Cheers,
Muli
Signature · 2 lines
Muli Ben-Yehuda http://www.mulix.org | http://mulix.livejournal.com/
Re: [patch] inotify, improved.
In reply to Robert LoveQuoted 6 lines · Robert Love
Quoted 1 line · Chris Friesen
On a newsgroup someone was using inotify, but was asking if there was any way to also determine which process/user had caused the notification.
I have been hesitant, though. I do not want feature creep to be a deterrent to acceptance into the Linux kernel.
Absolutely.
Quoted 4 lines
I also think that there could be arguments about security. ...can we
say that read rights are enough for a unprivileged user to know that root at pid 820 is writing the file? I don't know.
I'm sure some reasonable rules could be determined. Maybe you'd need to be the owner of the file to get the extra info, with root able to monitor everything.
Maybe there should be a way to load plugins into inotify (something like netfilter) so that people load modules to send themselves whatever information they want...
Let's get it in first though before thinking too hard about extra stuff.
Chris
Re: [patch] inotify, improved.
Quoted 16 lines · Chris Friesen
Quoted 7 lines · Robert Love
Quoted 2 lines · Chris Friesen
On a newsgroup someone was using inotify, but was asking if there was any way to also determine which process/user had caused the notification.
I have been hesitant, though. I do not want feature creep to be a deterrent to acceptance into the Linux kernel.
Absolutely.
Quoted 4 lines
I also think that there could be arguments about security. ...can we
say that read rights are enough for a unprivileged user to know that root at pid 820 is writing the file? I don't know.
Quoted 7 lines
I'm sure some reasonable rules could be determined. Maybe you'd need to be the owner of the file to get the extra info, with root able to monitor everything.
Maybe there should be a way to load plugins into inotify (something like netfilter) so that people load modules to send themselves whatever information they want...
This is probably a good idea for the _audit_ system. Inotify was designed to do 1 task well, and it should stay that way.
John
Re: [patch] inotify, improved.
In reply to Robert LoveReview
Quoted 2 lines
(quoted diff) +1
-0+ schedule();
Here's a stab at getting rid of that raw schedule() in inotify_read(). It maintains the behaviour where it returns when an event doesn't fit and returns after events have been copied instead of sleeping. It changes behaviour in that it returns partial reads that suceeded instead of the error that stopped processing. It also lets threads who race out of a wakeup to find an empty list go back to sleep instead of returning
0. Dunno if that's behaviour you'd prefer but it seemed reasonable. I hope that lockless list_empty() is OK, I didn't think very hard about it.
Compiles but totally untested. Check my work :)
- z
fs/inotify.c +24 -41
@@ -639,52 +639,32 @@static ssize_t inotify_read (struct file *file,char __user *buf,size_t count,loff_t *pos) { -size_t event_size =sizeof (struct inotify_event ); -struct inotify_device *dev; -char __user *start; -int ret; -DEFINE_WAIT (wait); - - start = buf; - dev = file->private_data; - -while (1 ) { -int events; - -prepare_to_wait (&dev->wq, &wait,TASK_INTERRUPTIBLE ); - -down (&dev->sem); - events = !list_empty (&dev->events); -up (&dev->sem); -if (events) { - ret =0 ; -break ; - } - -if (file->f_flags &O_NONBLOCK ) { - ret = -EAGAIN ; -break ; - } - -if (signal_pending (current)) { - ret = -EINTR ; -break ; - } - -schedule (); - } - -finish_wait (&dev->wq, &wait); -if (ret) -return ret; +struct inotify_device *dev = file->private_data; +char __user *start = buf; +int ret =0 ;down (&dev->sem);while (1 ) {struct inotify_kernel_event *kevent; +static size_t event_size =sizeof (struct inotify_event ); - ret = buf - start; -if (list_empty (&dev->events)) -break ; +if (list_empty (&dev->events)) { +/* return partial instead of sleeping */ +if (buf > start) +break ; +if (file->f_flags &O_NONBLOCK ) { + ret = -EAGAIN ; +break ; + } +up (&dev->sem); + ret =wait_event_interruptible (dev->wq, + !list_empty (&dev->events)); +down (&dev->sem); +if (ret) +break ; +continue ; + + } kevent =inotify_dev_get_event (dev);if (event_size + kevent->event.len > count)
@@ -710,6 +690,9 @@ }up (&dev->sem); +if (buf > start) + ret = buf - start; +return ret; }
Re: [patch] inotify, improved.
Quoted 12 lines · Zach Brown
Review
Quoted 2 lines
(quoted diff) +1
-0+ schedule();Here's a stab at getting rid of that raw
schedule()ininotify_read(). It maintains the behaviour where it returns when an event doesn't fit and returns after events have been copied instead of sleeping. It changes behaviour in that it returns partial reads that suceeded instead of the error that stopped processing. It also lets threads who race out of a wakeup to find an empty list go back to sleep instead of returning
0. Dunno if that's behaviour you'd prefer but it seemed reasonable. I hope that locklesslist_empty()is OK, I didn't think very hard about it.
I really don't like sending partial events. I don't think it's worth the extra effort in tracking how much of an event we sent out last time. I also don't see any added benefit to user space when providing partial events. It's going to complicate the user space event parsing code to.
Quoted 1 line
Compiles but totally untested. Check my work :)
At first glance, I don't see any code to restart the partially sent event. Am I missing the obvious or what? Without that, the user would get the first half of an event, then get both halfs on the next read. That simply won't work.
John
Re: [patch] inotify, improved.
Quoted 1 line · John McCutchan
I really don't like sending partial events.
Partial reads, not partial events. Just like the previous code, it returns to userspace after copying the events it found in the list before it went empty. It doesn't go back to sleep to fill the rest of the buffer like a more classical blocking read() method would.
Where it differs is in what happens if you get errors copying events after having successfully copied some. The previous code would return the error, that patch would return the bytes used by the good events.
- z
Re: [patch] inotify, improved.
Quoted 4 lines · Zach Brown
Quoted 1 line · John McCutchan
I really don't like sending partial events.
Quoted 8 lines
Partial reads, not partial events. Just like the previous code, it returns to userspace after copying the events it found in the list before it went empty. It doesn't go back to sleep to fill the rest of the buffer like a more classical blocking
read()method would.Where it differs is in what happens if you get errors copying events after having successfully copied some. The previous code would return the error, that patch would return the bytes used by the good events.
My bad. Shouldn't we return the error code to user space though? We shouldn't be hiding errors in the app. How does read() handle an error part way through a read?
John
Re: [patch] inotify, improved.
Quoted 4 lines
My bad. Shouldn't we return the error code to user space though? We shouldn't be hiding errors in the app. How does
read()handle an error part way through a read?
My preference is to give userspace a chance to work with what it can, though I'm not sure what the N read paths do right now.
- z
Documentation/filesystems/inotify.txt line 123
Quoted 13 lines · Robert Love
Documentation/filesystems/inotify.txt +13
-0+Q: Why a device node? + +A: The second biggest problem with dnotify is that the user +interface sucks ass. Signals are a terrible, terrible interface +for file notification. Or for anything, for that matter. The +idea solution, from all perspectives, is a file descriptor based +one that allows basic file I/O and poll/select. Obtaining the +fd and managing the watches could of been done either via a +device file or a family of new system calls. We decided to +implement a device file because adding three or four new system +calls that mirrored open, close, and ioctl seemed silly. A +character device makes sense from user-space and was easy to +implement inside of the kernel.
Sorry to bring up a topic that should have been settled a long time ago.
I found that the interface consisting of
- open a handle
- add a file descriptor with an event mask to handle
- remove a file/watch descriptor from handle
- wait on handle, get events
- close handle
in inotify is _very_ similar to how epoll is represented to user space. Is there a good reason that epoll is a set of syscalls while inotify is a character device, or is one of them simply wrong?
Arnd <><
Quoted 17 lines · Arnd Bergmann
Documentation/filesystems/inotify.txt line 123
Quoted 13 lines · Robert Love
Documentation/filesystems/inotify.txt +13
-0+Q: Why a device node? + +A: The second biggest problem with dnotify is that the user +interface sucks ass. ?Signals are a terrible, terrible interface +for file notification. ?Or for anything, for that matter. ?The +idea solution, from all perspectives, is a file descriptor based +one that allows basic file I/O and poll/select. ?Obtaining the +fd and managing the watches could of been done either via a +device file or a family of new system calls. ?We decided to +implement a device file because adding three or four new system +calls that mirrored open, close, and ioctl seemed silly. ?A +character device makes sense from user-space and was easy to +implement inside of the kernel.Sorry to bring up a topic that should have been settled a long time ago.
This was settled a long time ago. Robert, Andrew, and I had an off-list discussion months ago, and we all agreed that this was the right interface for inotify.
John
Quoted 4 lines · John McCutchan
This was settled a long time ago. Robert, Andrew, and I had an off-list discussion months ago, and we all agreed that this was the right interface for inotify.
I don't think I ever really affirmatively agreed to anything. I do recall various things being discussed at various times and various things being changed, but from where I sit it's all spread out and foggy.
I certainly remember that good-sounding recommendations which addressed the things which Christoph doesn't like were convincingly shot down by yourself and by Robert, but I don't recall why.
Look, this stuff is hard. This is why I've asked you and Robert again and again and again to generate some sort of design doc or FAQ which addresses each of these frequently-asked-questions. So the poor rest of us can look through it and say "oh yeah". Because inotify _is_ a tricky thing, and standard kernel interface designs _don't_ fit it well.
So. It's not too late. Please spend an hour and write up the Inofity Implementation FAQ? You probably remember and fully understand what all of our objections are and I know that you have explanations and rebuttals at hand.
Please? Something like:
q: Why does it use an ioctl multiplexer
a: Because ...
etc...
I haven't done a detailed review of the patch in months and I intend to do another soon. That FAQ will help! When I ask more silly questions we can update it, so those questions will never again be asked.
I know it's unusual process-wise, but inotify is an unusual feature.
Quoted 4 lines · Andrew Morton
So. It's not too late. Please spend an hour and write up the Inofity Implementation FAQ? You probably remember and fully understand what all of our objections are and I know that you have explanations and rebuttals at hand.
I wrote this up the first time you asked:
Documentation/filesystems/inotify.txt
It sounds like its never been addressed because some people don't take different views as an acceptable solution.
Robert Love
Quoted 3 lines · Robert Love
I wrote this up the first time you asked:
Documentation/filesystems/inotify.txt
If there is something I should add, just holler. I will write an encyclopedia if need be.
Robert Love
Quoted 8 lines · On Friday June 17, [email protected] wrote:
Quoted 3 lines · Robert Love
I wrote this up the first time you asked:
Documentation/filesystems/inotify.txtIf there is something I should add, just holler. I will write an encyclopedia if need be.
I haven't yet decided if I like it or not, but maybe I have some useful comments to make...
Quoted 7 lines
Documentation/filesystems/inotify.txt +7
-0+ +Q: What is the design decision behind using an-fd-per-device as +opposed to an fd-per-watch? + +A: An fd-per-watch quickly consumes more file descriptors than +are allowed, more fd's than are feasible to manage, and more +fd's than are ideally select()-able. Yes, root can bump the
Documentation/filesystems/inotify.txt line 110
Quoted 10 lines
Documentation/filesystems/inotify.txt +10
-0+per-process fd limit and yes, users can use epoll, but requiring +both is silly and an extraneous requirement. A watch consumes +less memory than an open file, separating the number spaces is +thus sensible. The current design is what user-space developers +want: Users open the device, once, and add n watches, requiring +but one fd and no twiddling with fd limits. +Opening /dev/inotify two thousand times is silly. If we can +implement user-space's preferences cleanly--and we can, the idr +layer makes stuff like this trivial--then we should. +
To address the particular points:
- "An fd-per-watch quickly consumes more file descriptors than are allowed"
I note that the current limit if 'wd's is 8192 per user, compared with the default 'fd' limit of 1024 per process. So if a user has more than 8 processes making very heavy use of inotify, they would be better off with the file-descriptor limit...
So I don't find this argument convincing.
I would suggest that it be removed, and you put in a separate section discussion resource usage and limits. You could explain why you don't use rlimit (probably not a hard case to win) and why the sysadmin cannot give different limits to different users (as a sys-admin, I would fight that), and why no one user would ever want more than about 8 processes using inotify :-)
- "more fd's than are feasible to manage,"
It's not clear what this means. The program will still need to manage lots of 'wd's. How is this different from handling 'fd's? The only 'manage'ment issue I could come up with is the hassle of shepherding all these 'wd's through a 'fork', only to close them before an 'exec'. Is that what you mean? If so it would help to make it more explicit.
Using the fd from /dev/inotify like a 'bag' of 'wd's which can be closed as a whole or passed around (through fork or exec or even over a unix domain socket) is, I think, I strong point. You could possibly leverage that in selling the current inotify interface.
- "and more fd's than are ideally select()-able" and "users can use epoll, but requiring [it] is silly and an extraneous requirement"
Having to use epoll *should* not be a problem.
"Surely" thought I "you an just select on the fd returned by epoll_create, and then adding inotify to your select() loop should be just as easy with epoll as with /dev/inotify". But then I discovered that you cannot select() on the epoll fd :-( Maybe this should be fixed rather than be used as an excuse.
- "A watch consumes less memory than an open filep"
This is largely valid. Two other possible responses:
1/ Make the filep smaller. Not every 'struct file' needs read-ahead data (which is BIG). Not every file needs the fown_struct. I suspect that a small and generic struct file could be created which was embedded inside a larger less generic struct file when needed.. If the size if filep is really an issue.
2/ An fd doesn't *have* to correspond to a 'struct file'. Suppose we allowed the 'watch_fd' systemcall to return an fd which actually mapped to a struct inotify_watch. Possibly using a few high bits to differentiate different types of 'fds'.
Most system calls would reject these fds immediately. Those we wanted to handle them could easily be fixed.
So sys_close would know how to close them. sys_fcntl would need to be able to set close-on-exec. Maybe a few others like poll. But from user-space, it would be 'just another fd'.
You wouldn't be able to use these 'fds' in select(), but I think we can all agree that you probably wouldn't want to.
It is also worth noting that there is a more telling reason to not use "struct file" than the size. A 'struct file' points to a dentry. A 'struct inotify_watch' points to an inode. This difference is fairly crucial to how inotify works.
I hope some of these reflections are useful.
NeilBrown
Quoted 32 lines · Neil Brown
I haven't yet decided if I like it or not, but maybe I have some useful comments to make...
Quoted 7 lines
(quoted diff) +7
-0+ +Q: What is the design decision behind using an-fd-per-device as +opposed to an fd-per-watch? + +A: An fd-per-watch quickly consumes more file descriptors than +are allowed, more fd's than are feasible to manage, and more +fd's than are ideally select()-able. Yes, root can bump theReview
Quoted 10 lines
(quoted diff) +10
-0+per-process fd limit and yes, users can use epoll, but requiring +both is silly and an extraneous requirement. A watch consumes +less memory than an open file, separating the number spaces is +thus sensible. The current design is what user-space developers +want: Users open the device, once, and add n watches, requiring +but one fd and no twiddling with fd limits. +Opening /dev/inotify two thousand times is silly. If we can +implement user-space's preferences cleanly--and we can, the idr +layer makes stuff like this trivial--then we should. +To address the particular points:
- "An fd-per-watch quickly consumes more file descriptors than are allowed"I note that the current limit if 'wd's is 8192 per user, compared with the default 'fd' limit of 1024 per process. So if a user has more than 8 processes making very heavy use of inotify, they would be better off with the file-descriptor limit...
So I don't find this argument convincing.
Quoted 7 lines
I would suggest that it be removed, and you put in a separate section discussion resource usage and limits. You could explain why you don't use rlimit (probably not a hard case to win) and why the sysadmin cannot give different limits to different users (as a sys-admin, I would fight that), and why no one user would ever want more than about 8 processes using inotify :-)
Looking at beagle, it is a single process that can easily watch more than 1024 directories. Beagle would have to work around the 1024 limit by managing processes, each of which watch a fraction of the total watches. PITA. Also, 8192 was an arbitrary choice that seemed big enough for most users needs and all of these limits can be set in sysfs. Checkout /sys/misc/inotify.
Quoted 9 lines
- "more fd's than are feasible to manage,"
It's not clear what this means. The program will still need to manage lots of 'wd's. How is this different from handling 'fd's? The only 'manage'ment issue I could come up with is the hassle of shepherding all these 'wd's through a 'fork', only to close them before an 'exec'. Is that what you mean? If so it would help to make it more explicit.
The program doesn't really have to manage the wd's. A program simply needs a map from wd to it's own structure.
Assume for a moment that we had chosen a fd-as-watch approach. Now take beagle, which has tons of watches. Every time beagle goes through it's loop, it has to add each of those watch-fd's to the select table, then check for each one afterwards. It's pretty easy to see how this is unmanageable and a waste of CPU time.
Quoted 5 lines
Using the fd from
/dev/inotifylike a 'bag' of 'wd's which can be closed as a whole or passed around (through fork or exec or even over a unix domain socket) is, I think, I strong point. You could possibly leverage that in selling the current inotify interface.
This is a great way of thinking about it.
Quoted 12 lines
- "A watch consumes less memory than an open filep"
This is largely valid. Two other possible responses:
1/ Make the filep smaller. Not every 'struct file' needs read-ahead data (which is BIG). Not every file needs the
fown_struct. I suspect that a small and generic struct file could be created which was embedded inside a larger less generic struct file when needed.. If the size if filep is really an issue.
Maybe a good idea, but this is overkill for inotify.
Quoted 11 lines
2/ An fd doesn't *have* to correspond to a 'struct file'. Suppose we allowed the '
watch_fd' systemcall to return an fd which actually mapped to a structinotify_watch. Possibly using a few high bits to differentiate different types of 'fds'.
Most system calls would reject these fds immediately. Those we wanted to handle them could easily be fixed.
Sosys_closewould know how to close them.sys_fcntlwould need to be able to set close-on-exec. Maybe a few others like poll. But from user-space, it would be 'just another fd'.
Why bother though? The idr layer lets us have a integer that maps to a data structure in kernel space, pretty much for free.
Signature · 1 line
John McCutchan <[email protected]>
Quoted 41 lines · On Monday June 20, [email protected] wrote:
Quoted 32 lines · Neil Brown
I haven't yet decided if I like it or not, but maybe I have some useful comments to make...
Quoted 7 lines
(quoted diff) +7
-0+ +Q: What is the design decision behind using an-fd-per-device as +opposed to an fd-per-watch? + +A: An fd-per-watch quickly consumes more file descriptors than +are allowed, more fd's than are feasible to manage, and more +fd's than are ideally select()-able. Yes, root can bump theReview
Quoted 10 lines
(quoted diff) +10
-0+per-process fd limit and yes, users can use epoll, but requiring +both is silly and an extraneous requirement. A watch consumes +less memory than an open file, separating the number spaces is +thus sensible. The current design is what user-space developers +want: Users open the device, once, and add n watches, requiring +but one fd and no twiddling with fd limits. +Opening /dev/inotify two thousand times is silly. If we can +implement user-space's preferences cleanly--and we can, the idr +layer makes stuff like this trivial--then we should. +To address the particular points:
- "An fd-per-watch quickly consumes more file descriptors than are allowed"I note that the current limit if 'wd's is 8192 per user, compared with the default 'fd' limit of 1024 per process. So if a user has more than 8 processes making very heavy use of inotify, they would be better off with the file-descriptor limit...
So I don't find this argument convincing.Quoted 7 lines
I would suggest that it be removed, and you put in a separate section discussion resource usage and limits. You could explain why you don't use rlimit (probably not a hard case to win) and why the sysadmin cannot give different limits to different users (as a sys-admin, I would fight that), and why no one user would ever want more than about 8 processes using inotify :-)
Quoted 6 lines
Looking at beagle, it is a single process that can easily watch more than 1024 directories. Beagle would have to work around the 1024 limit by managing processes, each of which watch a fraction of the total watches. PITA. Also, 8192 was an arbitrary choice that seemed big enough for most users needs and all of these limits can be set in sysfs. Checkout
/sys/misc/inotify.
I think you missed my point...
The FAQ entry which I found unconvincing said
"Yes, root can bump the per-process fd limit ... but requiring [this]
is silly and an extraneous requirement."
So when you say "... all of these limits can be set in sysfs", you are effectively contradicting the FAQ entry.
Yes, 1024 is an arbitrary limit. But so is 8192.
The argument "We cannot use fd's because the arbitrary limit is lower than the arbitrary limit that I want to use" simply doesn't hold water.
There may well be other good arguments against 'fd's, but I'm trying to point out that this isn't one of them, and so shouldn't appear in this part of the FAQ.
Quoted 11 lines
Quoted 10 lines
- "more fd's than are feasible to manage,"
It's not clear what this means. The program will still need to manage lots of 'wd's. How is this different from handling 'fd's? The only 'manage'ment issue I could come up with is the hassle of shepherding all these 'wd's through a 'fork', only to close them before an 'exec'. Is that what you mean? If so it would help to make it more explicit.
Quoted 2 lines
The program doesn't really have to manage the wd's. A program simply needs a map from wd to it's own structure.
So I think you are agreeing with me that the text "more fd's than are feasible to manage," should be removed from the FAQ? Thanks.
Quoted 6 lines
Assume for a moment that we had chosen a fd-as-watch approach. Now take beagle, which has tons of watches. Every time beagle goes through it's loop, it has to add each of those watch-fd's to the select table, then check for each one afterwards. It's pretty easy to see how this is unmanageable and a waste of CPU time.
I don't think there can be any question that having to tell select about each 'watch' is unreasonable.
Quoted 4 lines
Why bother though? The idr layer lets us have a integer that maps to a data structure in kernel space, pretty much for free.
Because some people think that creating a whole new abstraction when we have one (fds) that seem to fit the bill, is the wrong thing to do. I don't currently have an opinion on that, but I am trying to explore options with a bit of lateral thinking.
NeilBrown
Quoted 56 lines · Neil Brown
Quoted 41 lines · On Monday June 20, [email protected] wrote:
Quoted 32 lines · Neil Brown
I haven't yet decided if I like it or not, but maybe I have some useful comments to make...
Quoted 7 lines
(quoted diff) +7
-0+ +Q: What is the design decision behind using an-fd-per-device as +opposed to an fd-per-watch? + +A: An fd-per-watch quickly consumes more file descriptors than +are allowed, more fd's than are feasible to manage, and more +fd's than are ideally select()-able. Yes, root can bump theReview
Quoted 10 lines
(quoted diff) +10
-0+per-process fd limit and yes, users can use epoll, but requiring +both is silly and an extraneous requirement. A watch consumes +less memory than an open file, separating the number spaces is +thus sensible. The current design is what user-space developers +want: Users open the device, once, and add n watches, requiring +but one fd and no twiddling with fd limits. +Opening /dev/inotify two thousand times is silly. If we can +implement user-space's preferences cleanly--and we can, the idr +layer makes stuff like this trivial--then we should. +To address the particular points:
- "An fd-per-watch quickly consumes more file descriptors than are allowed"I note that the current limit if 'wd's is 8192 per user, compared with the default 'fd' limit of 1024 per process. So if a user has more than 8 processes making very heavy use of inotify, they would be better off with the file-descriptor limit...
So I don't find this argument convincing.Quoted 7 lines
I would suggest that it be removed, and you put in a separate section discussion resource usage and limits. You could explain why you don't use rlimit (probably not a hard case to win) and why the sysadmin cannot give different limits to different users (as a sys-admin, I would fight that), and why no one user would ever want more than about 8 processes using inotify :-)
Quoted 6 lines
Looking at beagle, it is a single process that can easily watch more than 1024 directories. Beagle would have to work around the 1024 limit by managing processes, each of which watch a fraction of the total watches. PITA. Also, 8192 was an arbitrary choice that seemed big enough for most users needs and all of these limits can be set in sysfs. Checkout
/sys/misc/inotify.I think you missed my point...
The FAQ entry which I found unconvincing said"Yes, root can bump the per-process fd limit ... but requiring [this]
is silly and an extraneous requirement."
Quoted 10 lines
So when you say "... all of these limits can be set in sysfs", you are effectively contradicting the FAQ entry.
Yes, 1024 is an arbitrary limit. But so is 8192.
The argument "We cannot use fd's because the arbitrary limit is lower than the arbitrary limit that I want to use" simply doesn't hold water.
There may well be other good arguments against 'fd's, but I'm trying to point out that this isn't one of them, and so shouldn't appear in this part of the FAQ.
Fair enough.
But, because the fd limit is established at 1024 and we will want the wd limit to be much higher than the current fd limit, we do need our own limit. fd's and wd's represent different things, why confuse the two limits?
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
- "more fd's than are feasible to manage,"
It's not clear what this means. The program will still need to manage lots of 'wd's. How is this different from handling 'fd's? The only 'manage'ment issue I could come up with is the hassle of shepherding all these 'wd's through a 'fork', only to close them before an 'exec'. Is that what you mean? If so it would help to make it more explicit.
Quoted 6 lines
Quoted 2 lines
The program doesn't really have to manage the wd's. A program simply needs a map from wd to it's own structure.
So I think you are agreeing with me that the text "more fd's than are feasible to manage," should be removed from the FAQ? Thanks.
No, I'm not. An application would have to manage them if they were fd's. My select() example in the previous mail explains.
Quoted 10 lines
Quoted 6 lines
Assume for a moment that we had chosen a fd-as-watch approach. Now take beagle, which has tons of watches. Every time beagle goes through it's loop, it has to add each of those watch-fd's to the select table, then check for each one afterwards. It's pretty easy to see how this is unmanageable and a waste of CPU time.
I don't think there can be any question that having to tell select about each 'watch' is unreasonable.
And if watches were represented as fd's we would have to do just that.
Quoted 8 lines
Quoted 4 lines
Why bother though? The idr layer lets us have a integer that maps to a data structure in kernel space, pretty much for free.
Because some people think that creating a whole new abstraction when we have one (fds) that seem to fit the bill, is the wrong thing to do. I don't currently have an opinion on that, but I am trying to explore options with a bit of lateral thinking.
I appreciate that you are trying to explore alternatives. I have done the same, and come to the conclusion that the current interface is the best approach. It is friendly to both the kernel and the user space programs which use inotify.
Signature · 1 line
John McCutchan <[email protected]>
Quoted 3 lines · Neil Brown
There may well be other good arguments against 'fd's, but I'm trying to point out that this isn't one of them, and so shouldn't appear in this part of the FAQ.
You raise a good point, although one could argue that raising the fd limit is not necessarily feasible.
There are other good arguments. With a single fd, there is a single item to block on, which is mapped to a single queue of events. The single fd returns all watch events and also any potential out-of-band data. If every fd was a separate watch,
- There would be no way to get event ordering. Events on file foo and file bar would pop poll() on both fd's, but there would be no way to tell which happened first. A single queue trivially gives you ordering.
- We'd have to maintain n fd's and n internal queues with state, versus just one. It is a lot messier in the kernel.
- User-space developers prefer the current API. The Beagle guys, for example, love it. Trust me, I asked. It is not a surprise: Who'd want to manage and block on 1000 fd's?
- You'd have to manage the fd's, as an example: call close() when you received a delete event.
- No way to get out of band data.
- 1024 is still too low. ;-)
When you talk about designing a file change notification system that scales to 1000s of directories, juggling 1000s of fd's just does not seem the right interface. It is too heavy.
I should add this to the FAQ, yes. ;-)
Robert Love
On Tuesday June 21, [email protected] wrote:
(Ok, so 3 weeks is too long (leave, family....), and inotify has been accepted into mainline, so maybe it isn't worth pursuing this discussion, but there are a couple of points I'd still like to make, or respond to....)
Quoted 14 lines · Neil Brown
Quoted 4 lines
There may well be other good arguments against 'fd's, but I'm trying to point out that this isn't one of them, and so shouldn't appear in this part of the FAQ.
You raise a good point, although one could argue that raising the fd limit is not necessarily feasible.
There are other good arguments. With a single fd, there is a single item to block on, which is mapped to a single queue of events. The single fd returns all watch events and also any potential out-of-band data. If every fd was a separate watch,
Quoted 4 lines
- There would be no way to get event ordering. Events on file foo and file bar would pop
poll()on both fd's, but there would be no way to tell which happened first. A single queue trivially gives you ordering.
"first"? Two processes on two processors modify two files both in the same timeslice. Which changed "first" ?
Timestamps is a well-proven mechanism for determining ordering, and your current 'cookie' is very far from a timestamp (or sequence-stamp, which would be as good).
Quoted 2 lines
- We'd have to maintain n fd's and n internal queues with state, versus just one. It is a lot messier in the kernel.
You don't maintain just one queue. You maintain one per open of /dev/inotify. So it is n vs m, not n vs 1.
Quoted 3 lines
- User-space developers prefer the current API. The Beagle guys, for example, love it. Trust me, I asked. It is not a surprise: Who'd want to manage and block on 1000 fd's?
But they still need to track 1000s of 'wd's. I'm sure the right library support would make any reasonable kernel interface easy to use for application developers.
Quoted 2 lines
- You'd have to manage the fd's, as an example: call
close()when you received a delete event.
This is not rocket science !
Quoted 1 line
- No way to get out of band data.
Not sure what you mean here.. Maybe IN_Q_OVERFLOW...
I first thought you might mean the filenames for directory events, but then thought maybe not. However while I was thinking this I also thought that your inotify_event structure doesn't seem to be designed for extension.
I recall you saying earlier that you had rejected requests for byte-range information about read/write, but that it could be open for reconsideration after inotify was well established in the kernel. Suppose you were to give in and provide such support, how would it appear in inotify_event?
I would have put the "len" at the start of the packet and have it give the length of the whole packet. Then 'mask' would list both the events and the accompanying fields. One bit to say "cookie is present" (it currently is very rarely meaningful, but always present), one bit for 'name is present' (I'm not sure how you are currently meant to tell if the name should be considered or not. maybe you have to know if 'wd' refers to a directory). Other bits for 'byterange is present', 'pid is present', 'uid is present', ....
Quoted 6 lines
- 1024 is still too low. ;-)
When you talk about designing a file change notification system that scales to 1000s of directories, juggling 1000s of fd's just does not seem the right interface. It is too heavy.
So I FINALLY asked myself the question "Why does he want 1000s of directories" and I come up with the answer "he probably doesn't".
What I suspect you really want to handle is a few objects where each object is either a file, a directory, or a directory tree. 1000's of directories sounds to me like you really want to monitor all the directories in a directory tree. But you don't even actually want that. You really want to watch all the files in all the directories in a directory tree (do tell me if I'm wrong).
Suppose you were to arrange that whenever a dentry is created or
walked: if the parent has an inotify_watch attached with a IN_RECURSE flag, then an appropriate inotify_watch was attached to the child (unless already present). It would be attached in such a way that it didn't hold a reference to the inode and so the inode could still be discarded when inactive.
This would (with a bit more work) allow you to get events for everything in a directory tree off just one filedescriptor.
You would need to be careful about allowing for sub-trees to be renamed out of the main tree, but I don't see that as insurmountable.
This approach has the benefit that you don't lock 1000's of inodes into memory when you aren't really interested in them.
So my preferred interface would be something like:
fd1 = open("/file/or/tree/i/want/to/watch", O_XXX) fd2 = sys_tnotify(fd1, BIT|MASK|OF|EVENTS(|TN_RECURSE),
...otherstuff);
if TN_RECURSE
ftw(fd1...)
/* This is needed partly because we need to make sure an inotify_watch is attached to any child already in cache, and partly because learning about changes isn't much use unless we have looked at the initial state
*/
....
select/poll/epoll on fd2
read(fd2, buf, maxbytes) returns a whole number of records
each record is:
__u32 record_size
__u32 events_and_fields /* lists the details of this record
* all other fields are optional and
* only present if the relevant bit
* is set
*/
__u64 inum
__u32 cookie
char name[] /* null terminated, 4byte padded */
....
name[] would be a path name from fd1 down to the affected object. This would probably be unreliable or even non-existent if a containing directory have been moved since (between event and read), but you would eventually see the directory move, and should then ftw the directory in the new location and check for interesting events "the old way".
One last little nit: The name "inotify" suggests to me that it is an inode-based notification scheme rather than a directory-entry-based notification scheme. However if I'm watching a directory and one of the files is changed though a hardlink to a different directory, or via NFS, then I may not see the change. I realise this is not really fixable. I assume it is/will be mentioned in the relevant documentation.
NeilBrown
PS I just noticed inotify.h spell writable as writtable :-)
Quoted 15 lines · Neil Brown
Quoted 14 lines · On Tuesday June 21, [email protected] wrote:
Quoted 3 lines · Neil Brown
There may well be other good arguments against 'fd's, but I'm trying to point out that this isn't one of them, and so shouldn't appear in this part of the FAQ.
You raise a good point, although one could argue that raising the fd limit is not necessarily feasible.
There are other good arguments. With a single fd, there is a single item to block on, which is mapped to a single queue of events. The single fd returns all watch events and also any potential out-of-band data. If every fd was a separate watch,
Quoted 7 lines
Quoted 4 lines
- There would be no way to get event ordering. Events on file foo and file bar would pop
poll()on both fd's, but there would be no way to tell which happened first. A single queue trivially gives you ordering."first"? Two processes on two processors modify two files both in the same timeslice. Which changed "first" ?
Yeah, you're right, when we are dealing with a tiny time slice on an SMP system, you can't really tell which came first. But this isn't the case we are trying to solve. We are trying to make things work on a larger (but still relatively small) time scale. Consider mv a b; mv b c; mv a c; If we had seperate event queues for a,b, and c, we would have no straight forward way to determine what order those events happened in.
Quoted 3 lines
Timestamps is a well-proven mechanism for determining ordering, and your current 'cookie' is very far from a timestamp (or sequence-stamp, which would be as good).
The cookie is never used as a way of determining order.
Quoted 8 lines
Quoted 4 lines
- We'd have to maintain n fd's and n internal queues with state, versus just one. It is a lot messier in the kernel.
You don't maintain just one queue. You maintain one per open of
/dev/inotify. So it is n vs m, not n vs 1.
Quoted 8 lines
Quoted 3 lines
- User-space developers prefer the current API. The Beagle guys, for example, love it. Trust me, I asked. It is not a surprise: Who'd want to manage and block on 1000 fd's?
But they still need to track 1000s of 'wd's. I'm sure the right library support would make any reasonable kernel interface easy to use for application developers.
Keeping track of a wd consists of mapping the wd to a structure. Keeping track of fds mean: mapping the fd to a structure, and for every loop:
add all the fd's to FD_SET
select on fd's
check EACH fd if it has events to be read.
and when app exists, close all fd's.
This is obviously a no go.
Quoted 14 lines
Quoted 1 line
- No way to get out of band data.
Not sure what you mean here.. Maybe
IN_Q_OVERFLOW...I first thought you might mean the filenames for directory events, but then thought maybe not. However while I was thinking this I also thought that your
inotify_eventstructure doesn't seem to be designed for extension.
I recall you saying earlier that you had rejected requests for byte-range information about read/write, but that it could be open for reconsideration after inotify was well established in the kernel. Suppose you were to give in and provide such support, how would it appear ininotify_event?
Quoted 9 lines
I would have put the "len" at the start of the packet and have it give the length of the whole packet. Then 'mask' would list both the events and the accompanying fields. One bit to say "cookie is present" (it currently is very rarely meaningful, but always present), one bit for 'name is present' (I'm not sure how you are currently meant to tell if the name should be considered or not. maybe you have to know if 'wd' refers to a directory). Other bits for 'byterange is present', 'pid is present', 'uid is present', ....
You raise a good point here. No apps currently using inotify expect it to be 100% stable, so now is the time to fix this.
Quoted 17 lines
Quoted 6 lines
- 1024 is still too low. ;-)
When you talk about designing a file change notification system that scales to 1000s of directories, juggling 1000s of fd's just does not seem the right interface. It is too heavy.
So I FINALLY asked myself the question "Why does he want 1000s of directories" and I come up with the answer "he probably doesn't".
What I suspect you really want to handle is a few objects where each object is either a file, a directory, or a directory tree. 1000's of directories sounds to me like you really want to monitor all the directories in a directory tree. But you don't even actually want that. You really want to watch all the files in all the directories in a directory tree (do tell me if I'm wrong).
Quoted 7 lines
Suppose you were to arrange that whenever a dentry is created or
walked: if the parent has aninotify_watchattached with aIN_RECURSEflag, then an appropriateinotify_watchwas attached to the child (unless already present). It would be attached in such a way that it didn't hold a reference to the inode and so the inode could still be discarded when inactive.
An inotify_watch has to map back to the inotify context that created it. Where would the events for this inotify context go? Not to mention the over head.
Quoted 3 lines
This would (with a bit more work) allow you to get events for everything in a directory tree off just one filedescriptor.
So say I create the initial watch at '/' with the recursive flag set, then something happens in /home/user/xxxxx/yyyyyy/zzzzzz/dddddd/ for the sake of the argument, lets say a file is being downloaded call foo.
Each time foo is written to, the kernel needs to traverse up the directory tree to build the full path name, allocate the memory for the event and queue it up.
The over head in constructing the full path name is unacceptable, and the memory needs of this approach would get huge, especially if the watcher didn't bother to read events from the queue.
For recursive watching to happen, you either need to walk down the directory tree at the time of watch creation, or walk up the directory tree for each event. Take your pick. Neither seems acceptable to me.
Quoted 6 lines
You would need to be careful about allowing for sub-trees to be renamed out of the main tree, but I don't see that as insurmountable.
This approach has the benefit that you don't lock 1000's of inodes into memory when you aren't really interested in them.
Yeah, you just waste tons of time constructing full path names and waste lots of memory saving those names around until the events are read.
Quoted 9 lines
One last little nit: The name "inotify" suggests to me that it is an inode-based notification scheme rather than a directory-entry-based notification scheme. However if I'm watching a directory and one of the files is changed though a hardlink to a different directory, or via NFS, then I may not see the change. I realise this is not really fixable. I assume it is/will be mentioned in the relevant documentation.
This will be added to the Documentation.
Quoted 5 lines
NeilBrown
PS I just noticed
inotify.hspell writable as writtable :-)
Thanks.
Quoted 7 lines · Robert Love
Quoted 4 lines · Andrew Morton
So. It's not too late. Please spend an hour and write up the Inofity Implementation FAQ? You probably remember and fully understand what all of our objections are and I know that you have explanations and rebuttals at hand.
Quoted 6 lines
I wrote this up the first time you asked:
Documentation/filesystems/inotify.txt
It sounds like its never been addressed because some people don't take different views as an acceptable solution.
Yes, this is the file that I quoted in my mail. However, "because adding three or four new system calls that mirrored open, close, and ioctl seemed silly" doesn't really explain the choice.
An explanation along the lines of "neither ioctl on cdev nor a syscall based approach is made everyone happy, so we decided to stick with the one that is already used" might give a little more insight.
However, this still does not explain why the choice here is different from epoll, as I clearly remember the decision to move from /dev/epoll to the final sys_epoll* interface [1]. If it's just a matter of different views like "ioctl is ugly" vs "syscall is silly", I would have gone for a sys_inotify_create()/sys_inotify_ctl() based interface simply for the reason of consistency with existing interfaces like epoll or mq_open().
Of course, at this point, compatibility with the existing user base might already be a much stronger argument.
Arnd <><
Quoted 3 lines · Arnd Bergmann
An explanation along the lines of "neither ioctl on cdev nor a syscall based approach is made everyone happy, so we decided to stick with the one that is already used" might give a little more insight.
I will add exactly this to the FAQ, thank you.
I suspect this is the situation that /dev/epoll faced, although in the case of epoll, the opening the device did not have any real significance (you would generally not restrict access and you would only open the device once).
Robert Love
Quoted 9 lines · Arnd Bergmann
Sorry to bring up a topic that should have been settled a long time ago.
I found that the interface consisting of
- open a handle
- add a file descriptor with an event mask to handle
- remove a file/watch descriptor from handle
- wait on handle, get events
- close handle
Quoted 3 lines
in inotify is _very_ similar to how epoll is represented to user space. Is there a good reason that epoll is a set of syscalls while inotify is a character device, or is one of them simply wrong?
It's because Robert and John insist on their horrible interface and simply ignore any feedback on how to do a better one.
Quoted 10 lines · Christoph Hellwig
Quoted 9 lines · Arnd Bergmann
Sorry to bring up a topic that should have been settled a long time ago.
I found that the interface consisting of
- open a handle
- add a file descriptor with an event mask to handle
- remove a file/watch descriptor from handle
- wait on handle, get events
- close handle
Quoted 6 lines
Quoted 3 lines
in inotify is _very_ similar to how epoll is represented to user space. Is there a good reason that epoll is a set of syscalls while inotify is a character device, or is one of them simply wrong?
It's because Robert and John insist on their horrible interface and simply ignore any feedback on how to do a better one.
Your feedback was considered during the off list discussion, that you were a part of. In the end Robert, Andrew, and I agreed on the current interface.
Quoted 2 lines · Christoph Hellwig
It's because Robert and John insist on their horrible interface and simply ignore any feedback on how to do a better one.
We considered your feedback, Christoph. Ultimately, John, Andrew, and I settled on the current approach. In life, not everyone agrees on every little detail and there usually exists a large difference between "not exactly the same" and "horrible". And never does the histrionics result in anything productive.
Robert Love
Quoted 5 lines · Robert Love
We considered your feedback, Christoph. Ultimately, John, Andrew, and I settled on the current approach. In life, not everyone agrees on every little detail and there usually exists a large difference between "not exactly the same" and "horrible". And never does the histrionics result in anything productive.
This shows exactly on how you're refusing feedback on the basis totally unfounded claims again.
You are using ioctl as an really bad syscall multiplexer. You're not using the file descriptor it's called on at all, so it does not qualify as a valid ioctl() usage even under the most lax rules.
Also you claimed the resource shortage for the proposed architecture with just a single syscall, aka one watch per fd without showing any reasons why that would be true, in fact by any means there's no reason to believe file descriptors are a rare ressource in a modern Linux system.
I don't care whether you adopt my interface proposal or a different passable one, but the current one is not acceptable at all.
Quoted 3 lines · Christoph Hellwig
You are using ioctl as an really bad syscall multiplexer. You're not using the file descriptor it's called on at all, so it does not qualify as a valid
ioctl()usage even under the most lax rules.
We provide two different ioctl commands, it is not a bad multiplexer. We have discussed this before.
We do use the fd. It maps back to the inotify device.
Quoted 4 lines
Also you claimed the resource shortage for the proposed architecture with just a single syscall, aka one watch per fd without showing any reasons why that would be true, in fact by any means there's no reason to believe file descriptors are a rare ressource in a modern Linux system.
It is not implausible to believe that a system might have the default maximum for file descriptors (not very high) but allow a _much_ greater number of inotify watches (32k, say).
That is our rationale. I hear what you are saying, I understand it, and at the end of the day I disagree. I appreciate your input, but I feel otherwise.
Quoted 2 lines
I don't care whether you adopt my interface proposal or a different passable one, but the current one is not acceptable at all.
Everything to you is "really bad" and "totally unacceptable". Chill out. Stop ranting so much and enjoy life.
Robert Love
Quoted 8 lines · Robert Love
Quoted 3 lines
You are using ioctl as an really bad syscall multiplexer. You're not using the file descriptor it's called on at all, so it does not qualify as a valid
ioctl()usage even under the most lax rules.We provide two different ioctl commands, it is not a bad multiplexer. We have discussed this before.
We do use the fd. It maps back to the inotify device.
inotify does indeed use file->private data to retrieve the inotify_dev structure. Of which by design exists a single instance only. So yes, you do not use the file descriptor at all.
This really looks like copied from the ioctl abuse 101 example code. We are beating up driver writers for stupidities like that all the time, and inotify is not going to get a special treatment just because it's core code.
Quoted 8 lines
Quoted 4 lines
Also you claimed the resource shortage for the proposed architecture with just a single syscall, aka one watch per fd without showing any reasons why that would be true, in fact by any means there's no reason to believe file descriptors are a rare ressource in a modern Linux system.
It is not implausible to believe that a system might have the default maximum for file descriptors (not very high) but allow a _much_ greater number of inotify watches (32k, say).
And a default limit matters exactly how?
Quoted 2 lines
Everything to you is "really bad" and "totally unacceptable". Chill out. Stop ranting so much and enjoy life.
Thanks a lot, I'm enjoying life a lot when I don't happen to have to deal with ignorant people :)
Quoted 3 lines · Christoph Hellwig
inotify does indeed use
file->privatedata to retrieve theinotify_devstructure. Of which by design exists a single instance only. So yes, you do not use the file descriptor at all.
Sorry, I am not following how we "do not use the file descriptor at all". It is given to the ioctl and, as you said, used to find the inotify_dev. The file descriptor maps to the device, and vice versa.
You _can_ have multiple open devices per process.
Quoted 1 line
And a default limit matters exactly how?
Because we want people to actually use inotify, to make the desktop better, and in the real world "become root and bump the fd limit" is not always feasible. The fd limit might be low for other reasons.
Quoted 2 lines
Thanks a lot, I'm enjoying life a lot when I don't happen to have to deal with ignorant people :)
Just because we disagree does not make either of us ignorant. Indeed, even if one of us were wrong, we need not be ignorant--wrong word. Neither of us is untaught on or unacquainted with the kernel. We are just viewing things differently.
Robert Love
Quoted 3 lines · Arnd Bergmann
in inotify is _very_ similar to how epoll is represented to user space. Is there a good reason that epoll is a set of syscalls while inotify is a character device, or is one of them simply wrong?
Tangentially inotify does some of what the DMAPI interface(s) want to do (albeit somewhat simpler). It would seem since DMAPI in it's current incarnation won't "go away and die[1]" it might be worth considering how much overlap there is.
[1] More than one fs has this out-of-tree and people are using it in various forms.