Re: [PATCH 06/22] fsnotify: Attach marks to object via dedicated head structure

From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>,
	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: Tue, 10 Jan 2017 14:30:27 +0100	[thread overview]
Message-ID: <20170110133027.GO4991@quack2.suse.cz> (raw)
In-Reply-To: <CAOQ4uxgyLkGBR4EczWd7+GJPm0kOQ3w5a5BxALK2oiviCvSP5w@mail.gmail.com>

On Sun 08-01-17 12:18:10, Amir Goldstein wrote:
> On Fri, Jan 6, 2017 at 12:43 PM, Jan Kara <jack@suse.cz> wrote:
> > +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); ?

Good catch (although this is only cosmetic - the assignment cannot escape
the conn->lock section and within it it does not matter where it happens
even for unlocked accesses). Done.

> > +/*
> > + * 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.

OK, probably you're right. I'll remove this function.

> > +/*
> > + * 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?

cmpxchg() is already guaranteed to contain the required barrier (see
Documentation/core-api/atomic_ops.rst).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR