From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Paul Moore <paul@paul-moore.com>,
Miklos Szeredi <miklos@szeredi.hu>
Subject: Re: [PATCH 06/22] fsnotify: Attach marks to object via dedicated head structure
Date: Sun, 8 Jan 2017 12:18:10 +0200 [thread overview]
Message-ID: <CAOQ4uxgyLkGBR4EczWd7+GJPm0kOQ3w5a5BxALK2oiviCvSP5w@mail.gmail.com> (raw)
In-Reply-To: <20170106104401.5894-7-jack@suse.cz>
On Fri, Jan 6, 2017 at 12:43 PM, Jan Kara <jack@suse.cz> wrote:
> Currently notification marks are attached to object (inode or vfsmnt) by
> a hlist_head in the object. The list is also protected by a spinlock in
> the object. So while there is any mark attached to the list of marks,
> the object must be pinned in memory (and thus e.g. last iput() deleting
> inode cannot happen). Also for list iteration in fsnotify() to work, we
> must hold fsnotify_mark_srcu lock so that mark itself and
> mark->obj_list.next cannot get freed. Thus we are required to wait for
> response to fanotify events from userspace process with
> fsnotify_mark_srcu lock held. That causes issues when userspace process
> is buggy and does not reply to some event - basically the whole
> notification subsystem gets eventually stuck.
>
> So to be able to drop fsnotify_mark_srcu lock while waiting for
> response, we have to pin the mark in memory and make sure it stays in
> the object list (as removing the mark waiting for response could lead to
> lost notification events for groups later in the list). However we don't
> want inode reclaim to block on such mark as that would lead to system
> just locking up elsewhere.
>
> This commit tries to pave a way towards solving these conflicting
> lifetime needs. Instead of anchoring the list of marks directly in the
> object, we anchor it in a dedicated structure (fsnotify_mark_connector) and
> just point to that structure from the object. Also the list is protected
> by a spinlock contained in that structure. With this, we can detach
> notification marks from object without having to modify the list itself.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
Looks good, except for some confusing semantics of grab/put
and question about rcu_assign_pointer()
...
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index b41515d3f081..d512ef9f75fc 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -193,6 +193,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
> struct hlist_node *inode_node = NULL, *vfsmount_node = NULL;
> struct fsnotify_mark *inode_mark = NULL, *vfsmount_mark = NULL;
> struct fsnotify_group *inode_group, *vfsmount_group;
> + struct fsnotify_mark_connector *inode_conn, *vfsmount_conn;
> struct mount *mnt;
> int idx, ret = 0;
> /* global tests shouldn't care about events on child only the specific event */
> @@ -210,8 +211,8 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
> * SRCU because we have no references to any objects and do not
> * need SRCU to keep them "alive".
> */
> - if (hlist_empty(&to_tell->i_fsnotify_marks) &&
> - (!mnt || hlist_empty(&mnt->mnt_fsnotify_marks)))
> + if (!to_tell->i_fsnotify_marks &&
> + (!mnt || !mnt->mnt_fsnotify_marks))
> return 0;
> /*
> * if this is a modify event we may need to clear the ignored masks
> @@ -226,16 +227,27 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
> idx = srcu_read_lock(&fsnotify_mark_srcu);
>
> if ((mask & FS_MODIFY) ||
> - (test_mask & to_tell->i_fsnotify_mask))
> - inode_node = srcu_dereference(to_tell->i_fsnotify_marks.first,
> + (test_mask & to_tell->i_fsnotify_mask)) {
> + inode_conn = srcu_dereference(to_tell->i_fsnotify_marks,
> &fsnotify_mark_srcu);
> + if (inode_conn)
> + inode_node = srcu_dereference(inode_conn->list.first,
> + &fsnotify_mark_srcu);
> + }
>
> if (mnt && ((mask & FS_MODIFY) ||
> (test_mask & mnt->mnt_fsnotify_mask))) {
> - vfsmount_node = srcu_dereference(mnt->mnt_fsnotify_marks.first,
> - &fsnotify_mark_srcu);
> - inode_node = srcu_dereference(to_tell->i_fsnotify_marks.first,
> + inode_conn = srcu_dereference(to_tell->i_fsnotify_marks,
> &fsnotify_mark_srcu);
> + if (inode_conn)
> + inode_node = srcu_dereference(inode_conn->list.first,
> + &fsnotify_mark_srcu);
> + vfsmount_conn = srcu_dereference(mnt->mnt_fsnotify_marks,
> + &fsnotify_mark_srcu);
> + if (vfsmount_conn)
> + vfsmount_node = srcu_dereference(
> + vfsmount_conn->list.first,
> + &fsnotify_mark_srcu);
> }
>
...
> +
> +static struct inode *fsnotify_detach_from_object(struct fsnotify_mark *mark)
> +{
> + struct fsnotify_mark_connector *conn;
> + struct inode *inode = NULL;
> + bool free_conn = false;
> +
> + conn = mark->connector;
> + spin_lock(&conn->lock);
> + hlist_del_init_rcu(&mark->obj_list);
> + if (hlist_empty(&conn->list)) {
> + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) {
> + inode = conn->inode;
> + inode->i_fsnotify_marks = NULL;
Shouldn't that assignment be done using rcu_assign_pointer() to
match srcu_dereference(to_tell->i_fsnotify_marks, &fsnotify_mark_srcu); ?
> + inode->i_fsnotify_mask = 0;
> + conn->inode = NULL;
> + conn->flags &= ~FSNOTIFY_OBJ_TYPE_INODE;
> + } else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
> + real_mount(conn->mnt)->mnt_fsnotify_marks = NULL;
Same here
> + real_mount(conn->mnt)->mnt_fsnotify_mask = 0;
> + conn->mnt = NULL;
> + conn->flags &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> + }
> + free_conn = true;
> + } else
> + __fsnotify_recalc_mask(conn);
> + mark->connector = NULL;
> + spin_unlock(&conn->lock);
> +
> + if (free_conn) {
> + spin_lock(&destroy_lock);
> + conn->destroy_next = connector_destroy_list;
> + connector_destroy_list = conn;
> + spin_unlock(&destroy_lock);
> + queue_work(system_unbound_wq, &connector_reaper_work);
> + }
> +
> + return inode;
> }
>
> /*
> @@ -137,13 +227,8 @@ void fsnotify_detach_mark(struct fsnotify_mark *mark)
>
> mark->flags &= ~FSNOTIFY_MARK_FLAG_ATTACHED;
>
> - if (mark->flags & FSNOTIFY_MARK_FLAG_INODE) {
> - inode = mark->inode;
> - fsnotify_destroy_inode_mark(mark);
> - } else if (mark->flags & FSNOTIFY_MARK_FLAG_VFSMOUNT)
> - fsnotify_destroy_vfsmount_mark(mark);
> - else
> - BUG();
> + inode = fsnotify_detach_from_object(mark);
> +
> /*
> * Note that we didn't update flags telling whether inode cares about
> * what's happening with children. We update these flags from
> @@ -155,7 +240,7 @@ void fsnotify_detach_mark(struct fsnotify_mark *mark)
>
> spin_unlock(&mark->lock);
>
> - if (inode && (mark->flags & FSNOTIFY_MARK_FLAG_OBJECT_PINNED))
> + if (inode)
> iput(inode);
>
> atomic_dec(&group->num_marks);
> @@ -220,45 +305,11 @@ void fsnotify_destroy_mark(struct fsnotify_mark *mark,
> fsnotify_free_mark(mark);
> }
>
> -void fsnotify_destroy_marks(struct hlist_head *head, spinlock_t *lock)
> -{
> - struct fsnotify_mark *mark;
> -
> - while (1) {
> - /*
> - * We have to be careful since we can race with e.g.
> - * fsnotify_clear_marks_by_group() and once we drop 'lock',
> - * mark can get removed from the obj_list and destroyed. But
> - * we are holding mark reference so mark cannot be freed and
> - * calling fsnotify_destroy_mark() more than once is fine.
> - */
> - spin_lock(lock);
> - if (hlist_empty(head)) {
> - spin_unlock(lock);
> - break;
> - }
> - mark = hlist_entry(head->first, struct fsnotify_mark, obj_list);
> - /*
> - * We don't update i_fsnotify_mask / mnt_fsnotify_mask here
> - * since inode / mount is going away anyway. So just remove
> - * mark from the list.
> - */
> - hlist_del_init_rcu(&mark->obj_list);
> - fsnotify_get_mark(mark);
> - spin_unlock(lock);
> - fsnotify_destroy_mark(mark, mark->group);
> - fsnotify_put_mark(mark);
> - }
> -}
> -
> void fsnotify_set_mark_mask_locked(struct fsnotify_mark *mark, __u32 mask)
> {
> assert_spin_locked(&mark->lock);
>
> mark->mask = mask;
> -
> - if (mark->flags & FSNOTIFY_MARK_FLAG_INODE)
> - fsnotify_set_inode_mark_mask_locked(mark, mask);
> }
>
> void fsnotify_set_mark_ignored_mask_locked(struct fsnotify_mark *mark, __u32 mask)
> @@ -304,37 +355,118 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
> return -1;
> }
>
> -/* Add mark into proper place in given list of marks */
> -int fsnotify_add_mark_list(struct hlist_head *head, struct fsnotify_mark *mark,
> - int allow_dups)
> +/*
> + * Get mark connector, make sure it is alive and return with its lock held.
> + * This is for users that get connector pointer from inode or mount. Users that
> + * hold reference to a mark on the list may directly lock connector->lock as
> + * they are sure list cannot go away under them.
> + */
> +static struct fsnotify_mark_connector *fsnotify_grab_connector(
> + struct fsnotify_mark_connector * __rcu *connp)
> +{
> + struct fsnotify_mark_connector *conn;
> + int idx;
> +
> + idx = srcu_read_lock(&fsnotify_mark_srcu);
> + conn = srcu_dereference(*connp, &fsnotify_mark_srcu);
> + if (!conn)
> + goto out;
> + spin_lock(&conn->lock);
> + if (!(conn->flags & (FSNOTIFY_OBJ_TYPE_INODE |
> + FSNOTIFY_OBJ_TYPE_VFSMOUNT))) {
> + spin_unlock(&conn->lock);
> + srcu_read_unlock(&fsnotify_mark_srcu, idx);
> + return NULL;
> + }
> +out:
> + srcu_read_unlock(&fsnotify_mark_srcu, idx);
> + return conn;
> +}
> +
> +static void fsnotify_put_connector(struct fsnotify_mark_connector *conn)
> +{
> + spin_unlock(&conn->lock);
> +}
> +
Is there a precedent in the kernel for using grab()/put() semantics for
what is essentially aquire()/release() for exclusive object access?
The counterpart for grab_cache_page(), for example, is
unlock_page(page); put_page(page); (and not just put_page()).
igrab() does not return with i_lock held, so igrab()/iput() sematics
do not apply to this case.
I find a function that is called put_connector() and actually does unlock
to be confusing and if that function is a one liner helper, I prefer that
it did not exist at all and call site should use spin_unlock(&conn->lock);
explicitly instead of hiding it.
> +/*
> + * Add mark into proper place in given list of marks. These marks may be used
> + * for the fsnotify backend to determine which event types should be delivered
> + * to which group and for which inodes. These marks are ordered according to
> + * priority, highest number first, and then by the group's location in memory.
> + */
> +static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
> + struct inode *inode, struct vfsmount *mnt,
> + int allow_dups)
> {
> struct fsnotify_mark *lmark, *last = NULL;
> + struct fsnotify_mark_connector *conn;
> + struct fsnotify_mark_connector **connp;
> int cmp;
> + int err = 0;
> +
> + BUG_ON(!inode && !mnt);
> + if (inode)
> + connp = &inode->i_fsnotify_marks;
> + else
> + connp = &real_mount(mnt)->mnt_fsnotify_marks;
> +restart:
> + spin_lock(&mark->lock);
> + conn = fsnotify_grab_connector(connp);
> + if (!conn) {
> + spin_unlock(&mark->lock);
> + conn = kmem_cache_alloc(fsnotify_mark_connector_cachep,
> + GFP_KERNEL);
> + if (!conn)
> + return -ENOMEM;
> + spin_lock_init(&conn->lock);
> + INIT_HLIST_HEAD(&conn->list);
> + if (inode) {
> + conn->flags = FSNOTIFY_OBJ_TYPE_INODE;
> + conn->inode = igrab(inode);
> + } else {
> + conn->flags = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> + conn->mnt = mnt;
> + }
> + if (cmpxchg(connp, NULL, conn)) {
Same question as in fsnotify_detach_from_object()
Is there a need for any smb_wmb() here to match
srcu_dereference() in fsnotify(), or is there no need because
cmpxchg from NULL to a live object is always safe for the reader?
> + /* Someone else created list structure for us */
> + if (inode)
> + iput(inode);
> + kmem_cache_free(fsnotify_mark_connector_cachep, conn);
> + }
> + goto restart;
> + }
>
> /* is mark the first mark? */
> - if (hlist_empty(head)) {
> - hlist_add_head_rcu(&mark->obj_list, head);
> - return 0;
> + if (hlist_empty(&conn->list)) {
> + hlist_add_head_rcu(&mark->obj_list, &conn->list);
> + goto added;
> }
>
> /* should mark be in the middle of the current list? */
> - hlist_for_each_entry(lmark, head, obj_list) {
> + hlist_for_each_entry(lmark, &conn->list, obj_list) {
> last = lmark;
>
> - if ((lmark->group == mark->group) && !allow_dups)
> - return -EEXIST;
> + if ((lmark->group == mark->group) && !allow_dups) {
> + err = -EEXIST;
> + goto out_err;
> + }
>
> cmp = fsnotify_compare_groups(lmark->group, mark->group);
> if (cmp >= 0) {
> hlist_add_before_rcu(&mark->obj_list, &lmark->obj_list);
> - return 0;
> + goto added;
> }
> }
>
> BUG_ON(last == NULL);
> /* mark should be the last entry. last is the current last entry */
> hlist_add_behind_rcu(&mark->obj_list, &last->obj_list);
> - return 0;
> +added:
> + mark->connector = conn;
> +out_err:
> + fsnotify_put_connector(conn);
> + spin_unlock(&mark->lock);
> + return err;
> }
>
> /*
> @@ -356,7 +488,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
> * LOCKING ORDER!!!!
> * group->mark_mutex
> * mark->lock
> - * inode->i_lock
> + * mark->connector->lock
> */
> spin_lock(&mark->lock);
> mark->flags |= FSNOTIFY_MARK_FLAG_ALIVE | FSNOTIFY_MARK_FLAG_ATTACHED;
> @@ -366,25 +498,14 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
> list_add(&mark->g_list, &group->marks_list);
> atomic_inc(&group->num_marks);
> fsnotify_get_mark(mark); /* for i_list and g_list */
> -
> - if (inode) {
> - ret = fsnotify_add_inode_mark(mark, group, inode, allow_dups);
> - if (ret)
> - goto err;
> - } else if (mnt) {
> - ret = fsnotify_add_vfsmount_mark(mark, group, mnt, allow_dups);
> - if (ret)
> - goto err;
> - } else {
> - BUG();
> - }
> -
> - /* this will pin the object if appropriate */
> - fsnotify_set_mark_mask_locked(mark, mark->mask);
> spin_unlock(&mark->lock);
>
> - if (inode)
> - __fsnotify_update_child_dentry_flags(inode);
> + ret = fsnotify_add_mark_list(mark, inode, mnt, allow_dups);
> + if (ret)
> + goto err;
> +
> + if (mark->mask)
> + fsnotify_recalc_mask(mark->connector);
>
> return ret;
> err:
> @@ -419,17 +540,23 @@ int fsnotify_add_mark(struct fsnotify_mark *mark, struct fsnotify_group *group,
> * Given a list of marks, find the mark associated with given group. If found
> * take a reference to that mark and return it, else return NULL.
> */
> -struct fsnotify_mark *fsnotify_find_mark(struct hlist_head *head,
> +struct fsnotify_mark *fsnotify_find_mark(struct fsnotify_mark_connector **connp,
> struct fsnotify_group *group)
> {
> struct fsnotify_mark *mark;
> + struct fsnotify_mark_connector *conn;
>
> - hlist_for_each_entry(mark, head, obj_list) {
> + conn = fsnotify_grab_connector(connp);
> + if (!conn)
> + return NULL;
> + hlist_for_each_entry(mark, &conn->list, obj_list) {
> if (mark->group == group) {
> fsnotify_get_mark(mark);
> + fsnotify_put_connector(conn);
> return mark;
> }
> }
> + fsnotify_put_connector(conn);
> return NULL;
> }
>
> @@ -453,7 +580,7 @@ void fsnotify_clear_marks_by_group_flags(struct fsnotify_group *group,
> */
> mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
> list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) {
> - if (mark->flags & flags)
> + if (mark->connector->flags & flags)
> list_move(&mark->g_list, &to_free);
> }
> mutex_unlock(&group->mark_mutex);
> @@ -499,6 +626,29 @@ void fsnotify_detach_group_marks(struct fsnotify_group *group)
> }
> }
>
> +/* Destroy all marks attached to inode / vfsmount */
> +void fsnotify_destroy_marks(struct fsnotify_mark_connector **connp)
> +{
> + struct fsnotify_mark_connector *conn;
> + struct fsnotify_mark *mark;
> +
> + while ((conn = fsnotify_grab_connector(connp))) {
> + /*
> + * We have to be careful since we can race with e.g.
> + * fsnotify_clear_marks_by_group() and once we drop the list
> + * lock, mark can get removed from the obj_list and destroyed.
> + * But we are holding mark reference so mark cannot be freed
> + * and calling fsnotify_destroy_mark() more than once is fine.
> + */
> + mark = hlist_entry(conn->list.first, struct fsnotify_mark,
> + obj_list);
> + fsnotify_get_mark(mark);
> + fsnotify_put_connector(conn);
> + fsnotify_destroy_mark(mark, mark->group);
> + fsnotify_put_mark(mark);
> + }
> +}
> +
next prev parent reply other threads:[~2017-01-08 10:18 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-06 10:43 [PATCH 0/22 v2] fsnotify: Avoid SRCU stalls with fanotify permission events Jan Kara
2017-01-06 10:43 ` [PATCH 01/22] fsnotify: Remove unnecessary tests when showing fdinfo Jan Kara
2017-01-06 10:43 ` [PATCH 02/22] inotify: Remove inode pointers from debug messages Jan Kara
2017-01-06 10:43 ` [PATCH 03/22] fanotify: Move recalculation of inode / vfsmount mask under mark_mutex Jan Kara
2017-01-06 10:43 ` [PATCH 04/22] audit: Abstract hash key handling Jan Kara
2017-01-06 10:43 ` [PATCH 05/22] fsnotify: Update comments Jan Kara
2017-01-06 10:43 ` [PATCH 06/22] fsnotify: Attach marks to object via dedicated head structure Jan Kara
2017-01-06 13:12 ` Amir Goldstein
2017-01-08 10:18 ` Amir Goldstein [this message]
2017-01-10 13:30 ` Jan Kara
2017-01-06 10:43 ` [PATCH 07/22] inotify: Do not drop mark reference under idr_lock Jan Kara
2017-01-06 10:43 ` [PATCH 08/22] fsnotify: Move queueing of mark for destruction into fsnotify_put_mark() Jan Kara
2017-01-08 11:15 ` Amir Goldstein
2017-01-06 10:43 ` [PATCH 09/22] fsnotify: Detach mark from object list when last reference is dropped Jan Kara
2017-01-08 12:10 ` Amir Goldstein
2017-01-10 13:47 ` Jan Kara
2017-01-06 10:43 ` [PATCH 10/22] fsnotify: Remove special handling of mark destruction on group shutdown Jan Kara
2017-01-06 10:43 ` [PATCH 11/22] fsnotify: Provide framework for dropping SRCU lock in ->handle_event Jan Kara
2017-01-08 9:02 ` Amir Goldstein
2017-01-10 13:00 ` Jan Kara
2017-01-06 10:43 ` [PATCH 12/22] fsnotify: Pass SRCU index into handle_event handler Jan Kara
2017-01-06 10:43 ` [PATCH 13/22] fanotify: Release SRCU lock when waiting for userspace response Jan Kara
2017-01-06 10:43 ` [PATCH 14/22] fsnotify: Remove fsnotify_set_mark_{,ignored_}mask_locked() Jan Kara
2017-01-06 10:43 ` [PATCH 15/22] fsnotify: Remove fsnotify_recalc_{inode|vfsmount}_mask() Jan Kara
2017-01-06 10:43 ` [PATCH 16/22] fsnotify: Inline fsnotify_clear_{inode|vfsmount}_mark_group() Jan Kara
2017-01-06 10:43 ` [PATCH 17/22] fsnotify: Rename fsnotify_clear_marks_by_group_flags() Jan Kara
2017-01-08 12:17 ` Amir Goldstein
2017-01-06 10:43 ` [PATCH 18/22] fsnotify: Remove fsnotify_detach_group_marks() Jan Kara
2017-01-08 12:26 ` Amir Goldstein
2017-01-06 10:43 ` [PATCH 19/22] fsnotify: Remove fsnotify_find_{inode|vfsmount}_mark() Jan Kara
2017-01-06 10:43 ` [PATCH 20/22] fsnotify: Drop inode_mark.c Jan Kara
2017-01-06 10:44 ` [PATCH 21/22] fsnotify: Add group pointer in fsnotify_init_mark() Jan Kara
2017-01-08 12:35 ` Amir Goldstein
2017-01-10 13:48 ` Jan Kara
2017-01-06 10:44 ` [PATCH 22/22] fsnotify: Move ->free_mark callback to fsnotify_ops Jan Kara
2017-01-08 12:50 ` [PATCH 0/22 v2] fsnotify: Avoid SRCU stalls with fanotify permission events Amir Goldstein
2017-01-20 13:21 [PATCH 0/22 v3] " Jan Kara
2017-01-20 13:21 ` [PATCH 06/22] fsnotify: Attach marks to object via dedicated head structure Jan Kara
2017-01-21 15:52 ` Amir Goldstein
2017-01-25 9:41 ` Miklos Szeredi
2017-01-31 15:41 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxgyLkGBR4EczWd7+GJPm0kOQ3w5a5BxALK2oiviCvSP5w@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=paul@paul-moore.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.