On Fri, Aug 31, 2018 at 4:56 PM Jan Kara <jack@xxxxxxx> 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@xxxxxxxxx>
>
> 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()").
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?
Thanks,
Amir.
[1] https://marc.info/?l=linux-fsdevel&m=152284295703053&w=2
