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...
> + 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;
...
> +
> 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?
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)) {
...
}
> 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.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
