lkml.org 
[lkml]   [2009]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -V2 02/13] fsnotify: unified filesystem notification backend
On Fri, 27 Mar 2009 16:05:15 -0400
Eric Paris <eparis@redhat.com> wrote:

> fsnotify is a backend for filesystem notification. fsnotify does
> not provide any userspace interface but does provide the basis
> needed for other notification schemes such as dnotify. fsnotify
> can be extended to be the backend for inotify or the upcoming
> fanotify. fsnotify provides a mechanism for "groups" to register for
> some set of filesystem events and to then deliver those events to
> those groups for processing.
>
> fsnotify has a number of benefits, the first being actually shrinking the size
> of an inode. Before fsnotify to support both dnotify and inotify an inode had
>
> unsigned long i_dnotify_mask; /* Directory notify events */
> struct dnotify_struct *i_dnotify; /* for directory notifications */
> struct list_head inotify_watches; /* watches on this inode */
> struct mutex inotify_mutex; /* protects the watches list
>
> But with fsnotify this same functionallity (and more) is done with just
>
> __u32 i_fsnotify_mask; /* all events for this inode */
> struct hlist_head i_fsnotify_mark_entries; /* marks on this inode */
>
> That's right, inotify, dnotify, and fanotify all in 64 bits. We used that
> much space just in inotify_watches alone, before this patch set.
>
> fsnotify object lifetime and locking is MUCH better than what we have today.
> inotify locking is incredibly complex. See 8f7b0ba1c8539 as an example of
> what's been busted since inception. inotify needs to know internal semantics
> of superblock destruction and unmounting to function. The inode pinning and
> vfs contortions are horrible.
>
> no fsnotify implementers do allocation under locks. This means things like
> f04b30de3 which (due to an overabundance of caution) changes GFP_KERNEL to
> GFP_NOFS can be reverted. There are no longer any allocation rules when using
> or implementing your own fsnotify listener.
>
> fsnotify paves the way for fanotify. people may not care for the original
> companies that pushed for TALPA, but fanotify was designed with flexibility in
> mind. A first group that wants fanotify like interfaces is the readahead
> group. So they can be profiling at boot time in order to dynamicly tune
> readahead to help with boot speed. I've got other ideas how to use fanotify
> to speed up boot when dealing with encrypted mounts, but I'm not ready to say
> it yet since I don't know if my idea will work.
>
> This patch series just builds fsnotify to the point that it can implement
> dnotify and inotify_user. Patches exist and will be sent soon after
> acceptance to finish the in kernel inotify conversion (audit) and implement
> fanotify.
>
>
> ...
>
> +void fsnotify(struct inode *to_tell, __u32 mask, void *data, int data_is)
> +{
> + struct fsnotify_group *group;
> + struct fsnotify_event *event = NULL;
> + int idx;
> +
> + if (list_empty(&fsnotify_groups))
> + return;
> +
> + if (!(mask & fsnotify_mask))
> + return;
> +
> + /*
> + * SRCU!! the groups list is very very much read only and the path is

I hinted to paulmck that he might like to review this ;)

> + * very hot. The VAST majority of events are not going to need to do
> + * anything other than walk the list so it's crazy to pre-allocate.
> + */
> + idx = srcu_read_lock(&fsnotify_grp_srcu);
> + list_for_each_entry_rcu(group, &fsnotify_groups, group_list) {
> + if (mask & group->mask) {
> + if (!event) {
> + event = fsnotify_create_event(to_tell, mask, data, data_is);
> + /* shit, we OOM'd and now we can't tell, maybe
> + * someday someone else will want to do something
> + * here */
> + if (!event)
> + break;
> + }
> + group->ops->handle_event(group, event);
> + }
> + }
> + srcu_read_unlock(&fsnotify_grp_srcu, idx);
> + /*
> + * fsnotify_create_event() took a reference so the event can't be cleaned
> + * up while we are still trying to add it to lists, drop that one.
> + */
> + if (event)
> + fsnotify_put_event(event);
> +}
>
> ...
>
> --- /dev/null
> +++ b/fs/notify/fsnotify.h
> @@ -0,0 +1,17 @@
> +#ifndef _LINUX_FSNOTIFY_PRIVATE_H
> +#define _LINUX_FSNOTIFY_PRIVATE_H

