Re: [PATCH -V2 07/13] fsnotify: generic notification queue and waitq

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 27 Mar 2009 16:05:43 -0400
Eric Paris <eparis@redhat.com> wrote:

> inotify needs to do asyc notification in which event information is stored
> on a queue until the listener is ready to receive it.  This patch
> implements a generic notification queue for inotify (and later fanotify) to
> store events to be sent at a later time.
> 
>
> ...
>
> +/* return 1 if something is available, return 0 otherwise */
> +int fsnotify_check_notif_queue(struct fsnotify_group *group)
> +{
> +	BUG_ON(!mutex_is_locked(&group->notification_mutex));
> +	return !list_empty(&group->notification_list);
> +}

It's a poorly named function, because the name ("check") doesn't convey
information about the return value.

Better would be

	bool fsnotify_notif_queue_nonempty(...)

or

	bool fsnotify_notif_queue_empty(...)

(and invert test in callers)

	
and the abbreviation of "notify" to "notif" just makes it harder to
remember its name.

best is

	bool fsnotify_notify_queue_is_empty(...)

>  void fsnotify_get_event(struct fsnotify_event *event)
>  {
> @@ -45,26 +60,180 @@ void fsnotify_put_event(struct fsnotify_event *event)
>  		return;
>  
>  	if (atomic_dec_and_test(&event->refcnt)) {
> -		if (event->data_type == FSNOTIFY_EVENT_PATH) {
> +		if (event->data_type == FSNOTIFY_EVENT_PATH)
>  			path_put(&event->path);
> -			event->path.dentry = NULL;
> -			event->path.mnt = NULL;
> -		}
> +		kmem_cache_free(event_kmem_cache, event);
> +	}
> +}
>  
> -		event->mask = 0;
> +struct fsnotify_event_holder *alloc_event_holder(void)
> +{
> +	return kmem_cache_alloc(event_holder_kmem_cache, GFP_KERNEL);
> +}

That's a pretty generic-sounding name for a global symbol.

> -		kmem_cache_free(event_kmem_cache, event);
> +void fsnotify_destroy_event_holder(struct fsnotify_event_holder *holder)
> +{
> +	kmem_cache_free(event_holder_kmem_cache, holder);
> +}

That one's better.

> +/*
> + * check if 2 events contain the same information.
> + */
> +static inline int event_compare(struct fsnotify_event *old, struct fsnotify_event *new)
> +{
> +	if ((old->mask == new->mask) &&
> +	    (old->to_tell == new->to_tell) &&
> +	    (old->data_type == new->data_type)) {
> +		switch (old->data_type) {
> +		case (FSNOTIFY_EVENT_INODE):
> +			if (old->inode == new->inode)
> +				return 1;
> +			break;
> +		case (FSNOTIFY_EVENT_PATH):
> +			if ((old->path.mnt == new->path.mnt) &&
> +			    (old->path.dentry == new->path.dentry))
> +				return 1;
> +		case (FSNOTIFY_EVENT_NONE):
> +			return 1;
> +		};
>  	}
> +	return 0;
>  }

The compiler would have inlined this.

> -struct fsnotify_event *fsnotify_create_event(struct inode *to_tell, __u32 mask, void *data, int data_type)
> +/*
> + * Add an event to the group notification queue.  The group can later pull this
> + * event off the queue to deal with.
> + */
> +int fsnotify_add_notif_event(struct fsnotify_group *group, struct fsnotify_event *event)

s/notif/notify/

> +{
> +	struct fsnotify_event_holder *holder = NULL;
> +	struct list_head *list = &group->notification_list;
> +	struct fsnotify_event_holder *last_holder;
> +	struct fsnotify_event *last_event;
> +
> +	/*
> +	 * Check if we expect to be able to use the in event holder.  If not alloc
> +	 * a new holder.
> +	 * For the overflow event it's possible that something will use the in
> +	 * event holder before we get the lock so we may need to jump back and
> +	 * alloc a new holder.
> +	 */

The term "in event" is unclear to this reader.

> +	if (!list_empty(&event->holder.event_list)) {
> +alloc_holder:
> +		holder = alloc_event_holder();
> +		if (!holder)
> +			return -ENOMEM;
> +	}
> +
> +	mutex_lock(&group->notification_mutex);
> +
> +	if (group->q_len >= group->max_events)
> +		event = &q_overflow_event;
> +
> +	spin_lock(&event->lock);
> +
> +	if (list_empty(&event->holder.event_list)) {
> +		if (unlikely(holder))
> +			fsnotify_destroy_event_holder(holder);
> +		holder = &event->holder;
> +	} else if (unlikely(!holder)) {
> +		/* between the time we checked above and got the lock the in
> +		 * event holder was used, go back and get a new one */
> +		spin_unlock(&event->lock);
> +		mutex_unlock(&group->notification_mutex);
> +		goto alloc_holder;
> +	}
> +
> +	if (!list_empty(list)) {
> +		last_holder = list_entry(list->prev, struct fsnotify_event_holder, event_list);
> +		last_event = last_holder->event;
> +		if (event_compare(last_event, event)) {
> +			spin_unlock(&event->lock);
> +			mutex_unlock(&group->notification_mutex);
> +			if (holder != &event->holder)
> +				fsnotify_destroy_event_holder(holder);
> +			return 0;
> +		}
> +	}
> +
> +	group->q_len++;
> +	holder->event = event;
> +
> +	fsnotify_get_event(event);
> +	list_add_tail(&holder->event_list, list);
> +	spin_unlock(&event->lock);
> +	mutex_unlock(&group->notification_mutex);
> +
> +	wake_up(&group->notification_waitq);
> +	return 0;
> +}
> +
>
> ...
>