| Message ID | 20181202113826.32133-2-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: add support for more event types | expand |
On Sun 02-12-18 13:38:12, Amir Goldstein wrote: > "dirent" events are referring to events that modify directory entries, > such as create,delete,rename. Those events should always be reported > on a watched directory, regardless if FS_EVENT_ON_CHILD is set > on the watch mask. > > fsnotify_nameremove() and fsnotify_move() were modified to no longer > set the FS_EVENT_ON_CHILD event bit. This is a semantic change to > align with the "dirent" event definition. It has no effect on any > existing backend, because dnotify, inotify and audit always requets the > child events and fanotify does not get the delete,rename events. > > The fsnotify_dirent() helper is used instead of fsnotify_parent() to > report a dirent event to dentry->d_parent without FS_EVENT_ON_CHILD > and regardless if parent has the FS_EVENT_ON_CHILD bit set. > > ALL_FSNOTIFY_DIRENT_EVENTS defines all the dirent event types and > those event types have been extracted out of FS_EVENTS_POSS_ON_CHILD. > > That means for a directory with an inotify watch and only dirent > events in the mask (i.e. create,delete,move), all children dentries > will no longer have the DCACHE_FSNOTIFY_PARENT_WATCHED flag set. > This will allow all events that happen on children to be optimized > away in __fsnotify_parent() without the need to dereference > child->d_parent->d_inode->i_fsnotify_mask. > > Since the dirent events are never repoted via __fsnotify_parent(), > this results in no change of logic, but only an optimization. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > include/linux/fsnotify.h | 42 +++++++++++++++++++++++++------- > include/linux/fsnotify_backend.h | 36 +++++++++++++++------------ > 2 files changed, 54 insertions(+), 24 deletions(-) > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > index 2ccb08cb5d6a..8de8f390cce2 100644 > --- a/include/linux/fsnotify.h > +++ b/include/linux/fsnotify.h > @@ -17,8 +17,35 @@ > #include <linux/slab.h> > #include <linux/bug.h> > > +/* > + * Notify this @dir inode about a change in the directory entry @dentry. > + * > + * Unlike fsnotify_parent(), the event will be reported regardless of the > + * FS_EVENT_ON_CHILD mask on the parent inode. > + * > + * When called with NULL @dir (from fsnotify_nameremove()), the dentry parent > + * inode is used as the inode to report the event to. > + */ > +static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry, > + __u32 mask) > +{ > + if (!dir) > + dir = d_inode(dentry->d_parent); Just a small nit: This !dir handling is only used for fsnotify_nameremove(). So why not move that d_parent logic to that single call site? That would make the function easier to argue about. Otherwise I like the patch. Honza > + > + /* > + * This helper assumes d_parent and d_name are stable. It must be true > + * when called from fsnotify_create()/fsnotify_mkdir(). Less sure about > + * all callers that get here from d_delete() => fsnotify_nameremove(). > + */ > + WARN_ON(!inode_is_locked(dir)); > + > + return fsnotify(dir, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE, > + dentry->d_name.name, 0); > +} > + > /* Notify this dentry's parent about a child's events. */ > -static inline int fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask) > +static inline int fsnotify_parent(const struct path *path, > + struct dentry *dentry, __u32 mask) > { > if (!dentry) > dentry = path->dentry; > @@ -85,8 +112,8 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir, > { > struct inode *source = moved->d_inode; > u32 fs_cookie = fsnotify_get_cookie(); > - __u32 old_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_FROM); > - __u32 new_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_TO); > + __u32 old_dir_mask = FS_MOVED_FROM; > + __u32 new_dir_mask = FS_MOVED_TO; > const unsigned char *new_name = moved->d_name.name; > > if (old_dir == new_dir) > @@ -136,7 +163,7 @@ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir) > if (isdir) > mask |= FS_ISDIR; > > - fsnotify_parent(NULL, dentry, mask); > + fsnotify_dirent(NULL, dentry, mask); > } > > /* > @@ -155,7 +182,7 @@ static inline void fsnotify_create(struct inode *inode, struct dentry *dentry) > { > audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE); > > - fsnotify(inode, FS_CREATE, dentry->d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0); > + fsnotify_dirent(inode, dentry, FS_CREATE); > } > > /* > @@ -176,12 +203,9 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode, struct > */ > static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry) > { > - __u32 mask = (FS_CREATE | FS_ISDIR); > - struct inode *d_inode = dentry->d_inode; > - > audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE); > > - fsnotify(inode, mask, d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0); > + fsnotify_dirent(inode, dentry, FS_CREATE | FS_ISDIR); > } > > /* > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h > index 7639774e7475..7f195d43efaf 100644 > --- a/include/linux/fsnotify_backend.h > +++ b/include/linux/fsnotify_backend.h > @@ -59,27 +59,33 @@ > * dnotify and inotify. */ > #define FS_EVENT_ON_CHILD 0x08000000 > > -/* This is a list of all events that may get sent to a parernt based on fs event > - * happening to inodes inside that directory */ > -#define FS_EVENTS_POSS_ON_CHILD (FS_ACCESS | FS_MODIFY | FS_ATTRIB |\ > - FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN |\ > - FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE |\ > - FS_DELETE | FS_OPEN_PERM | FS_ACCESS_PERM | \ > - FS_OPEN_EXEC | FS_OPEN_EXEC_PERM) > - > #define FS_MOVE (FS_MOVED_FROM | FS_MOVED_TO) > > +/* > + * Directory entry modification events - reported only to directory > + * where entry is modified and not to a watching parent. > + * The watching parent may get an FS_ATTRIB|FS_EVENT_ON_CHILD event > + * when a directory entry inside a child subdir changes. > + */ > +#define ALL_FSNOTIFY_DIRENT_EVENTS (FS_CREATE | FS_DELETE | FS_MOVE) > + > #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \ > FS_OPEN_EXEC_PERM) > > +/* > + * This is a list of all events that may get sent to a parent based on fs event > + * happening to inodes inside that directory. > + */ > +#define FS_EVENTS_POSS_ON_CHILD (ALL_FSNOTIFY_PERM_EVENTS | \ > + FS_ACCESS | FS_MODIFY | FS_ATTRIB | \ > + FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | \ > + FS_OPEN | FS_OPEN_EXEC) > + > /* Events that can be reported to backends */ > -#define ALL_FSNOTIFY_EVENTS (FS_ACCESS | FS_MODIFY | FS_ATTRIB | \ > - FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN | \ > - FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE | \ > - FS_DELETE | FS_DELETE_SELF | FS_MOVE_SELF | \ > - FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \ > - FS_OPEN_PERM | FS_ACCESS_PERM | FS_DN_RENAME | \ > - FS_OPEN_EXEC | FS_OPEN_EXEC_PERM) > +#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \ > + FS_EVENTS_POSS_ON_CHILD | \ > + FS_DELETE_SELF | FS_MOVE_SELF | FS_DN_RENAME | \ > + FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED) > > /* Extra flags that may be reported with event or control handling of events */ > #define ALL_FSNOTIFY_FLAGS (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \ > -- > 2.17.1 >
On Thu, Jan 3, 2019 at 5:41 PM Jan Kara <jack@suse.cz> wrote: > > On Sun 02-12-18 13:38:12, Amir Goldstein wrote: > > "dirent" events are referring to events that modify directory entries, > > such as create,delete,rename. Those events should always be reported > > on a watched directory, regardless if FS_EVENT_ON_CHILD is set > > on the watch mask. > > > > fsnotify_nameremove() and fsnotify_move() were modified to no longer > > set the FS_EVENT_ON_CHILD event bit. This is a semantic change to > > align with the "dirent" event definition. It has no effect on any > > existing backend, because dnotify, inotify and audit always requets the > > child events and fanotify does not get the delete,rename events. > > > > The fsnotify_dirent() helper is used instead of fsnotify_parent() to > > report a dirent event to dentry->d_parent without FS_EVENT_ON_CHILD > > and regardless if parent has the FS_EVENT_ON_CHILD bit set. > > > > ALL_FSNOTIFY_DIRENT_EVENTS defines all the dirent event types and > > those event types have been extracted out of FS_EVENTS_POSS_ON_CHILD. > > > > That means for a directory with an inotify watch and only dirent > > events in the mask (i.e. create,delete,move), all children dentries > > will no longer have the DCACHE_FSNOTIFY_PARENT_WATCHED flag set. > > This will allow all events that happen on children to be optimized > > away in __fsnotify_parent() without the need to dereference > > child->d_parent->d_inode->i_fsnotify_mask. > > > > Since the dirent events are never repoted via __fsnotify_parent(), > > this results in no change of logic, but only an optimization. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > include/linux/fsnotify.h | 42 +++++++++++++++++++++++++------- > > include/linux/fsnotify_backend.h | 36 +++++++++++++++------------ > > 2 files changed, 54 insertions(+), 24 deletions(-) > > > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > > index 2ccb08cb5d6a..8de8f390cce2 100644 > > --- a/include/linux/fsnotify.h > > +++ b/include/linux/fsnotify.h > > @@ -17,8 +17,35 @@ > > #include <linux/slab.h> > > #include <linux/bug.h> > > > > +/* > > + * Notify this @dir inode about a change in the directory entry @dentry. > > + * > > + * Unlike fsnotify_parent(), the event will be reported regardless of the > > + * FS_EVENT_ON_CHILD mask on the parent inode. > > + * > > + * When called with NULL @dir (from fsnotify_nameremove()), the dentry parent > > + * inode is used as the inode to report the event to. > > + */ > > +static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry, > > + __u32 mask) > > +{ > > + if (!dir) > > + dir = d_inode(dentry->d_parent); > > Just a small nit: This !dir handling is only used for > fsnotify_nameremove(). So why not move that d_parent logic to that single > call site? That would make the function easier to argue about. > Yeh, I did that it Errata patch 16/15 (already squashed on my dev branch): https://marc.info/?l=linux-fsdevel&m=154410829914931&w=2 > Otherwise I like the patch. > > Honza > > > + > > + /* > > + * This helper assumes d_parent and d_name are stable. It must be true > > + * when called from fsnotify_create()/fsnotify_mkdir(). Less sure about > > + * all callers that get here from d_delete() => fsnotify_nameremove(). > > + */ > > + WARN_ON(!inode_is_locked(dir)); In your review of v3 you wrote: https://marc.info/?l=linux-fsdevel&m=154340994815488&w=2 "generally directory i_rwsem is held when unlinking and so dentry name cannot change but then it would be good to assert that? Who knows what some weird fs is doing..." So I went and did the research. The result is in said 16/15 Errata patch. Not sure you will like it though... Thanks, Amir.
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index 2ccb08cb5d6a..8de8f390cce2 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -17,8 +17,35 @@ #include <linux/slab.h> #include <linux/bug.h> +/* + * Notify this @dir inode about a change in the directory entry @dentry. + * + * Unlike fsnotify_parent(), the event will be reported regardless of the + * FS_EVENT_ON_CHILD mask on the parent inode. + * + * When called with NULL @dir (from fsnotify_nameremove()), the dentry parent + * inode is used as the inode to report the event to. + */ +static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry, + __u32 mask) +{ + if (!dir) + dir = d_inode(dentry->d_parent); + + /* + * This helper assumes d_parent and d_name are stable. It must be true + * when called from fsnotify_create()/fsnotify_mkdir(). Less sure about + * all callers that get here from d_delete() => fsnotify_nameremove(). + */ + WARN_ON(!inode_is_locked(dir)); + + return fsnotify(dir, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE, + dentry->d_name.name, 0); +} + /* Notify this dentry's parent about a child's events. */ -static inline int fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask) +static inline int fsnotify_parent(const struct path *path, + struct dentry *dentry, __u32 mask) { if (!dentry) dentry = path->dentry; @@ -85,8 +112,8 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir, { struct inode *source = moved->d_inode; u32 fs_cookie = fsnotify_get_cookie(); - __u32 old_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_FROM); - __u32 new_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_TO); + __u32 old_dir_mask = FS_MOVED_FROM; + __u32 new_dir_mask = FS_MOVED_TO; const unsigned char *new_name = moved->d_name.name; if (old_dir == new_dir) @@ -136,7 +163,7 @@ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir) if (isdir) mask |= FS_ISDIR; - fsnotify_parent(NULL, dentry, mask); + fsnotify_dirent(NULL, dentry, mask); } /* @@ -155,7 +182,7 @@ static inline void fsnotify_create(struct inode *inode, struct dentry *dentry) { audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE); - fsnotify(inode, FS_CREATE, dentry->d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0); + fsnotify_dirent(inode, dentry, FS_CREATE); } /* @@ -176,12 +203,9 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode, struct */ static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry) { - __u32 mask = (FS_CREATE | FS_ISDIR); - struct inode *d_inode = dentry->d_inode; - audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE); - fsnotify(inode, mask, d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0); + fsnotify_dirent(inode, dentry, FS_CREATE | FS_ISDIR); } /* diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index 7639774e7475..7f195d43efaf 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -59,27 +59,33 @@ * dnotify and inotify. */ #define FS_EVENT_ON_CHILD 0x08000000 -/* This is a list of all events that may get sent to a parernt based on fs event - * happening to inodes inside that directory */ -#define FS_EVENTS_POSS_ON_CHILD (FS_ACCESS | FS_MODIFY | FS_ATTRIB |\ - FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN |\ - FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE |\ - FS_DELETE | FS_OPEN_PERM | FS_ACCESS_PERM | \ - FS_OPEN_EXEC | FS_OPEN_EXEC_PERM) - #define FS_MOVE (FS_MOVED_FROM | FS_MOVED_TO) +/* + * Directory entry modification events - reported only to directory + * where entry is modified and not to a watching parent. + * The watching parent may get an FS_ATTRIB|FS_EVENT_ON_CHILD event + * when a directory entry inside a child subdir changes. + */ +#define ALL_FSNOTIFY_DIRENT_EVENTS (FS_CREATE | FS_DELETE | FS_MOVE) + #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \ FS_OPEN_EXEC_PERM) +/* + * This is a list of all events that may get sent to a parent based on fs event + * happening to inodes inside that directory. + */ +#define FS_EVENTS_POSS_ON_CHILD (ALL_FSNOTIFY_PERM_EVENTS | \ + FS_ACCESS | FS_MODIFY | FS_ATTRIB | \ + FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | \ + FS_OPEN | FS_OPEN_EXEC) + /* Events that can be reported to backends */ -#define ALL_FSNOTIFY_EVENTS (FS_ACCESS | FS_MODIFY | FS_ATTRIB | \ - FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN | \ - FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE | \ - FS_DELETE | FS_DELETE_SELF | FS_MOVE_SELF | \ - FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \ - FS_OPEN_PERM | FS_ACCESS_PERM | FS_DN_RENAME | \ - FS_OPEN_EXEC | FS_OPEN_EXEC_PERM) +#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \ + FS_EVENTS_POSS_ON_CHILD | \ + FS_DELETE_SELF | FS_MOVE_SELF | FS_DN_RENAME | \ + FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED) /* Extra flags that may be reported with event or control handling of events */ #define ALL_FSNOTIFY_FLAGS (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
"dirent" events are referring to events that modify directory entries, such as create,delete,rename. Those events should always be reported on a watched directory, regardless if FS_EVENT_ON_CHILD is set on the watch mask. fsnotify_nameremove() and fsnotify_move() were modified to no longer set the FS_EVENT_ON_CHILD event bit. This is a semantic change to align with the "dirent" event definition. It has no effect on any existing backend, because dnotify, inotify and audit always requets the child events and fanotify does not get the delete,rename events. The fsnotify_dirent() helper is used instead of fsnotify_parent() to report a dirent event to dentry->d_parent without FS_EVENT_ON_CHILD and regardless if parent has the FS_EVENT_ON_CHILD bit set. ALL_FSNOTIFY_DIRENT_EVENTS defines all the dirent event types and those event types have been extracted out of FS_EVENTS_POSS_ON_CHILD. That means for a directory with an inotify watch and only dirent events in the mask (i.e. create,delete,move), all children dentries will no longer have the DCACHE_FSNOTIFY_PARENT_WATCHED flag set. This will allow all events that happen on children to be optimized away in __fsnotify_parent() without the need to dereference child->d_parent->d_inode->i_fsnotify_mask. Since the dirent events are never repoted via __fsnotify_parent(), this results in no change of logic, but only an optimization. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- include/linux/fsnotify.h | 42 +++++++++++++++++++++++++------- include/linux/fsnotify_backend.h | 36 +++++++++++++++------------ 2 files changed, 54 insertions(+), 24 deletions(-)