Re: [2.6.16 PATCH] Filesystem Events Connector v4
From: Yi Yang
Date: Tue Mar 28 2006 - 08:26:40 EST
Evgeniy Polyakov åé:
On Mon, Mar 27, 2006 at 11:05:38PM +0800, Yi Yang (yang.y.yi@xxxxxxxxx) 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@xxxxxxxxx>
+
+#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.
Which version of kernel do you mean? for 2.6.16, it doesn't return
netlink_broadcast's return value.
+
+ /* 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.
You are very right, I'll convert to RCU 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.
I'll plan to just move them to specific process so that they are only
for this process, so
lock can be removed completely.
+ 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.
Thank for your careful review very much, I'll commit a new version to
fix your concerns, thanks again.
-
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/