Messages in this thread | | | Date | Tue, 28 Mar 2006 11:51:26 +0400 | From | Evgeniy Polyakov <> | Subject | Re: [2.6.16 PATCH] Filesystem Events Connector v4 |
| |
On Mon, Mar 27, 2006 at 11:05:38PM +0800, Yi Yang (yang.y.yi@gmail.com) wrote: > This release is v4, compared with v3, it adds and improve some functions: > * The user can set event filter in order to just get > those events who concerns, filter may be based on > event mask, pid, uid and gid. > * it removes the atomic_t variable cn_fs_listener and > adopt a smart way to decide whether it should send a > event. > * Add several filter operation commands and acknowleges > , add a new struct fsevent_filter as command struct. > > basically, these functions enable it to meet the user's requirements better, > but, it can't do better because of connector broadcast property, I plan to > use netlink to let different user see different events, filter is based on > userspace appplication using it, every application can set its own filters > and see different events. > > drivers/connector/Kconfig | 12 + > drivers/connector/Makefile | 1 > drivers/connector/cn_fs.c | 298 +++++++++++++++++++++++++++++++++++++++++ > drivers/connector/connector.c | 4 > fs/namespace.c | 12 + > include/linux/connector.h | 8 - > include/linux/fsevent.h | 122 +++++++++++++++++ > include/linux/fsnotify.h | 37 +++++ > 8 files changed, 490 insertions(+), 4 deletions(-) > > Signed-off-by: Yi Yang <yang.y.yi@gmail.com>
> + > +#ifdef CONFIG_FS_EVENTS > + { > + u32 fsevent_mask = 0; > + if (ia_valid & (ATTR_UID | ATTR_GID | ATTR_MODE)) > + fsevent_mask |= FSEVENT_MODIFY_ATTRIB; > + if ((ia_valid & ATTR_ATIME) && (ia_valid & ATTR_MTIME)) > + fsevent_mask |= FSEVENT_MODIFY_ATTRIB; > + else if (ia_valid & ATTR_ATIME) > + fsevent_mask |= FSEVENT_ACCESS; > + else if (ia_valid & ATTR_MTIME) > + fsevent_mask |= FSEVENT_MODIFY; > + if (ia_valid & ATTR_SIZE) > + fsevent_mask |= FSEVENT_MODIFY; > + if (fsevent_mask) > + raise_fsevent(dentry, fsevent_mask); > + } > +#endif /* CONFIG_FS_EVENTS */ > }
Coding style?
> --- a/drivers/connector/connector.c.orig 2006-03-27 21:35:15.000000000 +0800 > +++ b/drivers/connector/connector.c 2006-03-27 21:35:53.000000000 +0800 > @@ -111,9 +111,7 @@ int cn_netlink_send(struct cn_msg *msg, > > NETLINK_CB(skb).dst_group = group; > > - netlink_broadcast(dev->nls, skb, 0, group, gfp_mask); > - > - return 0; > + return (netlink_broadcast(dev->nls, skb, 0, group, gfp_mask)); > > nlmsg_failure: > kfree_skb(skb);
This error value is propageted back in current connector code already.
> + > + /* The following definitions are commands for event filter > + * , in acknowlege event for the corresponding command, it > + * will set to type field of struct fsevent. > + */
Not an eye-candy.
> + FSEVENT_FILTER_ALL = 0x08000000, /* For all events */ > + FSEVENT_FILTER_PID = 0x10000000, /* For some process ID */ > + FSEVENT_FILTER_UID = 0x20000000, /* For some user ID */ > + FSEVENT_FILTER_GID = 0x40000000, /* For some group ID */ > + > + FSEVENT_ISDIR = 0x80000000 /* It is set for a dir */ > +}; > + > +#define FSEVENT_MASK 0x800003ff > + > +typedef unsigned long fsevent_mask_t; > + > +struct fsevent_filter { > + /* filter type, it just is one or bit-or of them > + * FSEVENT_FILTER_ALL > + * FSEVENT_FILTER_PID > + * FSEVENT_FILTER_UID > + * FSEVENT_FILTER_GID > + */ > + enum fsevent_type type; /* filter type */ > + > + /* mask of file system events the user listen or ignore > + * if the user need to ignore all the events of some pid > + * , gid or uid, he(she) must set mask to FSEVENT_MASK. > + */
"," on the new line.
> +static void turn_off_fsevents(void) > +{ > + write_lock(&fsevents_lock); > + fsevents_mask = 0; > + fsevents_pid_mask = 0; > + filter_pid = -1; > + fsevents_uid_mask = 0; > + filter_uid = -1; > + fsevents_gid_mask = 0; > + filter_gid = -1; > + write_unlock(&fsevents_lock); > +} > + > +static int filter_fsevents(u32 * mask) > +{ > + int ret = 0; > + > + (*mask) &= FSEVENT_MASK; > + read_lock(&fsevents_lock);
You do want to use RCU locking here. It is fast path and it is not allowed to get global smp-unfriendly read lock.
> + if (current->pid == filter_pid) { > + (*mask) &= fsevents_pid_mask; > + if ((*mask) == 0) { > + ret = -2; > + } > + goto release_lock; > + } > + > + if (current->uid == filter_uid) { > + (*mask) &= fsevents_uid_mask; > + if ((*mask) == 0) { > + ret = -3; > + } > + goto release_lock; > + } > + > + if (current->gid == filter_gid) { > + (*mask) &= fsevents_gid_mask; > + if ((*mask) == 0) { > + ret = -4; > + } > + goto release_lock; > + } > + > + if ((((*mask) & FSEVENT_ISDIR) == FSEVENT_ISDIR) > + & ((fsevents_mask & FSEVENT_ISDIR) == 0)) { > + ret = -1; > + goto release_lock; > + } > + > + (*mask) &= fsevents_mask; > + if ((*mask) == 0) { > + ret = -5; > + } > + > +release_lock: > + read_unlock(&fsevents_lock); > + return ret; > +}
Can it be somehow turned off or moved into per-cpu variables? It is a lot of smp-unfriendly access of global variables.
It of course costs much less than allocations and copies, but no global lock at least.
> + if (newname) { > + event->new_fname_len = strlen(newname); > + append_string(&nameptr, newname, event->new_fname_len); > + event->len += event->new_fname_len + 1; > + }
A space from nowhere.
> + if (ret == -ESRCH) { > + turn_off_fsevents(); > + }
Coding style.
> +void raise_fsevent(struct dentry * dentryp, u32 mask) > +{ > + if (dentryp->d_inode && (MAJOR(dentryp->d_inode->i_rdev) == 4)) > + return; > + __raise_fsevent(dentryp->d_name.name, NULL, mask); > +}
Yep, tty can mess the things. Maybe remove all special devices at all?
> +EXPORT_SYMBOL_GPL(raise_fsevent); > + > +void raise_fsevent_create(struct inode * inode, const char * name, u32 mask) > +{ > + read_lock(&fsevents_lock); > + __raise_fsevent(name, NULL, mask); > +}
Lost read_lock here - you will grab it in __raise_fsevent()->filter_fsevents().
> +static int __init cn_fs_init(void) > +{ > + int err; > + > + if ((err = cn_add_callback(&cn_fs_event_id, "cn_fs", > + &cn_fsevent_filter_ctl))) { > + printk(KERN_WARNING "cn_fs failed to register\n"); > + return err; > + } > + return 0; > +} > + > +module_init(cn_fs_init);
Add module_exit() too.
Main issue is locking in this patchset - it is not allowed to have such a global lock in those pathes, moving this to RCU is the way to go and definitely not a problem to implement.
Thank you.
-- Evgeniy Polyakov - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |