| Message ID | 20170315104632.7037-12-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: > Move locking of locks protecting a list of marks into > fsnotify_recalc_mask(). This reduces code churn in the following patch > which changes the lock protecting the list of marks. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- [...] > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > index e8c707d07f9f..e7929539203a 100644 > --- a/fs/notify/mark.c > +++ b/fs/notify/mark.c > @@ -105,18 +105,45 @@ void fsnotify_put_mark(struct fsnotify_mark *mark) > } > } > > -/* Calculate mask of events for a list of marks */ > -u32 fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) > +static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) > { > u32 new_mask = 0; > struct fsnotify_mark *mark; > > + hlist_for_each_entry(mark, &conn->list, obj_list) { > + if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED) How is this added flag check related to this patch? Leftover from future patch? > + new_mask |= mark->mask; > + } > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) > + conn->inode->i_fsnotify_mask = new_mask; > + else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) > + real_mount(conn->mnt)->mnt_fsnotify_mask = new_mask; > +} > + > +/* > + * Calculate mask of events for a list of marks. The caller must make sure list > + * cannot disappear under us (usually by holding a mark->lock or > + * mark->group->mark_mutex for a mark on this list). > + */ > +void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) > +{ > if (!conn) > - return 0; > + return; > + > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) > + spin_lock(&conn->inode->i_lock); > + else > + spin_lock(&conn->mnt->mnt_root->d_lock); > + __fsnotify_recalc_mask(conn); > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) { > + struct inode *inode = igrab(conn->inode); > igrab change here related to this patch or belongs to future patch? If belongs here, better drop a word about it in commit message. > - hlist_for_each_entry(mark, &conn->list, obj_list) > - new_mask |= mark->mask; > - return new_mask; > + spin_unlock(&inode->i_lock); > + __fsnotify_update_child_dentry_flags(inode); > + iput(inode); > + } else { > + spin_unlock(&conn->mnt->mnt_root->d_lock); > + } > } >
On Wed 15-03-17 22:03:05, Amir Goldstein wrote: > On Wed, Mar 15, 2017 at 12:46 PM, Jan Kara <jack@suse.cz> wrote: > > Move locking of locks protecting a list of marks into > > fsnotify_recalc_mask(). This reduces code churn in the following patch > > which changes the lock protecting the list of marks. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > [...] > > > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > > index e8c707d07f9f..e7929539203a 100644 > > --- a/fs/notify/mark.c > > +++ b/fs/notify/mark.c > > @@ -105,18 +105,45 @@ void fsnotify_put_mark(struct fsnotify_mark *mark) > > } > > } > > > > -/* Calculate mask of events for a list of marks */ > > -u32 fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) > > +static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) > > { > > u32 new_mask = 0; > > struct fsnotify_mark *mark; > > > > + hlist_for_each_entry(mark, &conn->list, obj_list) { > > + if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED) > > How is this added flag check related to this patch? > Leftover from future patch? Yeah, that flag check will make sense only in later patch. At this point of the series non-attached marks cannot be see on conn->list. I'll move it. > > + new_mask |= mark->mask; > > + } > > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) > > + conn->inode->i_fsnotify_mask = new_mask; > > + else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) > > + real_mount(conn->mnt)->mnt_fsnotify_mask = new_mask; > > +} > > + > > +/* > > + * Calculate mask of events for a list of marks. The caller must make sure list > > + * cannot disappear under us (usually by holding a mark->lock or > > + * mark->group->mark_mutex for a mark on this list). > > + */ > > +void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) > > +{ > > if (!conn) > > - return 0; > > + return; > > + > > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) > > + spin_lock(&conn->inode->i_lock); > > + else > > + spin_lock(&conn->mnt->mnt_root->d_lock); > > + __fsnotify_recalc_mask(conn); > > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) { > > + struct inode *inode = igrab(conn->inode); > > > > igrab change here related to this patch or belongs to future patch? > If belongs here, better drop a word about it in commit message. It is making fsnotify_recalc_mask() more self-contained - with igrab() it does not have to assume the caller has somehow pinned the inode. As such it is not directly related to this locking change so I'll move it to a separate commit with appropriate message. Honza > > - hlist_for_each_entry(mark, &conn->list, obj_list) > > - new_mask |= mark->mask; > > - return new_mask; > > + spin_unlock(&inode->i_lock); > > + __fsnotify_update_child_dentry_flags(inode); > > + iput(inode); > > + } else { > > + spin_unlock(&conn->mnt->mnt_root->d_lock); > > + } > > } > >
On Thu 16-03-17 09:10:56, Jan Kara wrote: > > > + new_mask |= mark->mask; > > > + } > > > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) > > > + conn->inode->i_fsnotify_mask = new_mask; > > > + else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) > > > + real_mount(conn->mnt)->mnt_fsnotify_mask = new_mask; > > > +} > > > + > > > +/* > > > + * Calculate mask of events for a list of marks. The caller must make sure list > > > + * cannot disappear under us (usually by holding a mark->lock or > > > + * mark->group->mark_mutex for a mark on this list). > > > + */ > > > +void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) > > > +{ > > > if (!conn) > > > - return 0; > > > + return; > > > + > > > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) > > > + spin_lock(&conn->inode->i_lock); > > > + else > > > + spin_lock(&conn->mnt->mnt_root->d_lock); > > > + __fsnotify_recalc_mask(conn); > > > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) { > > > + struct inode *inode = igrab(conn->inode); > > > > > > > igrab change here related to this patch or belongs to future patch? > > If belongs here, better drop a word about it in commit message. > > It is making fsnotify_recalc_mask() more self-contained - with igrab() > it does not have to assume the caller has somehow pinned the inode. As such > it is not directly related to this locking change so I'll move it to a > separate commit with appropriate message. In the end, I've decided to just completely drop this change (as I've verified no caller really needs it) and added a comment explaining how callers must make sure inode is not going away under us. Honza
On Thu, Mar 16, 2017 at 12:11 PM, Jan Kara <jack@suse.cz> wrote: > On Thu 16-03-17 09:10:56, Jan Kara wrote: >> > > + new_mask |= mark->mask; >> > > + } >> > > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) >> > > + conn->inode->i_fsnotify_mask = new_mask; >> > > + else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) >> > > + real_mount(conn->mnt)->mnt_fsnotify_mask = new_mask; >> > > +} >> > > + >> > > +/* >> > > + * Calculate mask of events for a list of marks. The caller must make sure list >> > > + * cannot disappear under us (usually by holding a mark->lock or >> > > + * mark->group->mark_mutex for a mark on this list). >> > > + */ >> > > +void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) >> > > +{ >> > > if (!conn) >> > > - return 0; >> > > + return; >> > > + >> > > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) >> > > + spin_lock(&conn->inode->i_lock); >> > > + else >> > > + spin_lock(&conn->mnt->mnt_root->d_lock); >> > > + __fsnotify_recalc_mask(conn); >> > > + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) { >> > > + struct inode *inode = igrab(conn->inode); >> > > >> > >> > igrab change here related to this patch or belongs to future patch? >> > If belongs here, better drop a word about it in commit message. >> >> It is making fsnotify_recalc_mask() more self-contained - with igrab() >> it does not have to assume the caller has somehow pinned the inode. As such >> it is not directly related to this locking change so I'll move it to a >> separate commit with appropriate message. > > In the end, I've decided to just completely drop this change (as I've > verified no caller really needs it) and added a comment explaining how > callers must make sure inode is not going away under us. > The less the better. Reviewed commit 1f9c069 on your pushed branch, so Reviewed-by: Amir Goldstein <amir73il@gmail.com>
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c index 5024729dba23..41b2a070761c 100644 --- a/fs/notify/dnotify/dnotify.c +++ b/fs/notify/dnotify/dnotify.c @@ -69,8 +69,7 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark) if (old_mask == new_mask) return; - if (fsn_mark->connector) - fsnotify_recalc_inode_mask(fsn_mark->connector->inode); + fsnotify_recalc_mask(fsn_mark->connector); } /* diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h index 0354338aad78..96051780d50e 100644 --- a/fs/notify/fsnotify.h +++ b/fs/notify/fsnotify.h @@ -14,9 +14,6 @@ extern void fsnotify_flush_notify(struct fsnotify_group *group); /* protects reads of inode and vfsmount marks list */ extern struct srcu_struct fsnotify_mark_srcu; -/* Calculate mask of events for a list of marks */ -extern u32 fsnotify_recalc_mask(struct fsnotify_mark_connector *conn); - /* compare two groups for sorting of marks lists */ extern int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b); diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c index 87bef7d802db..9b2f4e6eb8eb 100644 --- a/fs/notify/inode_mark.c +++ b/fs/notify/inode_mark.c @@ -30,17 +30,9 @@ #include "../internal.h" -/* - * Recalculate the inode->i_fsnotify_mask, or the mask of all FS_* event types - * any notifier is interested in hearing for this inode. - */ void fsnotify_recalc_inode_mask(struct inode *inode) { - spin_lock(&inode->i_lock); - inode->i_fsnotify_mask = fsnotify_recalc_mask(inode->i_fsnotify_marks); - spin_unlock(&inode->i_lock); - - __fsnotify_update_child_dentry_flags(inode); + fsnotify_recalc_mask(inode->i_fsnotify_marks); } struct inode *fsnotify_destroy_inode_mark(struct fsnotify_mark *mark) @@ -57,14 +49,10 @@ struct inode *fsnotify_destroy_inode_mark(struct fsnotify_mark *mark) empty = hlist_empty(&mark->connector->list); mark->connector = NULL; - /* - * this mark is now off the inode->i_fsnotify_marks list and we - * hold the inode->i_lock, so this is the perfect time to update the - * inode->i_fsnotify_mask - */ - inode->i_fsnotify_mask = fsnotify_recalc_mask(inode->i_fsnotify_marks); spin_unlock(&inode->i_lock); + fsnotify_recalc_mask(inode->i_fsnotify_marks); + return empty ? inode : NULL; } diff --git a/fs/notify/mark.c b/fs/notify/mark.c index e8c707d07f9f..e7929539203a 100644 --- a/fs/notify/mark.c +++ b/fs/notify/mark.c @@ -105,18 +105,45 @@ void fsnotify_put_mark(struct fsnotify_mark *mark) } } -/* Calculate mask of events for a list of marks */ -u32 fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) +static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) { u32 new_mask = 0; struct fsnotify_mark *mark; + hlist_for_each_entry(mark, &conn->list, obj_list) { + if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED) + new_mask |= mark->mask; + } + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) + conn->inode->i_fsnotify_mask = new_mask; + else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) + real_mount(conn->mnt)->mnt_fsnotify_mask = new_mask; +} + +/* + * Calculate mask of events for a list of marks. The caller must make sure list + * cannot disappear under us (usually by holding a mark->lock or + * mark->group->mark_mutex for a mark on this list). + */ +void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn) +{ if (!conn) - return 0; + return; + + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) + spin_lock(&conn->inode->i_lock); + else + spin_lock(&conn->mnt->mnt_root->d_lock); + __fsnotify_recalc_mask(conn); + if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) { + struct inode *inode = igrab(conn->inode); - hlist_for_each_entry(mark, &conn->list, obj_list) - new_mask |= mark->mask; - return new_mask; + spin_unlock(&inode->i_lock); + __fsnotify_update_child_dentry_flags(inode); + iput(inode); + } else { + spin_unlock(&conn->mnt->mnt_root->d_lock); + } } /* @@ -411,10 +438,8 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, if (ret) goto err; - if (inode) - fsnotify_recalc_inode_mask(inode); - else - fsnotify_recalc_vfsmount_mask(mnt); + if (mark->mask) + fsnotify_recalc_mask(mark->connector); return ret; err: diff --git a/fs/notify/vfsmount_mark.c b/fs/notify/vfsmount_mark.c index 49ccbdb74f82..ffe0d7098cba 100644 --- a/fs/notify/vfsmount_mark.c +++ b/fs/notify/vfsmount_mark.c @@ -34,17 +34,9 @@ void fsnotify_clear_vfsmount_marks_by_group(struct fsnotify_group *group) fsnotify_clear_marks_by_group_flags(group, FSNOTIFY_OBJ_TYPE_VFSMOUNT); } -/* - * Recalculate the mnt->mnt_fsnotify_mask, or the mask of all FS_* event types - * any notifier is interested in hearing for this mount point - */ void fsnotify_recalc_vfsmount_mask(struct vfsmount *mnt) { - struct mount *m = real_mount(mnt); - - spin_lock(&mnt->mnt_root->d_lock); - m->mnt_fsnotify_mask = fsnotify_recalc_mask(m->mnt_fsnotify_marks); - spin_unlock(&mnt->mnt_root->d_lock); + fsnotify_recalc_mask(real_mount(mnt)->mnt_fsnotify_marks); } void fsnotify_destroy_vfsmount_mark(struct fsnotify_mark *mark) @@ -60,8 +52,9 @@ void fsnotify_destroy_vfsmount_mark(struct fsnotify_mark *mark) hlist_del_init_rcu(&mark->obj_list); mark->connector = NULL; - m->mnt_fsnotify_mask = fsnotify_recalc_mask(m->mnt_fsnotify_marks); spin_unlock(&mnt->mnt_root->d_lock); + + fsnotify_recalc_mask(m->mnt_fsnotify_marks); } /* diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index 96333fb09309..b954f1b2571c 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -327,6 +327,8 @@ extern struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group /* functions used to manipulate the marks attached to inodes */ +/* Calculate mask of events for a list of marks */ +extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn); /* run all marks associated with a vfsmount and update mnt->mnt_fsnotify_mask */ extern void fsnotify_recalc_vfsmount_mask(struct vfsmount *mnt); /* run all marks associated with an inode and update inode->i_fsnotify_mask */
Move locking of locks protecting a list of marks into fsnotify_recalc_mask(). This reduces code churn in the following patch which changes the lock protecting the list of marks. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/notify/dnotify/dnotify.c | 3 +-- fs/notify/fsnotify.h | 3 --- fs/notify/inode_mark.c | 18 +++------------- fs/notify/mark.c | 45 +++++++++++++++++++++++++++++++--------- fs/notify/vfsmount_mark.c | 13 +++--------- include/linux/fsnotify_backend.h | 2 ++ 6 files changed, 44 insertions(+), 40 deletions(-)