The #define doesn't match the filename?

> +#include <linux/dcache.h>
> +#include <linux/list.h>
> +#include <linux/fs.h>
> +#include <linux/path.h>
> +#include <linux/spinlock.h>
> +
>
> ...
>
> +
> +DEFINE_MUTEX(fsnotify_grp_mutex);

This has global scope, but isn't declared in fsnotify.h

> +struct srcu_struct fsnotify_grp_srcu;
> +LIST_HEAD(fsnotify_groups);
> +__u32 fsnotify_mask;

It's nice to provide comments explaining the role of key globals such
as these. fsnotify_mask is particularly unobvious.

> +void fsnotify_recalc_global_mask(void)
> +{
> + struct fsnotify_group *group;
> + __u32 mask = 0;
> + int idx;
> +
> + idx = srcu_read_lock(&fsnotify_grp_srcu);
> + list_for_each_entry_rcu(group, &fsnotify_groups, group_list) {
> + mask |= group->mask;
> + }

unneeded braces.

> + srcu_read_unlock(&fsnotify_grp_srcu, idx);
> + fsnotify_mask = mask;
> +}

What does this function do?

It's hard to review code when things such as this are left unexplained.

> +static void fsnotify_add_group(struct fsnotify_group *group)
> +{
> + list_add_rcu(&group->group_list, &fsnotify_groups);
> + group->evicted = 0;
> +}

Locking? Seems to requrie that callers hold fsnotify_grp_mutex?

> +static void fsnotify_get_group(struct fsnotify_group *group)
> +{
> + atomic_inc(&group->refcnt);
> +}
> +
> +static void fsnotify_destroy_group(struct fsnotify_group *group)
> +{
> + if (group->ops->free_group_priv)
> + group->ops->free_group_priv(group);
> +
> + kfree(group);
> +}
> +
> +static void __fsnotify_evict_group(struct fsnotify_group *group)
> +{
> + BUG_ON(!mutex_is_locked(&fsnotify_grp_mutex));
> +
> + if (!group->evicted)
> + list_del_rcu(&group->group_list);
> + group->evicted = 1;
> +}

Why is this called "evict"? In Linux, the term "eviction" implies some
sort of involuntary asynchronous reclaimation. But afaict (and lacking
explanatory documentation) this function seems to be a plain old
freeing function. So why is it not called fsnotify_free_group()?

This is all a bit unaproachable.

