On Wed, Mar 15, 2017 at 12:46 PM, Jan Kara <jack@xxxxxxx> wrote:
> Adding notification mark to object list has been currently done through
> fsnotify_add_{inode|vfsmount}_mark() helpers from
> fsnotify_add_mark_locked() which call fsnotify_add_mark_list(). Remove
> this unnecessary indirection to simplify the code.
>
> Pushing all the locking to fsnotify_add_mark_list() also allows us to
> allocate the connector structure with GFP_KERNEL mode.
>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
[...]
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 6a1c305038ea..12310d140d58 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -324,17 +324,30 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
> * 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.
> */
> -int fsnotify_add_mark_list(struct fsnotify_mark_connector **connp,
> - struct fsnotify_mark *mark, struct inode *inode,
> - struct vfsmount *mnt, int allow_dups)
> +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;
> + spinlock_t *lock;
> int cmp;
> + int err = 0;
> +
> + if (WARN_ON(!inode && !mnt))
> + return -EINVAL;
> + if (inode) {
> + connp = &inode->i_fsnotify_marks;
> + lock = &inode->i_lock;
> + } else {
> + connp = &real_mount(mnt)->mnt_fsnotify_marks;
> + lock = &mnt->mnt_root->d_lock;
> + }
>
> if (!*connp) {
> conn = kmem_cache_alloc(fsnotify_mark_connector_cachep,
> - GFP_ATOMIC);
> + GFP_KERNEL);
> if (!conn)
> return -ENOMEM;
> INIT_HLIST_HEAD(&conn->list);
> @@ -350,8 +363,17 @@ int fsnotify_add_mark_list(struct fsnotify_mark_connector **connp,
> * lockless_dereference() in fsnotify().
> */
> smp_wmb();
> - *connp = conn;
> + spin_lock(&mark->lock);
> + spin_lock(lock);
> + if (!*connp) {
> + *connp = conn;
> + } else {
> + kmem_cache_free(fsnotify_mark_connector_cachep, conn);
> + conn = *connp;
> + }
> } else {
> + spin_lock(&mark->lock);
> + spin_lock(lock);
> conn = *connp;
> }
>
This chunk has become overly convoluted IMO.
How about something like this:
conn = fsnotify_connector_get(connp, lock);
spin_lock(&mark->lock);
spin_lock(lock);
Where fsnotify_connector_get() acquires and releases only the object lock
and only for the case of allocating a new connector.
