| Message ID | 20181114174344.17530-3-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsnotify prep work for fanotify dentry events | expand |
On Wed 14-11-18 19:43:41, Amir Goldstein wrote: > Filename 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. OK, I find 'directory modification events' clearer than 'filename events'. But I can live with your name since I don't really have a better alternative :). Just please define these events in terms of all FS_<foo> events that are involved so that everyone is on the same page which events you mean. > 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 filename event definition. It has no effect on any > existing backend, because dnotify and inotify always requets the > child events and fanotify does not get the delete,rename events. You keep forgetting about audit ;) But in this case it is fine as it always sets FS_EVENT_ON_CHILD as well. > The fsnotify_filename() helper is used to report all the filename > events. It gets a reference on parent dentry and passes it as the > data for the event along with the filename. > > fsnotify_filename() is different from fsnotify_parent(). > fsnotify_parent() is intended to report any events that happened on > child inodes when FS_EVENT_ON_CHILD is requested. > fsnotify_filename() is intended to report only filename events, > such as create,mkdir,link. Those events must always be reported > on a watched directory, regardless if FS_EVENT_ON_CHILD was requested. > > fsnotify_d_name() is a helper for the common case where the > filename to pass is dentry->d_name.name. > > It is safe to use these helpers with negative or not instantiated > dentries, such as the case with fsnotify_link() and > fsnotify_nameremove(). > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > include/linux/fsnotify.h | 40 +++++++++++++++++++++++++++++++--------- > 1 file changed, 31 insertions(+), 9 deletions(-) > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > index 9dadc0bcd7a9..d00ec5838d6e 100644 > --- a/include/linux/fsnotify.h > +++ b/include/linux/fsnotify.h > @@ -17,8 +17,31 @@ > #include <linux/slab.h> > #include <linux/bug.h> > > +/* > + * Notify this parent about a filename event (create,delete,rename). > + * Unlike fsnotify_parent(), the event will be reported regardless of the > + * FS_EVENT_ON_CHILD mask on the parent inode > + */ How about specifying this as 'Notify directory 'parent' about a change of some of its directory entries'? That way you avoid using 'filename' event in the description which is not defined. > +static inline int fsnotify_filename(struct dentry *parent, __u32 mask, > + const unsigned char *file_name, u32 cookie) And how about calling the function fsnotify_dir_change()? > +{ > + return fsnotify(d_inode(parent), mask, parent, FSNOTIFY_EVENT_DENTRY, > + file_name, cookie); > +} > + > +/* > + * Call fsnotify_filename() with parent and d_name of this dentry. > + * Safe to call with negative dentry, e.g. from fsnotify_nameremove() > + */ > +static inline int fsnotify_d_name(struct dentry *dentry, __u32 mask) Maybe call this fsnotify_dir_dentry_change()? > +{ > + return fsnotify_filename(dentry->d_parent, mask, > + 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 +108,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; You can just inline these two masks now. There's no point for the variable anymore. > @@ -99,8 +122,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir, > > fsnotify(old_dir, old_dir_mask, source, FSNOTIFY_EVENT_INODE, old_name, > fs_cookie); > - fsnotify(new_dir, new_dir_mask, moved, FSNOTIFY_EVENT_DENTRY, new_name, > - fs_cookie); > + fsnotify_filename(moved->d_parent, new_dir_mask, new_name, fs_cookie); The same comment as for the patch 1 here. You should justify that moved->d_parent is actually the same as new_dir and the dentry cannot change under us. The same holds for all the calls of fsnotify_d_name() where the directory inode originally passed in gets replaced with d_inode(dentry->d_parent). Honza
On Tue, Nov 20, 2018 at 1:59 PM Jan Kara <jack@suse.cz> wrote: > > On Wed 14-11-18 19:43:41, Amir Goldstein wrote: > > Filename 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. > > OK, I find 'directory modification events' clearer than 'filename events'. > But I can live with your name since I don't really have a better > alternative :). Just please define these events in terms of all FS_<foo> > events that are involved so that everyone is on the same page which events > you mean. > From a later fanotify patch: /* * Events whose reported fid is the parent directory. * fanotify may get support for reporting the filename in the future. * For now, listener only gets notified that a create/delete/rename took * place in that directory. */ #define FANOTIFY_FILENAME_EVENTS (FAN_MOVE | FAN_CREATE | FAN_DELETE) I went back and forth with this trying to come up with a better name and DIR_MODIFY_EVENTS did cross my mind, but the problem is that FS_MODIFY|FS_ISDIR is technically also a directory modification event, so we are really looking at "directory entry modification" and I didn't like the sounds of DIRENT_EVENTS. So I went for a name that described the information reported in the events which is parent+filename. Since you say you can live with this choice, I will add FS_FILENAME_EVENTS with a similar comment in this patch. > > 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 filename event definition. It has no effect on any > > existing backend, because dnotify and inotify always requets the > > child events and fanotify does not get the delete,rename events. > > You keep forgetting about audit ;) But in this case it is fine as it always > sets FS_EVENT_ON_CHILD as well. > Right... :-/ > > The fsnotify_filename() helper is used to report all the filename > > events. It gets a reference on parent dentry and passes it as the > > data for the event along with the filename. > > > > fsnotify_filename() is different from fsnotify_parent(). > > fsnotify_parent() is intended to report any events that happened on > > child inodes when FS_EVENT_ON_CHILD is requested. > > fsnotify_filename() is intended to report only filename events, > > such as create,mkdir,link. Those events must always be reported > > on a watched directory, regardless if FS_EVENT_ON_CHILD was requested. > > > > fsnotify_d_name() is a helper for the common case where the > > filename to pass is dentry->d_name.name. > > > > It is safe to use these helpers with negative or not instantiated > > dentries, such as the case with fsnotify_link() and > > fsnotify_nameremove(). > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > include/linux/fsnotify.h | 40 +++++++++++++++++++++++++++++++--------- > > 1 file changed, 31 insertions(+), 9 deletions(-) > > > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > > index 9dadc0bcd7a9..d00ec5838d6e 100644 > > --- a/include/linux/fsnotify.h > > +++ b/include/linux/fsnotify.h > > @@ -17,8 +17,31 @@ > > #include <linux/slab.h> > > #include <linux/bug.h> > > > > +/* > > + * Notify this parent about a filename event (create,delete,rename). > > + * Unlike fsnotify_parent(), the event will be reported regardless of the > > + * FS_EVENT_ON_CHILD mask on the parent inode > > + */ > > How about specifying this as 'Notify directory 'parent' about a change of > some of its directory entries'? That way you avoid using 'filename' event > in the description which is not defined. > Sure. Although I am going to define filename events now... > > +static inline int fsnotify_filename(struct dentry *parent, __u32 mask, > > + const unsigned char *file_name, u32 cookie) > > And how about calling the function fsnotify_dir_change()? Not comfortable with this name because of fsnotify_change() being passed a directory sounds like it should call here. The name of this helper signifies that it takes a filename argument and pass a non NULL filename argument to fsnotify(). Nothing more, nothing less. > > > +{ > > + return fsnotify(d_inode(parent), mask, parent, FSNOTIFY_EVENT_DENTRY, > > + file_name, cookie); > > +} > > + > > +/* > > + * Call fsnotify_filename() with parent and d_name of this dentry. > > + * Safe to call with negative dentry, e.g. from fsnotify_nameremove() > > + */ > > +static inline int fsnotify_d_name(struct dentry *dentry, __u32 mask) > > Maybe call this fsnotify_dir_dentry_change()? > Similar reasoning as above. The name of this wrapper signifies that it passed dentry->d_name as a non NULL filename argument to fsnotify(). Nothing more, nothing less. Thanks, Amir.
On Tue 20-11-18 16:58:31, Amir Goldstein wrote: > On Tue, Nov 20, 2018 at 1:59 PM Jan Kara <jack@suse.cz> wrote: > > > > On Wed 14-11-18 19:43:41, Amir Goldstein wrote: > > > Filename 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. > > > > OK, I find 'directory modification events' clearer than 'filename events'. > > But I can live with your name since I don't really have a better > > alternative :). Just please define these events in terms of all FS_<foo> > > events that are involved so that everyone is on the same page which events > > you mean. > > > > From a later fanotify patch: > > /* > * Events whose reported fid is the parent directory. > * fanotify may get support for reporting the filename in the future. > * For now, listener only gets notified that a create/delete/rename took > * place in that directory. > */ > #define FANOTIFY_FILENAME_EVENTS (FAN_MOVE | FAN_CREATE | FAN_DELETE) > > I went back and forth with this trying to come up with a better > name and DIR_MODIFY_EVENTS did cross my mind, but the > problem is that FS_MODIFY|FS_ISDIR is technically also a directory > modification event, so we are really looking at "directory entry modification" > and I didn't like the sounds of DIRENT_EVENTS. But we never generate FS_MODIFY|FS_ISDIR events so I don't think there's a big space for confusion (and I've deliberately used CHANGE instead of MODIFY to make the distinction even clearer). FWIW FANOTIFY_DIRENT_MODIFY_EVENTS would also look better than _FILENAME_EVENTS to me. > So I went for a name that described the information reported in the events > which is parent+filename. Well, but you don't report parent + filename, you report file handle and it isn't clear whether name will ever get reported. What initially confused me about filename events was that it sounded like you want to report all events (so also open / read / write / ...) for a _filename_. > Since you say you can live with this choice, I will add FS_FILENAME_EVENTS > with a similar comment in this patch. Talking about this more I'd really prefer if we could change the name to something else... > > > +static inline int fsnotify_filename(struct dentry *parent, __u32 mask, > > > + const unsigned char *file_name, u32 cookie) > > > > And how about calling the function fsnotify_dir_change()? > > Not comfortable with this name because of fsnotify_change() > being passed a directory sounds like it should call here. > The name of this helper signifies that it takes a filename argument > and pass a non NULL filename argument to fsnotify(). > Nothing more, nothing less. I find the current name confusing because, as I wrote above, it creates a feeling in me that we are notifying filename that something happens with it. But instead we notify a directory that something happens with filenames inside it... Generally we have names of form fsnotify_<what-happened> or fsnotify_<who-to-tell> and fsnotify_filename() does not fall into either of the cathegories which confused my brain trying to fit it in one of the two cathegories and failing. Maybe fsnotify_dir_op() because it is just a helper function for various directory operations? > > > +{ > > > + return fsnotify(d_inode(parent), mask, parent, FSNOTIFY_EVENT_DENTRY, > > > + file_name, cookie); > > > +} > > > + > > > +/* > > > + * Call fsnotify_filename() with parent and d_name of this dentry. > > > + * Safe to call with negative dentry, e.g. from fsnotify_nameremove() > > > + */ > > > +static inline int fsnotify_d_name(struct dentry *dentry, __u32 mask) > > > > Maybe call this fsnotify_dir_dentry_change()? > > > > Similar reasoning as above. > The name of this wrapper signifies that it passed dentry->d_name as > a non NULL filename argument to fsnotify(). > Nothing more, nothing less. And same objection to the original name here... Honza
On Wed, Nov 21, 2018 at 3:18 PM Jan Kara <jack@suse.cz> wrote: > > On Tue 20-11-18 16:58:31, Amir Goldstein wrote: > > On Tue, Nov 20, 2018 at 1:59 PM Jan Kara <jack@suse.cz> wrote: > > > > > > On Wed 14-11-18 19:43:41, Amir Goldstein wrote: > > > > Filename 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. > > > > > > OK, I find 'directory modification events' clearer than 'filename events'. > > > But I can live with your name since I don't really have a better > > > alternative :). Just please define these events in terms of all FS_<foo> > > > events that are involved so that everyone is on the same page which events > > > you mean. > > > > > > > From a later fanotify patch: > > > > /* > > * Events whose reported fid is the parent directory. > > * fanotify may get support for reporting the filename in the future. > > * For now, listener only gets notified that a create/delete/rename took > > * place in that directory. > > */ > > #define FANOTIFY_FILENAME_EVENTS (FAN_MOVE | FAN_CREATE | FAN_DELETE) > > > > I went back and forth with this trying to come up with a better > > name and DIR_MODIFY_EVENTS did cross my mind, but the > > problem is that FS_MODIFY|FS_ISDIR is technically also a directory > > modification event, so we are really looking at "directory entry modification" > > and I didn't like the sounds of DIRENT_EVENTS. > > But we never generate FS_MODIFY|FS_ISDIR events so I don't think there's a > big space for confusion (and I've deliberately used CHANGE instead of > MODIFY to make the distinction even clearer). FWIW > FANOTIFY_DIRENT_MODIFY_EVENTS would also look better than _FILENAME_EVENTS > to me. > Fair enough. I'll change to FANOTIFY_DIRENT_MODIFY_EVENTS and similar named helpers and comments in fsnotify.h. Thanks, Amir.
> > But we never generate FS_MODIFY|FS_ISDIR events so I don't think there's a > > big space for confusion (and I've deliberately used CHANGE instead of > > MODIFY to make the distinction even clearer). FWIW > > FANOTIFY_DIRENT_MODIFY_EVENTS would also look better than _FILENAME_EVENTS > > to me. > > > > Fair enough. I'll change to FANOTIFY_DIRENT_MODIFY_EVENTS > and similar named helpers and comments in fsnotify.h. > On second thought, if you don't object, I will opt for brevity and use: FANOTIFY_DIRENT_EVENTS. I don't thing that dropping the _MODIFY_ /_change part of the name is going to be a source of ambiguity. /* Notify this directory about a change in one of its directory entries. */ static inline int __fsnotify_dirent(struct inode *dir, __u32 mask, const unsigned char *file_name, u32 cookie) /* * Notify this dentry's parent about a change in the directory entry * associated with this dentry. * Unlike fsnotify_parent(), the event will be reported regardless of the * FS_EVENT_ON_CHILD mask on the parent inode * Safe to call with negative dentry, e.g. from fsnotify_nameremove() */ static inline int fsnotify_dirent(struct dentry *dentry, __u32 mask) Thanks, Amir.
> > @@ -85,8 +108,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; > > You can just inline these two masks now. There's no point for the variable > anymore. > He? why not? We still this this: if (old_dir == new_dir) old_dir_mask |= FS_DN_RENAME; if (isdir) { old_dir_mask |= FS_ISDIR; new_dir_mask |= FS_ISDIR; } @isdir refers to the d_type of the modified dirent. Thanks, Amir.
On Thu 22-11-18 09:45:06, Amir Goldstein wrote: > > > But we never generate FS_MODIFY|FS_ISDIR events so I don't think there's a > > > big space for confusion (and I've deliberately used CHANGE instead of > > > MODIFY to make the distinction even clearer). FWIW > > > FANOTIFY_DIRENT_MODIFY_EVENTS would also look better than _FILENAME_EVENTS > > > to me. > > > > > > > Fair enough. I'll change to FANOTIFY_DIRENT_MODIFY_EVENTS > > and similar named helpers and comments in fsnotify.h. > > > > On second thought, if you don't object, I will opt for brevity and use: > FANOTIFY_DIRENT_EVENTS. > I don't thing that dropping the _MODIFY_ /_change part of the name is > going to be a source of ambiguity. > > /* Notify this directory about a change in one of its directory entries. */ > static inline int __fsnotify_dirent(struct inode *dir, __u32 mask, > const unsigned char *file_name, u32 cookie) > /* > * Notify this dentry's parent about a change in the directory entry > * associated with this dentry. > * Unlike fsnotify_parent(), the event will be reported regardless of the > * FS_EVENT_ON_CHILD mask on the parent inode > * Safe to call with negative dentry, e.g. from fsnotify_nameremove() > */ > static inline int fsnotify_dirent(struct dentry *dentry, __u32 mask) Fine by me. Thanks! Honza
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index 9dadc0bcd7a9..d00ec5838d6e 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -17,8 +17,31 @@ #include <linux/slab.h> #include <linux/bug.h> +/* + * Notify this parent about a filename event (create,delete,rename). + * Unlike fsnotify_parent(), the event will be reported regardless of the + * FS_EVENT_ON_CHILD mask on the parent inode + */ +static inline int fsnotify_filename(struct dentry *parent, __u32 mask, + const unsigned char *file_name, u32 cookie) +{ + return fsnotify(d_inode(parent), mask, parent, FSNOTIFY_EVENT_DENTRY, + file_name, cookie); +} + +/* + * Call fsnotify_filename() with parent and d_name of this dentry. + * Safe to call with negative dentry, e.g. from fsnotify_nameremove() + */ +static inline int fsnotify_d_name(struct dentry *dentry, __u32 mask) +{ + return fsnotify_filename(dentry->d_parent, mask, + 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 +108,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) @@ -99,8 +122,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir, fsnotify(old_dir, old_dir_mask, source, FSNOTIFY_EVENT_INODE, old_name, fs_cookie); - fsnotify(new_dir, new_dir_mask, moved, FSNOTIFY_EVENT_DENTRY, new_name, - fs_cookie); + fsnotify_filename(moved->d_parent, new_dir_mask, new_name, fs_cookie); if (target) fsnotify_link_count(target); @@ -136,7 +158,7 @@ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir) if (isdir) mask |= FS_ISDIR; - fsnotify_parent(NULL, dentry, mask); + fsnotify_d_name(dentry, mask); } /* @@ -155,7 +177,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, FSNOTIFY_EVENT_DENTRY, dentry->d_name.name, 0); + fsnotify_d_name(dentry, FS_CREATE); } /* @@ -168,7 +190,7 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode, struct fsnotify_link_count(inode); audit_inode_child(dir, new_dentry, AUDIT_TYPE_CHILD_CREATE); - fsnotify(dir, FS_CREATE, new_dentry, FSNOTIFY_EVENT_DENTRY, new_dentry->d_name.name, 0); + fsnotify_d_name(new_dentry, FS_CREATE); } /* @@ -180,7 +202,7 @@ static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry) audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE); - fsnotify(inode, mask, dentry, FSNOTIFY_EVENT_DENTRY, dentry->d_name.name, 0); + fsnotify_d_name(dentry, mask); } /*
Filename 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 filename event definition. It has no effect on any existing backend, because dnotify and inotify always requets the child events and fanotify does not get the delete,rename events. The fsnotify_filename() helper is used to report all the filename events. It gets a reference on parent dentry and passes it as the data for the event along with the filename. fsnotify_filename() is different from fsnotify_parent(). fsnotify_parent() is intended to report any events that happened on child inodes when FS_EVENT_ON_CHILD is requested. fsnotify_filename() is intended to report only filename events, such as create,mkdir,link. Those events must always be reported on a watched directory, regardless if FS_EVENT_ON_CHILD was requested. fsnotify_d_name() is a helper for the common case where the filename to pass is dentry->d_name.name. It is safe to use these helpers with negative or not instantiated dentries, such as the case with fsnotify_link() and fsnotify_nameremove(). Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- include/linux/fsnotify.h | 40 +++++++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 9 deletions(-)