| Message ID | 20170315104632.7037-10-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 15, 2017 at 12:46 PM, Jan Kara <jack@suse.cz> 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@suse.cz> > --- [...] > 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.
On Wed 15-03-17 21:19:13, Amir Goldstein wrote: > On Wed, Mar 15, 2017 at 12:46 PM, Jan Kara <jack@suse.cz> 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
On Thu, Mar 16, 2017 at 9:58 AM, Jan Kara <jack@suse.cz> wrote: > On Wed 15-03-17 21:19:13, Amir Goldstein wrote: >> On Wed, Mar 15, 2017 at 12:46 PM, Jan Kara <jack@suse.cz> 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. > Looks very neat now. You may add Reviewed-by: Amir Goldstein <amir73il@gmail.com> (based on commit 6e23640 on your testing branch) I believe that addresses the last of my review comments. Let me know if I missed any Rvb tag. Amir.
On Thu 16-03-17 17:52:46, Amir Goldstein wrote: > >> 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. > > > > Looks very neat now. > You may add > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > (based on commit 6e23640 on your testing branch) > > I believe that addresses the last of my review comments. > Let me know if I missed any Rvb tag. Thanks for review! All patches have your tag now. Honza
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h index 1a2aec65ebd8..0354338aad78 100644 --- a/fs/notify/fsnotify.h +++ b/fs/notify/fsnotify.h @@ -21,20 +21,6 @@ extern u32 fsnotify_recalc_mask(struct fsnotify_mark_connector *conn); extern int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b); -/* Add mark to a proper place in mark list */ -extern int fsnotify_add_mark_list(struct fsnotify_mark_connector **connp, - struct fsnotify_mark *mark, - struct inode *inode, struct vfsmount *mnt, - int allow_dups); -/* add a mark to an inode */ -extern int fsnotify_add_inode_mark(struct fsnotify_mark *mark, - struct fsnotify_group *group, struct inode *inode, - int allow_dups); -/* add a mark to a vfsmount */ -extern int fsnotify_add_vfsmount_mark(struct fsnotify_mark *mark, - struct fsnotify_group *group, struct vfsmount *mnt, - int allow_dups); - /* vfsmount specific destruction of a mark */ extern void fsnotify_destroy_vfsmount_mark(struct fsnotify_mark *mark); /* inode specific destruction of a mark */ diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c index c3873b6920e7..87bef7d802db 100644 --- a/fs/notify/inode_mark.c +++ b/fs/notify/inode_mark.c @@ -92,31 +92,6 @@ struct fsnotify_mark *fsnotify_find_inode_mark(struct fsnotify_group *group, return mark; } -/* - * Attach an initialized mark to a given inode. - * 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. - */ -int fsnotify_add_inode_mark(struct fsnotify_mark *mark, - struct fsnotify_group *group, struct inode *inode, - int allow_dups) -{ - int ret; - - BUG_ON(!mutex_is_locked(&group->mark_mutex)); - assert_spin_locked(&mark->lock); - - spin_lock(&inode->i_lock); - ret = fsnotify_add_mark_list(&inode->i_fsnotify_marks, mark, inode, - NULL, allow_dups); - inode->i_fsnotify_mask = fsnotify_recalc_mask(inode->i_fsnotify_marks); - spin_unlock(&inode->i_lock); - - return ret; -} - /** * fsnotify_unmount_inodes - an sb is unmounting. handle any watched inodes. * @sb: superblock being unmounted. 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; } @@ -367,8 +389,10 @@ int fsnotify_add_mark_list(struct fsnotify_mark_connector **connp, hlist_for_each_entry(lmark, &conn->list, obj_list) { last = lmark; - if ((lmark->group == mark->group) && !allow_dups) - return -EEXIST; + if ((lmark->group == mark->group) && !allow_dups) { + err = -EEXIST; + goto out_err; + } cmp = fsnotify_compare_groups(lmark->group, mark->group); if (cmp >= 0) { @@ -382,7 +406,10 @@ int fsnotify_add_mark_list(struct fsnotify_mark_connector **connp, hlist_add_behind_rcu(&mark->obj_list, &last->obj_list); added: mark->connector = conn; - return 0; +out_err: + spin_unlock(lock); + spin_unlock(&mark->lock); + return err; } /* @@ -414,22 +441,16 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, list_add(&mark->g_list, &group->marks_list); atomic_inc(&group->num_marks); fsnotify_get_mark(mark); /* for i_list and g_list */ - - if (inode) { - ret = fsnotify_add_inode_mark(mark, group, inode, allow_dups); - if (ret) - goto err; - } else if (mnt) { - ret = fsnotify_add_vfsmount_mark(mark, group, mnt, allow_dups); - if (ret) - goto err; - } else { - BUG(); - } spin_unlock(&mark->lock); + ret = fsnotify_add_mark_list(mark, inode, mnt, allow_dups); + if (ret) + goto err; + if (inode) - __fsnotify_update_child_dentry_flags(inode); + fsnotify_recalc_inode_mask(inode); + else + fsnotify_recalc_vfsmount_mask(mnt); return ret; err: diff --git a/fs/notify/vfsmount_mark.c b/fs/notify/vfsmount_mark.c index e04e33ef02d4..49ccbdb74f82 100644 --- a/fs/notify/vfsmount_mark.c +++ b/fs/notify/vfsmount_mark.c @@ -80,27 +80,3 @@ struct fsnotify_mark *fsnotify_find_vfsmount_mark(struct fsnotify_group *group, return mark; } - -/* - * Attach an initialized mark to a given group and vfsmount. - * These marks may be used for the fsnotify backend to determine which - * event types should be delivered to which groups. - */ -int fsnotify_add_vfsmount_mark(struct fsnotify_mark *mark, - struct fsnotify_group *group, struct vfsmount *mnt, - int allow_dups) -{ - struct mount *m = real_mount(mnt); - int ret; - - BUG_ON(!mutex_is_locked(&group->mark_mutex)); - assert_spin_locked(&mark->lock); - - spin_lock(&mnt->mnt_root->d_lock); - ret = fsnotify_add_mark_list(&m->mnt_fsnotify_marks, mark, NULL, mnt, - allow_dups); - m->mnt_fsnotify_mask = fsnotify_recalc_mask(m->mnt_fsnotify_marks); - spin_unlock(&mnt->mnt_root->d_lock); - - return ret; -}
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@suse.cz> --- fs/notify/fsnotify.h | 14 ----------- fs/notify/inode_mark.c | 25 ------------------- fs/notify/mark.c | 63 +++++++++++++++++++++++++++++++---------------- fs/notify/vfsmount_mark.c | 24 ------------------ 4 files changed, 42 insertions(+), 84 deletions(-)