Re: [2.6.16 PATCH] Filesystem Events Connector v4

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/

Relevant Pages

  • Re: [2.6.16 PATCH] Filesystem Events Connector v4
    ... * The user can set event filter in order to just get ... event mask, pid, uid and gid. ... but no global lock at least. ...
    (Linux-Kernel)
  • DirectShow base classes and the possible deadlocks
    ... CBaseFilter::Pause locks the filter lock (okay, it's recursive locking, ... twice and release them ... stop the filter, so the lock is recursively acquired, and the deadlock never ...
    (microsoft.public.win32.programmer.directx.video)
  • Re: Digital PLL acquisition problem
    ... I want to lock on a 22170 Hz sine signal in 10 ms lock time ... So I put a window at the ouput of my filter to only sweep in a 2600 Hz ...
    (comp.dsp)
  • Re: Digital PLL acquisition problem
    ... I want to lock on a 22170 Hz sine signal in 10 ms lock time ... So I put a window at the ouput of my filter to only sweep in a 2600 Hz ... If you sample the data first, then implement your PLL, a pure-logic state machine frequency/phase detector will have an uncertainty around the edges of +/- 1/2 sample, which quite broad for many purposes. ... Assuming that the tone is strong enough you can get the same effect by estimating the incoming signal's phase and counting rotations -- this works very well in practice, with much noisier signals than you can hope to use a frequency/phase detector on, and doesn't have the edge uncertainty. ...
    (comp.dsp)
  • Re: DirectShow base classes and the possible deadlocks
    ... >> start the filter, and another thread tries to stop the filter, is: ... the CBaseFilter no longer need to lock the FilterLock before ... >thread is blocked because it is waiting for an event in FillBuffer to be set. ...
    (microsoft.public.win32.programmer.directx.video)