On Sun 08-01-17 12:18:10, Amir Goldstein wrote:
> On Fri, Jan 6, 2017 at 12:43 PM, Jan Kara <jack@xxxxxxx> 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@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
