Re: [PATCH v3 2/3] fsnotify: send path type events to group with super block marks

[Jan Kara <jack@suse.cz>]

On Fri 31-08-18 18:07:27, Amir Goldstein wrote:
> On Fri, Aug 31, 2018 at 4:56 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 30-08-18 18:15:50, Amir Goldstein wrote:
> > > Send events to group if super block mark mask matches the event
> > > and unless the same group has an ignore mask on the vfsmount or
> > > the inode on which the event occurred.
> > >
> > > Soon, fanotify backend is going to support super block marks and
> > > fanotify backend only supports path type events.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Two small questions below. Otherwise the patch looks good to me.
> >
> > > ---
> > >  fs/notify/fsnotify.c | 27 +++++++++++++++++----------
> > >  1 file changed, 17 insertions(+), 10 deletions(-)
> > >
> > ...
> > >       if (!(mask & FS_MODIFY) &&
> > >           !(test_mask & to_tell->i_fsnotify_mask) &&
> > > -         !(mnt && test_mask & mnt->mnt_fsnotify_mask))
> > > +         !(mnt && (test_mask & mnt_or_sb_mask)))
> >
> > When you use mnt_or_sb_mask, the 'mnt' check is useless, right?
> 
> Right. it could be !(test_mask & (to_tell->i_fsnotify_mask | mnt_or_sb_mask))
> if you think that is nicer.
> 
> >
> > >       iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
> > > @@ -364,16 +367,20 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
> > >       }
> > >
> > >       if (mnt && ((mask & FS_MODIFY) ||
> > > -                 (test_mask & mnt->mnt_fsnotify_mask))) {
> > > +                 (test_mask & mnt_or_sb_mask))) {
> > >               iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
> > >                       fsnotify_first_mark(&to_tell->i_fsnotify_marks);
> > >               iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
> > >                       fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
> > > +             if ((mask & FS_MODIFY) ||
> > > +                 (test_mask & sb->s_fsnotify_mask))
> >
> > Why is here this additional test? We might need to clear ignore masks on SB
> > list if nothing else. Also we need to reflect ignore mask from the
> > superblock marks... I agree there's probably no huge use for either of
> > these two functionalities but I just don't see a strong reason for
> > sb & mnt marks to behave differently.
> >
> 
> Hmm, that is indeed not pretty.
> It seems that I perpetuated the asymetric ignore relations between inode and
> mnt mark that the test above implemented forever.
> 
> In this thread [1], we already agreed that include-the-exclude is the desired
> semantics for ignore masks and the result was commit 92183a42898d
> ("fsnotify: fix ignore mask logic in send_to_group()").

Right.

> However, it seems we have missed this subtle spot here and need to fix
> it as well. The end result should look like this with no tests at all: (?)
> 
>         iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
>                 fsnotify_first_mark(&to_tell->i_fsnotify_marks);
>         iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
>                 fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
>         iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
>                 fsnotify_first_mark(&sb->s_fsnotify_marks);
> 
> Right?

Let me think loud and please correct me if I'm wrong somewhere. We need all
three lists if:

1) It is a FS_MODIFY event as that may need to clear ignore masks on some
list.

or

2) Mask for any list type (inode, mnt, sb) matches the mask of event - so
that we can collect also ignore masks and thus find out whether event
really should be reported.

This is already checked by a condition early in fsnotify(). So I agree that
the iter_info initialization should look like:

        iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
		fsnotify_first_mark(&to_tell->i_fsnotify_marks);
	if (mnt) {
		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
		iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
			fsnotify_first_mark(&sb->s_fsnotify_marks);
	}


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR