On Thu 03-12-20 14:58:41, Amir Goldstein wrote:
> On Thu, Dec 3, 2020 at 1:53 PM Jan Kara <jack@xxxxxxx> wrote:
> > > The semantics of INODE and CHILD marks were hard to follow and made the
> > > logic more complicated than it should have been. Replace it with INODE
> > > and PARENT marks semantics to hopefully make the logic more clear.
> >
> > Heh, wasn't I complaining about this when I was initially reviewing the
> > changes? ;)
>
> You certainly did and rightfully so.
> It took me a long time to untangle this knot, so I hope you like the result.
Yes, it is IMO more readable now.
> > > Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback")
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > ---
> > > fs/notify/fanotify/fanotify.c | 7 ++-
> > > fs/notify/fsnotify.c | 78 ++++++++++++++++++--------------
> > > include/linux/fsnotify_backend.h | 6 +--
> > > 3 files changed, 51 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > index 9167884a61ec..1192c9953620 100644
> > > --- a/fs/notify/fanotify/fanotify.c
> > > +++ b/fs/notify/fanotify/fanotify.c
> > > @@ -268,12 +268,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > > continue;
> > >
> > > /*
> > > - * If the event is for a child and this mark is on a parent not
> > > + * If the event is on a child and this mark is on a parent not
> > > * watching children, don't send it!
> > > */
> > > - if (event_mask & FS_EVENT_ON_CHILD &&
> > > - type == FSNOTIFY_OBJ_TYPE_INODE &&
> > > - !(mark->mask & FS_EVENT_ON_CHILD))
> > > + if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> > > + !(mark->mask & FS_EVENT_ON_CHILD))
> > > continue;
> > >
> > > marks_mask |= mark->mask;
> > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > > index c5c68bcbaadf..0676ce4d3352 100644
> > > --- a/fs/notify/fsnotify.c
> > > +++ b/fs/notify/fsnotify.c
> > > @@ -152,6 +152,13 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt,
> > > if (mask & FS_ISDIR)
> > > return false;
> > >
> > > + /*
> > > + * All events that are possible on child can also may be reported with
> > > + * parent/name info to inode/sb/mount. Otherwise, a watching parent
> > > + * could result in events reported with unexpected name info to sb/mount.
> > > + */
> > > + BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT);
> > > +
> > > /* Did either inode/sb/mount subscribe for events with parent/name? */
> > > marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask);
> > > marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask);
> > > @@ -249,6 +256,10 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,
> > > path && d_unlinked(path->dentry))
> > > return 0;
> > >
> > > + /* Check interest of this mark in case event was sent with two marks */
> > > + if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS))
> > > + return 0;
> > > +
> > > return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie);
> > > }
> > >
> > > @@ -258,38 +269,40 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
> > > u32 cookie, struct fsnotify_iter_info *iter_info)
> > > {
> > > struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
> > > - struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
> > > + struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info);
> > > int ret;
> > >
> > > if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) ||
> > > WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
> > > return 0;
> > >
> > > - /*
> > > - * An event can be sent on child mark iterator instead of inode mark
> > > - * iterator because of other groups that have interest of this inode
> > > - * and have marks on both parent and child. We can simplify this case.
> > > - */
> > > - if (!inode_mark) {
> > > - inode_mark = child_mark;
> > > - child_mark = NULL;
> > > + if (parent_mark) {
> > > + /*
> > > + * parent_mark indicates that the parent inode is watching children
> > > + * and interested in this event, which is an event possible on child.
> > > + * But is this mark watching children and interested in this event?
> > > + */
> > > + if (parent_mark->mask & FS_EVENT_ON_CHILD) {
> >
> > Is this really enough? I'd expect us to also check (mask &
> > parent_mark->mask & ALL_FSNOTIFY_EVENTS) != 0...
>
> I put it up in fsnotify_event_needs_parent() because this check is needed
> for both parent and child.
Right, I missed that. Thanks for explanation.
> BTW, at first I was thinking we needed to check EVENTS_POSS_ON_CHILD
> here but we don't because if event is not EVENTS_POSS_ON_CHILD
> (a.k.a. !parent_interested) then flag ON_CHILD is not set and parent_mark
> is not iterated .
OK :). I'll just improve the changelog and pick this patch up.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
