[PATCH 03/10] fsnotify: use reference counting for groups
Stefan Bader
stefan.bader at canonical.com
Mon Apr 16 18:30:55 UTC 2012
On 16.04.2012 20:02, Andy Whitcroft wrote:
> From: Lino Sanfilippo <LinoSanfilippo at gmx.de>
>
> Get a group ref for each mark that is added to the groups list and release that
> ref when the mark is freed in fsnotify_put_mark().
> We also use get a group reference for duplicated marks and for private event
> data.
> Now we dont free a group any more when the number of marks becomes 0 but when
> the groups ref count does. Since this will only happen when all marks are removed
> from a groups mark list, we dont have to set the groups number of marks to 1 at
> group creation.
>
> Beside clearing all marks in fsnotify_destroy_group() we do also flush the
> groups event queue. This is since events may hold references to groups (due to
> private event data) and we have to put those references first before we get a
> chance to put the final ref, which will result in a call to
> fsnotify_final_destroy_group().
>
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo at gmx.de>
> Signed-off-by: Eric Paris <eparis at redhat.com>
>
> (cherry-picked from commit 0f948ec4aff7eb629b08e7dd3d9761be17a1ae9e git://git.infradead.org/users/eparis/notify.git)
> BugLink: http://bugs.launchpad.net/bugs/922906
> Signed-off-by: Andy Whitcroft <apw at canonical.com>
> ---
> fs/notify/group.c | 28 ++++++++++------------------
> fs/notify/inotify/inotify_fsnotify.c | 2 ++
> fs/notify/inotify/inotify_user.c | 1 +
> fs/notify/mark.c | 24 ++++++++++++++----------
> 4 files changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/fs/notify/group.c b/fs/notify/group.c
> index 59a6b53..37569ec 100644
> --- a/fs/notify/group.c
> +++ b/fs/notify/group.c
> @@ -34,9 +34,6 @@
> */
> void fsnotify_final_destroy_group(struct fsnotify_group *group)
> {
> - /* clear the notification queue of all events */
> - fsnotify_flush_notify(group);
> -
> if (group->ops->free_group_priv)
> group->ops->free_group_priv(group);
>
> @@ -44,12 +41,10 @@ void fsnotify_final_destroy_group(struct fsnotify_group *group)
> }
>
> /*
> - * Trying to get rid of a group. We need to first get rid of any outstanding
> - * allocations and then free the group. Remember that fsnotify_clear_marks_by_group
> - * could miss marks that are being freed by inode and those marks could still
> - * hold a reference to this group (via group->num_marks) If we get into that
> - * situtation, the fsnotify_final_destroy_group will get called when that final
> - * mark is freed.
> + * Trying to get rid of a group. Remove all marks, flush all events and release
> + * the group reference.
> + * Note that another thread calling fsnotify_clear_marks_by_group() may still
> + * hold a ref to the group.
> */
> void fsnotify_destroy_group(struct fsnotify_group *group)
> {
> @@ -58,9 +53,10 @@ void fsnotify_destroy_group(struct fsnotify_group *group)
>
> synchronize_srcu(&fsnotify_mark_srcu);
>
> - /* past the point of no return, matches the initial value of 1 */
> - if (atomic_dec_and_test(&group->num_marks))
> - fsnotify_final_destroy_group(group);
> + /* clear the notification queue of all events */
> + fsnotify_flush_notify(group);
> +
> + fsnotify_put_group(group);
> }
>
> /*
> @@ -77,7 +73,7 @@ void fsnotify_get_group(struct fsnotify_group *group)
> void fsnotify_put_group(struct fsnotify_group *group)
> {
> if (atomic_dec_and_test(&group->refcnt))
> - fsnotify_destroy_group(group);
> + fsnotify_final_destroy_group(group);
> }
> EXPORT_SYMBOL(fsnotify_put_group);
>
> @@ -94,11 +90,7 @@ struct fsnotify_group *fsnotify_alloc_group(const struct fsnotify_ops *ops)
>
> /* set to 0 when there a no external references to this group */
> atomic_set(&group->refcnt, 1);
> - /*
> - * hits 0 when there are no external references AND no marks for
> - * this group
> - */
> - atomic_set(&group->num_marks, 1);
> + atomic_set(&group->num_marks, 0);
>
> mutex_init(&group->notification_mutex);
> INIT_LIST_HEAD(&group->notification_list);
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index e3cbd74..74977fb 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -118,6 +118,7 @@ static int inotify_handle_event(struct fsnotify_group *group,
>
> fsn_event_priv = &event_priv->fsnotify_event_priv_data;
>
> + fsnotify_get_group(group);
> fsn_event_priv->group = group;
> event_priv->wd = wd;
>
> @@ -210,6 +211,7 @@ void inotify_free_event_priv(struct fsnotify_event_private_data *fsn_event_priv)
> event_priv = container_of(fsn_event_priv, struct inotify_event_private_data,
> fsnotify_event_priv_data);
>
> + fsnotify_put_group(fsn_event_priv->group);
> kmem_cache_free(event_priv_cachep, event_priv);
> }
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index dbafbfc..246250f 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -531,6 +531,7 @@ void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
>
> fsn_event_priv = &event_priv->fsnotify_event_priv_data;
>
> + fsnotify_get_group(group);
> fsn_event_priv->group = group;
> event_priv->wd = i_mark->wd;
>
Want to check with the applied code tomorrow... Just looks a bit curious without
a matching put...
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 54f36db..5b784aa 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -109,8 +109,11 @@ void fsnotify_get_mark(struct fsnotify_mark *mark)
>
> void fsnotify_put_mark(struct fsnotify_mark *mark)
> {
> - if (atomic_dec_and_test(&mark->refcnt))
> + if (atomic_dec_and_test(&mark->refcnt)) {
> + if (mark->group)
> + fsnotify_put_group(mark->group);
> mark->free_mark(mark);
> + }
> }
> EXPORT_SYMBOL(fsnotify_put_mark);
>
> @@ -126,12 +129,13 @@ void fsnotify_destroy_mark(struct fsnotify_mark *mark)
>
> spin_lock(&mark->lock);
>
> + fsnotify_get_group(mark->group);
> group = mark->group;
>
> /* something else already called this function on this mark */
> if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE)) {
> spin_unlock(&mark->lock);
> - return;
> + goto put_group;
> }
>
> mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE;
> @@ -178,19 +182,15 @@ void fsnotify_destroy_mark(struct fsnotify_mark *mark)
>
> if (inode && (mark->flags & FSNOTIFY_MARK_FLAG_OBJECT_PINNED))
> iput(inode);
> -
> /*
> * We don't necessarily have a ref on mark from caller so the above iput
> * may have already destroyed it. Don't touch from now on.
> */
>
> - /*
> - * it's possible that this group tried to destroy itself, but this
> - * this mark was simultaneously being freed by inode. If that's the
> - * case, we finish freeing the group here.
> - */
> - if (unlikely(atomic_dec_and_test(&group->num_marks)))
> - fsnotify_final_destroy_group(group);
> + atomic_dec(&group->num_marks);
> +
> +put_group:
> + fsnotify_put_group(group);
> }
> EXPORT_SYMBOL(fsnotify_destroy_mark);
>
> @@ -236,6 +236,7 @@ int fsnotify_add_mark(struct fsnotify_mark *mark,
>
> mark->flags |= FSNOTIFY_MARK_FLAG_ALIVE;
>
> + fsnotify_get_group(group);
> mark->group = group;
> list_add(&mark->g_list, &group->marks_list);
> atomic_inc(&group->num_marks);
> @@ -267,6 +268,7 @@ int fsnotify_add_mark(struct fsnotify_mark *mark,
> err:
> mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE;
> list_del_init(&mark->g_list);
> + fsnotify_put_group(group);
> mark->group = NULL;
> atomic_dec(&group->num_marks);
>
> @@ -320,6 +322,8 @@ void fsnotify_duplicate_mark(struct fsnotify_mark *new, struct fsnotify_mark *ol
> assert_spin_locked(&old->lock);
> new->i.inode = old->i.inode;
> new->m.mnt = old->m.mnt;
> + if (old->group)
> + fsnotify_get_group(old->group);
> new->group = old->group;
> new->mask = old->mask;
> new->free_mark = old->free_mark;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 900 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20120416/03f58223/attachment.sig>
More information about the kernel-team
mailing list