> +void fsnotify_evict_group(struct fsnotify_group *group)
> +{
> + mutex_lock(&fsnotify_grp_mutex);
> + __fsnotify_evict_group(group);
> + mutex_unlock(&fsnotify_grp_mutex);
> +}
> +
> +void fsnotify_put_group(struct fsnotify_group *group)
> +{
> + if (!atomic_dec_and_mutex_lock(&group->refcnt, &fsnotify_grp_mutex))
> + return;
> +
> + /* OK, now we know that there's no other users *and* we hold mutex,
> + * so no new references will appear */

The usual commenting style is

/*
* OK, now we know that there's no other users *and* we hold mutex,
* so no new references will appear
*/
> + __fsnotify_evict_group(group);
> +
> + /* now it's off the list, so the only thing we might care about is
> + * srcu acces.... */

"access"

> + mutex_unlock(&fsnotify_grp_mutex);
> + synchronize_srcu(&fsnotify_grp_srcu);
> +
> + /* and now it is really dead. _Nothing_ could be seeing it */
> + fsnotify_recalc_global_mask();
> + fsnotify_destroy_group(group);
> +}
> +
>
> ...
>
> +/*
> + * Either finds an existing group which matches the group_num, mask, and ops or
> + * creates a new group and adds it to the global group list. In either case we
> + * take a reference for the group returned.
> + *
> + * low use function, could be faster to check if the group is there before we do
> + * the allocation and the initialization, but this is only called when notification
> + * systems make changes, so why make it more complex?

Yup. But that would seem to be a pretty simple change to make?

> + */
> +struct fsnotify_group *fsnotify_obtain_group(unsigned int group_num, __u32 mask,
> + const struct fsnotify_ops *ops)
> +{
> + struct fsnotify_group *group, *tgroup;
> +
> + group = kmalloc(sizeof(struct fsnotify_group), GFP_KERNEL);
> + if (!group)
> + return ERR_PTR(-ENOMEM);
> +
> + atomic_set(&group->refcnt, 1);
> +
> + group->group_num = group_num;
> + group->mask = mask;
> +
> + group->ops = ops;
> +
> + mutex_lock(&fsnotify_grp_mutex);
> + tgroup = fsnotify_find_group(group_num, mask, ops);
> + if (tgroup) {
> + /* group already exists */
> + mutex_unlock(&fsnotify_grp_mutex);
> + /* destroy the new one we made */
> + fsnotify_put_group(group);
> + return tgroup;
> + }
> +
> + /* group not found, add a new one */
> + fsnotify_add_group(group);

This is the only fsnotify_add_group() callsite and it's just two lines.
Open-code it here?

> + mutex_unlock(&fsnotify_grp_mutex);
> +
> + if (mask)
> + fsnotify_recalc_global_mask();

I'd understand this if I knew what fsnotify_mask does :(

> + return group;
> +}
>
> ...
>
> +void fsnotify_put_event(struct fsnotify_event *event)
> +{
> + if (!event)
> + return;
> +
> + if (atomic_dec_and_test(&event->refcnt)) {
> + if (event->data_type == FSNOTIFY_EVENT_PATH) {
> + path_put(&event->path);
> + event->path.dentry = NULL;
> + event->path.mnt = NULL;

Why are these fields zeroed here? If it's for debugging then slab
poisoning should suffice?

> + }
> +
> + event->mask = 0;
> +
> + kmem_cache_free(event_kmem_cache, event);
> + }
> +}
> +
> +struct fsnotify_event *fsnotify_create_event(struct inode *to_tell, __u32 mask, void *data, int data_type)
> +{
> + struct fsnotify_event *event;
> +
> + event = kmem_cache_alloc(event_kmem_cache, GFP_KERNEL);
> + if (!event)
> + return NULL;
> +
> + atomic_set(&event->refcnt, 1);
> +
> + spin_lock_init(&event->lock);
> +
> + event->path.dentry = NULL;
> + event->path.mnt = NULL;
> + event->inode = NULL;
> +
> + event->to_tell = to_tell;
> +
> + switch (data_type) {
> + case FSNOTIFY_EVENT_FILE: {
> + struct file *file = data;
> + struct path *path = &file->f_path;
> + event->path.dentry = path->dentry;
> + event->path.mnt = path->mnt;
> + path_get(&event->path);
> + event->data_type = FSNOTIFY_EVENT_PATH;
> + break;
> + }
> + case FSNOTIFY_EVENT_PATH: {
> + struct path *path = data;
> + event->path.dentry = path->dentry;
> + event->path.mnt = path->mnt;
> + path_get(&event->path);
> + event->data_type = FSNOTIFY_EVENT_PATH;
> + break;
> + }
> + case FSNOTIFY_EVENT_INODE:
> + event->inode = data;
> + event->data_type = FSNOTIFY_EVENT_INODE;
> + break;
> + default:
> + BUG();
> + };

unneeded semicolon

> + event->mask = mask;
> +
> + return event;
> +}
> +
> +__init int fsnotify_notification_init(void)
> +{
> + event_kmem_cache = kmem_cache_create("fsnotify_event", sizeof(struct fsnotify_event), 0, SLAB_PANIC, NULL);

Can use the cheesy KMEM_CACHE() macro?

> + return 0;
> +}
> +subsys_initcall(fsnotify_notification_init);
> +
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 00fbd5b..3d68058 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -13,6 +13,7 @@
>
> #include <linux/dnotify.h>
> #include <linux/inotify.h>
> +#include <linux/fsnotify_backend.h>
> #include <linux/audit.h>
>
> /*
> @@ -35,6 +36,17 @@ static inline void fsnotify_d_move(struct dentry *entry)
> }
>
> /*
> + * fsnotify_inoderemove - an inode is going away
> + */
> +static inline void fsnotify_inoderemove(struct inode *inode)

inode_remove?

