[01/22] fsnotify: Remove unnecessary tests when showing fdinfo
diff mbox

Message ID 20161222091538.28702-2-jack@suse.cz
State New
Headers show

Commit Message

Jan Kara Dec. 22, 2016, 9:15 a.m. UTC
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(-)

Comments

Amir Goldstein Dec. 22, 2016, 12:59 p.m. UTC | #1
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
Jan Kara Dec. 22, 2016, 3:16 p.m. UTC | #2
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
> >
Amir Goldstein Dec. 22, 2016, 3:54 p.m. UTC | #3
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

Patch
diff mbox

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;