On Wed 15-03-17 21:19:13, Amir Goldstein wrote:
> On Wed, Mar 15, 2017 at 12:46 PM, Jan Kara <jack@xxxxxxx> wrote:
> > +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.
Agreed and changed.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