> +{
> + inotify_inode_queue_event(inode, IN_DELETE_SELF, 0, NULL, NULL);
> + inotify_inode_is_dead(inode);
> +
> + fsnotify(inode, FS_DELETE_SELF, inode, FSNOTIFY_EVENT_INODE);
> +}
> +
>
> ...
>
> +/*
> + * IN_* from inotfy.h lines up EXACTLY with FS_*, this is so we can easily
> + * convert between them. dnotify only needs conversion at watch creation
> + * so no perf loss there. fanotify isn't defined yet, so it can use the
> + * wholes if it needs more events.
> + */
> +#define FS_ACCESS 0x00000001ul /* File was accessed */
> +#define FS_MODIFY 0x00000002ul /* File was modified */
> +#define FS_ATTRIB 0x00000004ul /* Metadata changed */
> +#define FS_CLOSE_WRITE 0x00000008ul /* Writtable file was closed */
> +#define FS_CLOSE_NOWRITE 0x00000010ul /* Unwrittable file closed */
> +#define FS_OPEN 0x00000020ul /* File was opened */
> +#define FS_MOVED_FROM 0x00000040ul /* File was moved from X */
> +#define FS_MOVED_TO 0x00000080ul /* File was moved to Y */
> +#define FS_CREATE 0x00000100ul /* Subfile was created */
> +#define FS_DELETE 0x00000200ul /* Subfile was deleted */
> +#define FS_DELETE_SELF 0x00000400ul /* Self was deleted */
> +#define FS_MOVE_SELF 0x00000800ul /* Self was moved */
> +
> +#define FS_UNMOUNT 0x00002000ul /* inode on umount fs */
> +#define FS_Q_OVERFLOW 0x00004000ul /* Event queued overflowed */
> +#define FS_IN_IGNORED 0x00008000ul /* last inotify event here */
> +
> +#define FS_IN_ISDIR 0x40000000ul /* event occurred against dir */
> +#define FS_IN_ONESHOT 0x80000000ul /* only send event once */
> +
> +#define FS_DN_RENAME 0x10000000ul /* file renamed */
> +#define FS_DN_MULTISHOT 0x20000000ul /* dnotify multishot */
> +
> +#define FS_EVENT_ON_CHILD 0x08000000ul

All the "ul"s seem redundant?

> +struct fsnotify_group;
> +struct fsnotify_event;
> +
> +/*
> + * Each group much define these ops.
> + *
> + * handle_event - main call for a group to handle an fs event
> + * free_group_priv - called when a group refcnt hits 0 to clean up the private union
> + */
> +struct fsnotify_ops {
> + int (*handle_event)(struct fsnotify_group *group, struct fsnotify_event *event);
> + void (*free_group_priv)(struct fsnotify_group *group);

"free_group_private"

> +};
> +
> +/*
> + * A group is a "thing" that wants to receive notification about filesystem
> + * events. The mask holds the subset of event types this group cares about.

It's unclear what the "event types" are. FS_* from above?

Perhaps things would be clearer if they were named FS_EVENT_*, or FSE_*?

> + * refcnt on a group is up to the implementor and at any moment if it goes 0
> + * everything will be cleaned up.
> + */
> +struct fsnotify_group {
> + struct list_head group_list; /* list of all groups on the system */
> + __u32 mask; /* mask of events this group cares about */
> + atomic_t refcnt; /* num of processes with a special file open */
> + unsigned int group_num; /* the 'name' of the event */
> +
> + const struct fsnotify_ops *ops; /* how this group handles things */
> +
> + unsigned int evicted:1; /* has this group been evicted? */

If someone adds another bitfield here then they will share the same
word and will hence need locking. It'd be less risky to just make this
a plain old `unsigned'. Or `bool'.

> + /* groups can define private fields here */
> + union {
> + };
> +};
> +
> +/*
> + * all of the information about the original object we want to now send to
> + * a group. If you want to carry more info from the accessing task to the
> + * listener this structure is where you need to be adding fields.
> + */
> +struct fsnotify_event {
> + spinlock_t lock; /* protection for the associated event_holder and private_list */
> + struct inode *to_tell;

Does the existence of a `struct fsnotify_event' cause a reference to be
taken on fsnotify_event.to_tell?

If so, that's useful information to add here.

Either way, a few words about the design of the lifetime management
would be helpful.

>
> ...
>



\
 
 \ /
  Last update: 2009-04-08 01:17    [from the cache]
©2003-2011 Jasper Spaans