* [PATCH] fsnotify: Do not always merge overflow events @ 2010-11-26 11:39 Lino Sanfilippo 2011-01-19 20:37 ` Eric Paris 0 siblings, 1 reply; 3+ messages in thread From: Lino Sanfilippo @ 2010-11-26 11:39 UTC (permalink / raw) To: eparis; +Cc: linux-kernel, linux-fsdevel Currently we always merge an overflow event if such an event already exists somewhere in the notification queue. This makes it hard to determine how often the limit of the queue has actually been exceeded. But this information could be useful as a hint that the queue is overloaded permanently. With this patch we only merge an overflow event if the last event in the queue is also an overflow event. An example explains the new behaviour (PU = another event is generated and pushed into the event queue, PO = an event is read by userspace and popped from the queue, E = Event, O = Overflow event) for a queue with a max number of queued events of 4 whereby 3 events are already queued. 1. E E E PU 2. E E E E PU -> queue full, generate overflow event 3. E E E E O PU -> queue full, last event already overflow event, so do nothing 4. E E E E O PO 5. E E E O PU -> queue full, last event already overflow event, so do nothing 6. E E E O PO -> queue ready to take events again 7. E E O PU 8. E E O E PU -> queue full, generate overflow event 9. E E O E O Now a listener could see that the queue has been overflowed 2 times. With the recent implementation it would only know that the queue has been overflowed at least one time. Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> --- fs/notify/notification.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-) This patch applies against patch "Dont try to open a file descriptor for the overflow event" that was sent to lkml on Nov 24. diff --git a/fs/notify/notification.c b/fs/notify/notification.c index f39260f..e82dabc 100644 --- a/fs/notify/notification.c +++ b/fs/notify/notification.c @@ -165,21 +165,30 @@ alloc_holder: mutex_lock(&group->notification_mutex); - if (group->q_len >= group->max_events) { - event = q_overflow_event; + if (group->q_len >= group->max_events) { /* overflow */ + struct fsnotify_event_holder *last; + + BUG_ON(list_empty(list)); /* * we need to return the overflow event * which means we need a ref */ + event = q_overflow_event; fsnotify_get_event(event); + last = list_entry(list->prev, struct fsnotify_event_holder, + event_list); + if (last->event == q_overflow_event) { + mutex_unlock(&group->notification_mutex); + if (holder) + fsnotify_destroy_event_holder(holder); + return q_overflow_event; + } return_event = event; /* sorry, no private data on the overflow event */ priv = NULL; - } - - if (!list_empty(list) && merge) { + } else if (!list_empty(list) && merge) { struct fsnotify_event *tmp; tmp = merge(list, event); -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fsnotify: Do not always merge overflow events 2010-11-26 11:39 [PATCH] fsnotify: Do not always merge overflow events Lino Sanfilippo @ 2011-01-19 20:37 ` Eric Paris 2011-01-21 14:41 ` Lino Sanfilippo 0 siblings, 1 reply; 3+ messages in thread From: Eric Paris @ 2011-01-19 20:37 UTC (permalink / raw) To: Lino Sanfilippo; +Cc: linux-kernel, linux-fsdevel On Fri, 2010-11-26 at 12:39 +0100, Lino Sanfilippo wrote: > With this patch we only merge an overflow event if the last event in the > queue is also an overflow event. I liked the idea, but not the patch. It left some dead return_event code, but more importantly I'd rather keep the generic code generic if we could. I merged this patch instead which pushes the handling out to fanotify_merge, so if some listener wants other complex behavior they can handle it. What do you think? >From 8c33f8e252ac94bf4714fa3133163857463ab9ea Mon Sep 17 00:00:00 2001 From: Eric Paris <eparis@redhat.com> Date: Wed, 19 Jan 2011 15:25:53 -0500 Subject: [PATCH] fanotify: Do not always merge overflow events Currently we always merge an overflow event if such an event already exists somewhere in the notification queue. This makes it hard to determine how often the limit of the queue has actually been exceeded. But this information could be useful as a hint that the queue is overloaded permanently. With this patch we only merge an overflow event if the last event in the queue is also an overflow event. An example explains the new behaviour (PU = another event is generated and pushed into the event queue, PO = an event is read by userspace and popped from the queue, E = Event, O = Overflow event) for a queue with a max number of queued events of 4 whereby 3 events are already queued. 1. E E E PU 2. E E E E PU -> queue full, generate overflow event 3. E E E E O PU -> queue full, last event already overflow event, so do nothing 4. E E E E O PO 5. E E E O PU -> queue full, last event already overflow event, so do nothing 6. E E E O PO -> queue ready to take events again 7. E E O PU 8. E E O E PU -> queue full, generate overflow event 9. E E O E O Now a listener could see that the queue has been overflowed 2 times. With the recent implementation it would only know that the queue has been overflowed at least one time. Based-on-patch-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> Signed-off-by: Eric Paris <eparis@redhat.com> --- fs/notify/fanotify/fanotify.c | 15 +++++++++++++++ fs/notify/notification.c | 2 +- include/linux/fsnotify_backend.h | 2 ++ 3 files changed, 18 insertions(+), 1 deletions(-) diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index c2ba86a..0adb80f 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -30,6 +30,19 @@ static bool should_merge(struct fsnotify_event *old, struct fsnotify_event *new) return false; } +/* special merging for q_overflow, only merge with the last event */ +static struct fsnotify_event *fanotify_merge_q_overflow(struct list_head *list) +{ + struct fsnotify_event_holder *last; + + last = list_entry(list->prev, struct fsnotify_event_holder, event_list); + if (last->event == q_overflow_event) { + fsnotify_get_event(q_overflow_event); + return q_overflow_event; + } + return NULL; +} + /* and the list better be locked by something too! */ static struct fsnotify_event *fanotify_merge(struct list_head *list, struct fsnotify_event *event) @@ -40,6 +53,8 @@ static struct fsnotify_event *fanotify_merge(struct list_head *list, pr_debug("%s: list=%p event=%p\n", __func__, list, event); + if (event == q_overflow_event) + return fanotify_merge_q_overflow(list); list_for_each_entry_reverse(test_holder, list, event_list) { if (should_merge(test_holder->event, event)) { diff --git a/fs/notify/notification.c b/fs/notify/notification.c index f39260f..e69573e 100644 --- a/fs/notify/notification.c +++ b/fs/notify/notification.c @@ -56,7 +56,7 @@ static struct kmem_cache *fsnotify_event_holder_cachep; * it is needed. It's refcnt is set 1 at kernel init time and will never * get set to 0 so it will never get 'freed' */ -static struct fsnotify_event *q_overflow_event; +struct fsnotify_event *q_overflow_event; static atomic_t fsnotify_sync_cookie = ATOMIC_INIT(0); /** diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index 6a3c660..dbcbcae 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -364,6 +364,8 @@ extern struct fsnotify_group *fsnotify_alloc_group(const struct fsnotify_ops *op /* drop reference on a group from fsnotify_alloc_group */ extern void fsnotify_put_group(struct fsnotify_group *group); +/* special event used to indicate the queue is full */ +extern struct fsnotify_event *q_overflow_event; /* take a reference to an event */ extern void fsnotify_get_event(struct fsnotify_event *event); extern void fsnotify_put_event(struct fsnotify_event *event); -- 1.7.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fsnotify: Do not always merge overflow events 2011-01-19 20:37 ` Eric Paris @ 2011-01-21 14:41 ` Lino Sanfilippo 0 siblings, 0 replies; 3+ messages in thread From: Lino Sanfilippo @ 2011-01-21 14:41 UTC (permalink / raw) To: Eric Paris; +Cc: linux-kernel, linux-fsdevel On Wed, Jan 19, 2011 at 03:37:01PM -0500, Eric Paris wrote: > On Fri, 2010-11-26 at 12:39 +0100, Lino Sanfilippo wrote: > > > With this patch we only merge an overflow event if the last event in the > > queue is also an overflow event. > > I liked the idea, but not the patch. It left some dead return_event > code, but more importantly I'd rather keep the generic code generic if > we could. I merged this patch instead which pushes the handling out to > fanotify_merge, so if some listener wants other complex behavior they > can handle it. > > What do you think? I think keeping it generic and up to the listener is a very good idea. But what we also have to consider then is how overflow events are handled if _no_ merge() function is defined by a listener: Right now, since o-events are not merged in that case, a new o-event is put in the queue everytime the queue is found full, possibly increasing the queue more and more. So we could end up with a queue that is permanently overflowed only because its filled with countless overflow events :|. I dont think that this is a good default behaviour. My suggestion is to do both, keep generic and offer a default merge() function - which does nothing but merge overflow events in a sane way. So listeners could decide whether they want to keep the default behaviour, reuse it in their own implementation or completely replace it with their own implementation. In the example below the implementation from fanotify_merge_q_overflow() is used as a generic (default) merge() function: diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index c2ba86a..87925c0 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -40,6 +40,8 @@ static struct fsnotify_event *fanotify_merge(struct list_head *list, pr_debug("%s: list=%p event=%p\n", __func__, list, event); + if (event == q_overflow_event) + return fsnotify_generic_merge(list, event); list_for_each_entry_reverse(test_holder, list, event_list) { if (should_merge(test_holder->event, event)) { diff --git a/fs/notify/notification.c b/fs/notify/notification.c index f39260f..8e66252 100644 --- a/fs/notify/notification.c +++ b/fs/notify/notification.c @@ -56,7 +56,7 @@ static struct kmem_cache *fsnotify_event_holder_cachep; * it is needed. It's refcnt is set 1 at kernel init time and will never * get set to 0 so it will never get 'freed' */ -static struct fsnotify_event *q_overflow_event; +struct fsnotify_event *q_overflow_event; static atomic_t fsnotify_sync_cookie = ATOMIC_INIT(0); /** @@ -132,6 +132,22 @@ struct fsnotify_event_private_data *fsnotify_remove_priv_from_event(struct fsnot return priv; } +/* generic merging for q_overflow, only merge with the last event */ +struct fsnotify_event *fsnotify_generic_merge(struct list_head *list, + struct fsnotify_event *event) +{ + struct fsnotify_event_holder *last; + + if (event != q_overflow_event) + return NULL; + last = list_entry(list->prev, struct fsnotify_event_holder, event_list); + if (last->event == q_overflow_event) { + fsnotify_get_event(q_overflow_event); + return q_overflow_event; + } + return NULL; +} + /* * Add an event to the group notification queue. The group can later pull this * event off the queue to deal with. If the event is successfully added to the @@ -145,9 +161,14 @@ struct fsnotify_event *fsnotify_add_notify_event(struct fsnotify_group *group, s struct fsnotify_event *return_event = NULL; struct fsnotify_event_holder *holder = NULL; struct list_head *list = &group->notification_list; + struct fsnotify_event *(* merge_event) (struct list_head *, + struct fsnotify_event *) + = fsnotify_generic_merge; pr_debug("%s: group=%p event=%p priv=%p\n", __func__, group, event, priv); + if (merge) /* use merge() function provided by listener */ + merge_event = merge; /* * There is one fsnotify_event_holder embedded inside each fsnotify_event. * Check if we expect to be able to use that holder. If not alloc a new @@ -179,10 +200,10 @@ alloc_holder: priv = NULL; } - if (!list_empty(list) && merge) { + if (!list_empty(list)) { struct fsnotify_event *tmp; - tmp = merge(list, event); + tmp = merge_event(list, event); if (tmp) { mutex_unlock(&group->notification_mutex); @@ -210,7 +231,6 @@ alloc_holder: fsnotify_put_event(return_event); return_event = NULL; } - goto alloc_holder; } diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index 6a3c660..e3d1b78 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -364,6 +364,11 @@ extern struct fsnotify_group *fsnotify_alloc_group(const struct fsnotify_ops *op /* drop reference on a group from fsnotify_alloc_group */ extern void fsnotify_put_group(struct fsnotify_group *group); +/* special event used to indicate the queue is full */ +extern struct fsnotify_event *q_overflow_event; +/* generic merging */ +extern struct fsnotify_event *fsnotify_generic_merge(struct list_head *list, + struct fsnotify_event *event); /* take a reference to an event */ extern void fsnotify_get_event(struct fsnotify_event *event); extern void fsnotify_put_event(struct fsnotify_event *event); ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-01-21 14:41 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-11-26 11:39 [PATCH] fsnotify: Do not always merge overflow events Lino Sanfilippo 2011-01-19 20:37 ` Eric Paris 2011-01-21 14:41 ` Lino Sanfilippo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).