Re: [PATCH -V2 06/13] dnotify: reimplement dnotify using fsnotify

From: Andrew Morton
Date: Tue Apr 07 2009 - 19:14:38 EST


On Fri, 27 Mar 2009 16:05:37 -0400
Eric Paris <eparis@xxxxxxxxxx> wrote:

> Reimplement dnotify using fsnotify.
>
>
> ...
>
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1431,6 +1431,8 @@ S: Orphan
> DIRECTORY NOTIFICATION (DNOTIFY)
> P: Stephen Rothwell
> M: sfr@xxxxxxxxxxxxxxxx
> +P: Eric Paris
> +M: eparis@xxxxxxxxxxxxxx

hah!

> L: linux-kernel@xxxxxxxxxxxxxxx
> S: Supported
>
>
> ...
>
> +static int dnotify_should_send_event(struct fsnotify_group *group, struct inode *inode, __u32 mask)

could return bool, if you like that sort of thing.

> +{
> + struct fsnotify_mark_entry *entry;
> + int send;
> +
> + /* !dir_notify_enable should never get here, don't waste time checking
> + if (!dir_notify_enable)
> + return 0; */
> +
> + /* not a dir, dnotify doesn't care */
> + if (!S_ISDIR(inode->i_mode))
> + return 0;
> +
> + spin_lock(&inode->i_lock);
> + entry = fsnotify_find_mark_entry(group, inode);
> + spin_unlock(&inode->i_lock);
> +
> + /* no mark means no dnotify watch */
> + if (!entry)
> + return 0;
> +
> + spin_lock(&entry->lock);
> + send = !!(mask & entry->mask);
> + spin_unlock(&entry->lock);
> + fsnotify_put_mark(entry);
> +
> + return send;
> +}
> +
>
> ...
>
> -int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
> +/* this conversion is done only at watch creation */
> +static inline __u32 convert_arg(unsigned long arg)

The compiler will inline this anyway.

s/ / /

> +{
> + __u32 new_mask = FS_EVENT_ON_CHILD;
> +
> + if (arg & DN_MULTISHOT)
> + new_mask |= FS_DN_MULTISHOT;
> + if (arg & DN_DELETE)
> + new_mask |= (FS_DELETE | FS_MOVED_FROM);
> + if (arg & DN_MODIFY)
> + new_mask |= FS_MODIFY;
> + if (arg & DN_ACCESS)
> + new_mask |= FS_ACCESS;
> + if (arg & DN_ATTRIB)
> + new_mask |= FS_ATTRIB;
> + if (arg & DN_RENAME)
> + new_mask |= FS_DN_RENAME;
> + if (arg & DN_CREATE)
> + new_mask |= (FS_CREATE | FS_MOVED_TO);
> +
> + return new_mask;
> +}
> +
> +static int attach_dn(struct dnotify_struct *dn, struct dnotify_mark_entry *dnentry, fl_owner_t id,
> + int fd, struct file *filp, __u32 mask)

Given that the definition is already broken over two lines, there's
nothing to be gained by making it look messy in 80-cols?

> {
> - struct dnotify_struct *dn;
> struct dnotify_struct *odn;
> struct dnotify_struct **prev;
> - struct inode *inode;
> - fl_owner_t id = current->files;
> - struct file *f;
> - int error = 0;
>
> - if ((arg & ~DN_MULTISHOT) == 0) {
> - dnotify_flush(filp, id);
> - return 0;
> - }
> - if (!dir_notify_enable)
> - return -EINVAL;
> - inode = filp->f_path.dentry->d_inode;
> - if (!S_ISDIR(inode->i_mode))
> - return -ENOTDIR;
> - dn = kmem_cache_alloc(dn_cache, GFP_KERNEL);
> - if (dn == NULL)
> - return -ENOMEM;
> - spin_lock(&inode->i_lock);
> - prev = &inode->i_dnotify;
> + prev = &dnentry->dn;
> while ((odn = *prev) != NULL) {
> + /* do we already have a dnotify struct and we are just adding more events? */
> if ((odn->dn_owner == id) && (odn->dn_filp == filp)) {
> odn->dn_fd = fd;
> - odn->dn_mask |= arg;
> - inode->i_dnotify_mask |= arg & ~DN_MULTISHOT;
> - goto out_free;
> + odn->dn_mask |= mask;
> + return -EEXIST;
> }
> prev = &odn->dn_next;
> }
>
> - rcu_read_lock();
> - f = fcheck(fd);
> - rcu_read_unlock();
> - /* we'd lost the race with close(), sod off silently */
> - /* note that inode->i_lock prevents reordering problems
> - * between accesses to descriptor table and ->i_dnotify */
> - if (f != filp)
> - goto out_free;
> -
> - error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
> - if (error)
> - goto out_free;
> -
> - dn->dn_mask = arg;
> + dn->dn_mask = mask;
> dn->dn_fd = fd;
> dn->dn_filp = filp;
> dn->dn_owner = id;
> - inode->i_dnotify_mask |= arg & ~DN_MULTISHOT;
> - dn->dn_next = inode->i_dnotify;
> - inode->i_dnotify = dn;
> - spin_unlock(&inode->i_lock);
> - return 0;
> + dn->dn_next = dnentry->dn;
> + dnentry->dn = dn;
>
> -out_free:
> - spin_unlock(&inode->i_lock);
> - kmem_cache_free(dn_cache, dn);
> - return error;
> + return 0;
> }
>
>
> ...
>
> -extern void __inode_dir_notify(struct inode *, unsigned long);
> +#define ALL_DNOTIFY_EVENTS (FS_DELETE | FS_DELETE_CHILD |\
> + FS_MODIFY | FS_MODIFY_CHILD |\
> + FS_ACCESS | FS_ACCESS_CHILD |\
> + FS_ATTRIB | FS_ATTRIB_CHILD |\
> + FS_CREATE | FS_DN_RENAME |\
> + FS_MOVED_FROM | FS_MOVED_TO)

"DNOTIFY_ALL_EVENTS"?

> extern void dnotify_flush(struct file *, fl_owner_t);
> extern int fcntl_dirnotify(int, struct file *, unsigned long);
> -extern void dnotify_parent(struct dentry *, unsigned long);
> -
> -static inline void inode_dir_notify(struct inode *inode, unsigned long event)
> -{
> - if (inode->i_dnotify_mask & (event))
> - __inode_dir_notify(inode, event);
> -}
>
>
> ...
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/