diff mbox series

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

Message ID 20180830151551.27422-3-amir73il@gmail.com (mailing list archive)
State New, archived
Headers show
Series fanotify super block marks | expand

Commit Message

Amir Goldstein Aug. 30, 2018, 3:15 p.m. UTC 
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>
---
 fs/notify/fsnotify.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Jan Kara Aug. 31, 2018, 1:50 p.m. UTC | #1 
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?

>  	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.

> +			iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
> +				fsnotify_first_mark(&sb->s_fsnotify_marks);
>  	}
>  
>  	/*
> -	 * We need to merge inode & vfsmount mark lists so that inode mark
> -	 * ignore masks are properly reflected for mount mark notifications.
> +	 * We need to merge inode/vfsmount/sb mark lists so that e.g. inode mark
> +	 * ignore masks are properly reflected for mount/sb mark notifications.
>  	 * That's why this traversal is so complicated...
>  	 */
>  	while (fsnotify_iter_select_report_types(&iter_info)) {
> -- 
> 2.7.4
> 

								Honza
Amir Goldstein Aug. 31, 2018, 3:07 p.m. UTC | #2 
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()").
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
Jan Kara Sept. 3, 2018, 8:36 a.m. UTC | #3 
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
diff mbox series

Patch

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 775e731d3016..89a71242b786 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -325,15 +325,18 @@  int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	     const unsigned char *file_name, u32 cookie)
 {
 	struct fsnotify_iter_info iter_info = {};
-	struct mount *mnt;
+	struct super_block *sb = NULL;
+	struct mount *mnt = NULL;
+	__u32 mnt_or_sb_mask = 0;
 	int ret = 0;
 	/* global tests shouldn't care about events on child only the specific event */
 	__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
 
-	if (data_is == FSNOTIFY_EVENT_PATH)
+	if (data_is == FSNOTIFY_EVENT_PATH) {
 		mnt = real_mount(((const struct path *)data)->mnt);
-	else
-		mnt = NULL;
+		sb = mnt->mnt.mnt_sb;
+		mnt_or_sb_mask = mnt->mnt_fsnotify_mask | sb->s_fsnotify_mask;
+	}
 
 	/*
 	 * Optimization: srcu_read_lock() has a memory barrier which can
@@ -343,16 +346,16 @@  int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	 * need SRCU to keep them "alive".
 	 */
 	if (!to_tell->i_fsnotify_marks &&
-	    (!mnt || !mnt->mnt_fsnotify_marks))
+	    (!mnt || (!mnt->mnt_fsnotify_marks && !sb->s_fsnotify_marks)))
 		return 0;
 	/*
 	 * if this is a modify event we may need to clear the ignored masks
-	 * otherwise return if neither the inode nor the vfsmount care about
+	 * otherwise return if neither the inode nor the vfsmount/sb care about
 	 * this type of event.
 	 */
 	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)))
 		return 0;
 
 	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))
+			iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
+				fsnotify_first_mark(&sb->s_fsnotify_marks);
 	}
 
 	/*
-	 * We need to merge inode & vfsmount mark lists so that inode mark
-	 * ignore masks are properly reflected for mount mark notifications.
+	 * We need to merge inode/vfsmount/sb mark lists so that e.g. inode mark
+	 * ignore masks are properly reflected for mount/sb mark notifications.
 	 * That's why this traversal is so complicated...
 	 */
 	while (fsnotify_iter_select_report_types(&iter_info)) {