On Tue, Jul 14, 2020 at 2:54 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Thu 02-07-20 15:57:38, Amir Goldstein wrote:
> > Similar to events "on child" to watching directory, send event "on child"
> > with parent/name info if sb/mount/non-dir marks are interested in
> > parent/name info.
> >
> > The FS_EVENT_ON_CHILD flag can be set on sb/mount/non-dir marks to specify
> > interest in parent/name info for events on non-directory inodes.
> >
> > Events on "orphan" children (disconnected dentries) are sent without
> > parent/name info.
> >
> > Events on direcories are send with parent/name info only if the parent
> > directory is watching.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/notify/fsnotify.c | 50 +++++++++++++++++++++++---------
> > include/linux/fsnotify.h | 10 +++++--
> > include/linux/fsnotify_backend.h | 32 +++++++++++++++++---
> > 3 files changed, 73 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > index 7c6e624b24c9..6683c77a5b13 100644
> > --- a/fs/notify/fsnotify.c
> > +++ b/fs/notify/fsnotify.c
> > @@ -144,27 +144,55 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
> >
> > /*
> > * Notify this dentry's parent about a child's events with child name info
> > - * if parent is watching.
> > - * Notify only the child without name info if parent is not watching.
> > + * if parent is watching or if inode/sb/mount are interested in events with
> > + * parent and name info.
> > + *
> > + * Notify only the child without name info if parent is not watching and
> > + * inode/sb/mount are not interested in events with parent and name info.
> > */
> > int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
> > int data_type)
> > {
>
> Ugh, I have to say this function is now pretty difficult to digest. I was
> staring at it for an hour before I could make some sence of it...
>
Yeh, it took many trials to get to something that does the job,
but it could use some cosmetic changes ;-)
> > + const struct path *path = fsnotify_data_path(data, data_type);
> > + struct mount *mnt = path ? real_mount(path->mnt) : NULL;
> > struct inode *inode = d_inode(dentry);
> > struct dentry *parent;
> > + bool parent_watched = dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED;
> > + __u32 p_mask, test_mask, marks_mask = 0;
> > struct inode *p_inode;
> > int ret = 0;
> >
> > + /*
> > + * Do inode/sb/mount care about parent and name info on non-dir?
> > + * Do they care about any event at all?
> > + */
> > + if (!inode->i_fsnotify_marks && !inode->i_sb->s_fsnotify_marks &&
> > + (!mnt || !mnt->mnt_fsnotify_marks)) {
> > + if (!parent_watched)
> > + return 0;
> > + } else if (!(mask & FS_ISDIR) && !IS_ROOT(dentry)) {
> > + marks_mask |= fsnotify_want_parent(inode->i_fsnotify_mask);
> > + marks_mask |= fsnotify_want_parent(inode->i_sb->s_fsnotify_mask);
> > + if (mnt)
> > + marks_mask |= fsnotify_want_parent(mnt->mnt_fsnotify_mask);
> > + }
>
> OK, so AFAIU at this point (mask & marks_mask) tells us whether we need to
> grab parent because some mark needs parent into reported. Correct?
>
> Maybe I'd rename fsnotify_want_parent() (which seems like it returns bool)
> to fsnotify_parent_needed_mask() or something like that. Also I'd hide all
> those checks in a helper function like:
>
> fsnotify_event_needs_parent(mnt, inode, mask)
>
> So we'd then have just something like:
> if (!inode->i_fsnotify_marks && !inode->i_sb->s_fsnotify_marks &&
> (!mnt || !mnt->mnt_fsnotify_marks) && !parent_watched)
> return 0;
> if (!parent_watched && !fsnotify_event_needs_parent(mnt, inode, mask))
> goto notify_child;
OK, but we'd still need to store marks_mask for later in case event
needs parent.
> ...
>
> > +
> > parent = NULL;
> > - if (!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))
> > + test_mask = mask & FS_EVENTS_POSS_TO_PARENT;
> > + if (!(marks_mask & test_mask) && !parent_watched)
> > goto notify_child;
> >
> > + /* Does parent inode care about events on children? */
> > parent = dget_parent(dentry);
> > p_inode = parent->d_inode;
> > + p_mask = fsnotify_inode_watches_children(p_inode);
> >
> > - if (unlikely(!fsnotify_inode_watches_children(p_inode))) {
> > + if (p_mask)
> > + marks_mask |= p_mask;
> > + else if (unlikely(parent_watched))
> > __fsnotify_update_child_dentry_flags(p_inode);
> > - } else if (p_inode->i_fsnotify_mask & mask & ALL_FSNOTIFY_EVENTS) {
> > +
> > + if ((marks_mask & test_mask) && p_inode != inode) {
> ^^ this is effectively
> !IS_ROOT(dentry), isn't it? But since you've checked that above can it ever
> be that p_inode == inode?
>
True, but only if you add the hidden assumption (which should be true) that
DCACHE_FSNOTIFY_PARENT_WATCHED cannot be set on an IS_ROOT
dentry. Otherwise you can have parent_watched and not check IS_ROOT()
so prefered defensive here, but I don't mind dropping it.
> Also if marks_mask & test_mask == 0, why do you go to a branch that
> notifies child? There shouldn't be anything to report there either. Am I
> missing something? ... Oh, I see, the FS_EVENTS_POSS_TO_PARENT masking
> above could cause that child is still interested. Because mask & marks_mask
> can still contain something. Maybe it would be more comprehensible if we
> restructure the above like:
>
> p_mask = fsnotify_inode_watches_children(p_inode);
> if (unlikely(parent_watched && !p_mask))
> __fsnotify_update_child_dentry_flags(p_inode);
> /*
> * Include parent in notification either if some notification
> * groups require parent info (!parent_watched case) or the parent is
> * interested in this event.
> */
> if (!parent_watched || (mask & p_mask & ALL_FSNOTIFY_EVENTS)) {
> ...
> }
>
Sure!
> > struct name_snapshot name;
> >
> > /* When notifying parent, child should be passed as data */
> > @@ -346,15 +374,11 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
> > (!child || !child->i_fsnotify_marks))
> > return 0;
> >
> > - /* An event "on child" is not intended for a mount/sb mark */
> > - marks_mask = to_tell->i_fsnotify_mask;
> > - if (!child) {
> > - marks_mask |= sb->s_fsnotify_mask;
> > - if (mnt)
> > - marks_mask |= mnt->mnt_fsnotify_mask;
> > - } else {
> > + marks_mask = to_tell->i_fsnotify_mask | sb->s_fsnotify_mask;
> > + if (mnt)
> > + marks_mask |= mnt->mnt_fsnotify_mask;
> > + if (child)
> > marks_mask |= child->i_fsnotify_mask;
> > - }
>
> This hunk seems to belong to the previous patch... It fixes the problem
> I've spotted there.
I figured that we would find it here :-)
I will fix the previous patch.
Thanks,
Amir.
