| Message ID | 20161222091538.28702-2-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 22, 2016 at 11:15 AM, Jan Kara <jack@suse.cz> wrote: > show_fdinfo() iterates group's list of marks. All marks found there are > guaranteed to be alive and they stay so until we release > group->mark_mutex. So remove uncecessary tests whether mark is alive. > The statement above is true for fanotify. I don't think it holds for inotify. SYS_inotify_rm_watch() fsnotify_destroy_mark() fsnotify_destroy_mark(mark, group) mutex_lock_nested(&group->mark_mutex) /* not really nested for inotify */ fsnotify_detach_mark(mark) mutex_unlock(&group->mark_mutex); fsnotify_free_mark(mark) mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE; /* !!! mark is not alive and on the group's list. group->mark_mutex is not held !!! */ list_add(&mark->g_list, &destroy_list); > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/notify/fdinfo.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c > index fd98e5100cab..601a59c8d87e 100644 > --- a/fs/notify/fdinfo.c > +++ b/fs/notify/fdinfo.c > @@ -76,8 +76,7 @@ static void inotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark) > struct inotify_inode_mark *inode_mark; > struct inode *inode; > > - if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE) || > - !(mark->flags & FSNOTIFY_MARK_FLAG_INODE)) > + if (!(mark->flags & FSNOTIFY_MARK_FLAG_INODE)) > return; > > inode_mark = container_of(mark, struct inotify_inode_mark, fsn_mark); > @@ -113,9 +112,6 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark) > unsigned int mflags = 0; > struct inode *inode; > > - if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE)) > - return; > - > if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY) > mflags |= FAN_MARK_IGNORED_SURV_MODIFY; > > -- > 2.10.2 > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 22-12-16 14:59:35, Amir Goldstein wrote: > On Thu, Dec 22, 2016 at 11:15 AM, Jan Kara <jack@suse.cz> wrote: > > show_fdinfo() iterates group's list of marks. All marks found there are > > guaranteed to be alive and they stay so until we release > > group->mark_mutex. So remove uncecessary tests whether mark is alive. > > > > The statement above is true for fanotify. I don't think it holds for inotify. > > SYS_inotify_rm_watch() > fsnotify_destroy_mark() > fsnotify_destroy_mark(mark, group) > mutex_lock_nested(&group->mark_mutex) /* not really nested for > inotify */ > fsnotify_detach_mark(mark) > mutex_unlock(&group->mark_mutex); > fsnotify_free_mark(mark) > mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE; > /* !!! mark is not alive and on the group's list. group->mark_mutex is > not held !!! */ How come it is on the group's list? fsnotify_detach_mark() will remove it from that list... The destroy_list is just a private list used for mark destruction, not a list of any group. > list_add(&mark->g_list, &destroy_list); Honza > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/notify/fdinfo.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c > > index fd98e5100cab..601a59c8d87e 100644 > > --- a/fs/notify/fdinfo.c > > +++ b/fs/notify/fdinfo.c > > @@ -76,8 +76,7 @@ static void inotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark) > > struct inotify_inode_mark *inode_mark; > > struct inode *inode; > > > > - if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE) || > > - !(mark->flags & FSNOTIFY_MARK_FLAG_INODE)) > > + if (!(mark->flags & FSNOTIFY_MARK_FLAG_INODE)) > > return; > > > > inode_mark = container_of(mark, struct inotify_inode_mark, fsn_mark); > > @@ -113,9 +112,6 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark) > > unsigned int mflags = 0; > > struct inode *inode; > > > > - if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE)) > > - return; > > - > > if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY) > > mflags |= FAN_MARK_IGNORED_SURV_MODIFY; > > > > -- > > 2.10.2 > >
On Thu, Dec 22, 2016 at 5:16 PM, Jan Kara <jack@suse.cz> wrote: > On Thu 22-12-16 14:59:35, Amir Goldstein wrote: >> On Thu, Dec 22, 2016 at 11:15 AM, Jan Kara <jack@suse.cz> wrote: >> > show_fdinfo() iterates group's list of marks. All marks found there are >> > guaranteed to be alive and they stay so until we release >> > group->mark_mutex. So remove uncecessary tests whether mark is alive. >> > >> >> The statement above is true for fanotify. I don't think it holds for inotify. >> >> SYS_inotify_rm_watch() >> fsnotify_destroy_mark() >> fsnotify_destroy_mark(mark, group) >> mutex_lock_nested(&group->mark_mutex) /* not really nested for >> inotify */ >> fsnotify_detach_mark(mark) >> mutex_unlock(&group->mark_mutex); >> fsnotify_free_mark(mark) >> mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE; >> /* !!! mark is not alive and on the group's list. group->mark_mutex is >> not held !!! */ > > How come it is on the group's list? fsnotify_detach_mark() will remove it > from that list... The destroy_list is just a private list used for mark > destruction, not a list of any group. > I stand corrected. This is bound to happen a few more times ;-) Reviewed-by: Amir Goldstein <amir73il@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c index fd98e5100cab..601a59c8d87e 100644 --- a/fs/notify/fdinfo.c +++ b/fs/notify/fdinfo.c @@ -76,8 +76,7 @@ static void inotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark) struct inotify_inode_mark *inode_mark; struct inode *inode; - if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE) || - !(mark->flags & FSNOTIFY_MARK_FLAG_INODE)) + if (!(mark->flags & FSNOTIFY_MARK_FLAG_INODE)) return; inode_mark = container_of(mark, struct inotify_inode_mark, fsn_mark); @@ -113,9 +112,6 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark) unsigned int mflags = 0; struct inode *inode; - if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE)) - return; - if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY) mflags |= FAN_MARK_IGNORED_SURV_MODIFY;
show_fdinfo() iterates group's list of marks. All marks found there are guaranteed to be alive and they stay so until we release group->mark_mutex. So remove uncecessary tests whether mark is alive. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/notify/fdinfo.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)