lkml.org 
[lkml]   [2006]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@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.
>
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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-03-28 15:32    [W:0.052 / U:35.440 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site