On Tue, Mar 28, 2017 at 06:13:13PM +0200, Jan Kara wrote:
> fsnotify_detach_mark() calls fsnotify_destroy_inode_mark() or
> fsnotify_destroy_vfsmount_mark() to remove mark from object list. These
> two functions are however very similar and differ only in the lock they
> use to protect the object list of marks. Simplify the code by removing
> the indirection and removing mark from the object list in a common
> function.
>
> Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
> fs/notify/fsnotify.h | 4 ----
> fs/notify/inode_mark.c | 21 ---------------------
> fs/notify/mark.c | 33 +++++++++++++++++++++++++++------
> fs/notify/vfsmount_mark.c | 18 ------------------
> 4 files changed, 27 insertions(+), 49 deletions(-)
>
> diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
> index 225924274f8a..510f027bdf0f 100644
> --- a/fs/notify/fsnotify.h
> +++ b/fs/notify/fsnotify.h
> @@ -18,10 +18,6 @@ extern struct srcu_struct fsnotify_mark_srcu;
> extern int fsnotify_compare_groups(struct fsnotify_group *a,
> struct fsnotify_group *b);
>
> -/* vfsmount specific destruction of a mark */
> -extern void fsnotify_destroy_vfsmount_mark(struct fsnotify_mark *mark);
> -/* inode specific destruction of a mark */
> -extern struct inode *fsnotify_destroy_inode_mark(struct fsnotify_mark *mark);
> /* Find mark belonging to given group in the list of marks */
> extern struct fsnotify_mark *fsnotify_find_mark(
> struct fsnotify_mark_connector *conn,
> diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c
> index f05fc49b8242..080b6d8b9973 100644
> --- a/fs/notify/inode_mark.c
> +++ b/fs/notify/inode_mark.c
> @@ -35,27 +35,6 @@ void fsnotify_recalc_inode_mask(struct inode *inode)
> fsnotify_recalc_mask(inode->i_fsnotify_marks);
> }
>
> -struct inode *fsnotify_destroy_inode_mark(struct fsnotify_mark *mark)
> -{
> - struct inode *inode = mark->connector->inode;
> - bool empty;
> -
> - BUG_ON(!mutex_is_locked(&mark->group->mark_mutex));
> - assert_spin_locked(&mark->lock);
> -
> - spin_lock(&inode->i_lock);
> -
> - hlist_del_init_rcu(&mark->obj_list);
> - empty = hlist_empty(&mark->connector->list);
> - mark->connector = NULL;
> -
> - spin_unlock(&inode->i_lock);
> -
> - fsnotify_recalc_mask(inode->i_fsnotify_marks);
> -
> - return empty ? inode : NULL;
> -}
> -
> /*
> * Given a group clear all of the inode marks associated with that group.
> */
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index f32ca924c44e..509bb17f1a7e 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -141,6 +141,31 @@ void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
> }
> }
>
> +static struct inode *fsnotify_detach_from_object(struct fsnotify_mark *mark)
> +{
> + struct fsnotify_mark_connector *conn;
> + struct inode *inode = NULL;
> + spinlock_t *lock;
> +
> + conn = mark->connector;
> + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE)
> + lock = &conn->inode->i_lock;
> + else
> + lock = &conn->mnt->mnt_root->d_lock;
> + spin_lock(lock);
> + hlist_del_init_rcu(&mark->obj_list);
> + if (hlist_empty(&conn->list)) {
> + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE)
> + inode = conn->inode;
> + } else {
> + __fsnotify_recalc_mask(conn);
Is the mask recalculation deliberately done only when the mark list is
non-empty? To me it doesn't look like a worthwhile optimization even if the
mask value does not matter in that case.
Also now __fsnotify_update_child_dentry_flags() is missing. Even if it's not a
bug, it should be explained in the changelog.
> + }
> + mark->connector = NULL;
> + spin_unlock(lock);
> +
> + return inode;
> +}
> +
> /*
> * Remove mark from inode / vfsmount list, group list, drop inode reference
> * if we got one.
> @@ -164,12 +189,8 @@ void fsnotify_detach_mark(struct fsnotify_mark *mark)
>
> mark->flags &= ~FSNOTIFY_MARK_FLAG_ATTACHED;
>
> - if (mark->connector->flags & FSNOTIFY_OBJ_TYPE_INODE)
> - inode = fsnotify_destroy_inode_mark(mark);
> - else if (mark->connector->flags & FSNOTIFY_OBJ_TYPE_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
> diff --git a/fs/notify/vfsmount_mark.c b/fs/notify/vfsmount_mark.c
> index 3476ee44b2c5..26da5c209944 100644
> --- a/fs/notify/vfsmount_mark.c
> +++ b/fs/notify/vfsmount_mark.c
> @@ -39,24 +39,6 @@ void fsnotify_recalc_vfsmount_mask(struct vfsmount *mnt)
> fsnotify_recalc_mask(real_mount(mnt)->mnt_fsnotify_marks);
> }
>
> -void fsnotify_destroy_vfsmount_mark(struct fsnotify_mark *mark)
> -{
> - struct vfsmount *mnt = mark->connector->mnt;
> - struct mount *m = real_mount(mnt);
> -
> - BUG_ON(!mutex_is_locked(&mark->group->mark_mutex));
> - assert_spin_locked(&mark->lock);
> -
> - spin_lock(&mnt->mnt_root->d_lock);
> -
> - hlist_del_init_rcu(&mark->obj_list);
> - mark->connector = NULL;
> -
> - spin_unlock(&mnt->mnt_root->d_lock);
> -
> - fsnotify_recalc_mask(m->mnt_fsnotify_marks);
> -}
> -
> /*
> * given a group and vfsmount, find the mark associated with that combination.
> * if found take a reference to that mark and return it, else return NULL
> --
> 2.10.2
>
