From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:47750 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725990AbeICMzY (ORCPT ); Mon, 3 Sep 2018 08:55:24 -0400 Date: Mon, 3 Sep 2018 10:36:16 +0200 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Marko Rauhamaa , linux-fsdevel Subject: Re: [PATCH v3 2/3] fsnotify: send path type events to group with super block marks Message-ID: <20180903083616.GB10027@quack2.suse.cz> References: <20180830151551.27422-1-amir73il@gmail.com> <20180830151551.27422-3-amir73il@gmail.com> <20180831135020.GI11622@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri 31-08-18 18:07:27, Amir Goldstein wrote: > On Fri, Aug 31, 2018 at 4:56 PM Jan Kara 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 > > > > 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 SUSE Labs, CR