* [PATCH 1/1] ftruncate: create FAN_MODIFY and IN_MODIFY events @ 2014-10-03 18:16 Heinrich Schuchardt 2014-10-06 13:24 ` Jan Kara 0 siblings, 1 reply; 12+ messages in thread From: Heinrich Schuchardt @ 2014-10-03 18:16 UTC (permalink / raw) To: Alexander Viro Cc: linux-fsdevel, linux-kernel, Jan Kara, Eric Paris, Michael Kerrisk, John McCutchan, Robert Love, Heinrich Schuchardt The fanotify and the inotify API can be used to monitor changes of the file system. System call ftruncate modifies files. Hence it should trigger the corresponding fanotify (FAN_MODIFY) and inotify (IN_MODIFY) events. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- fs/open.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/open.c b/fs/open.c index d6fd3ac..e36f26e 100644 --- a/fs/open.c +++ b/fs/open.c @@ -189,6 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small) error = security_path_truncate(&f.file->f_path); if (!error) error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file); + if (!error) + fsnotify_modify(f.file); sb_end_write(inode->i_sb); out_putf: fdput(f); -- 2.1.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] ftruncate: create FAN_MODIFY and IN_MODIFY events 2014-10-03 18:16 [PATCH 1/1] ftruncate: create FAN_MODIFY and IN_MODIFY events Heinrich Schuchardt @ 2014-10-06 13:24 ` Jan Kara 2014-10-06 20:09 ` Heinrich Schuchardt 0 siblings, 1 reply; 12+ messages in thread From: Jan Kara @ 2014-10-06 13:24 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Alexander Viro, linux-fsdevel, linux-kernel, Jan Kara, Eric Paris, Michael Kerrisk, John McCutchan, Robert Love On Fri 03-10-14 20:16:30, Heinrich Schuchardt wrote: > The fanotify and the inotify API can be used to monitor changes of the file > system. > > System call ftruncate modifies files. Hence it should trigger the corresponding > fanotify (FAN_MODIFY) and inotify (IN_MODIFY) events. Hum, I would think that the appropriate event gets generated by fsnotify_change() called from notify_change()? Honza > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > fs/open.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/open.c b/fs/open.c > index d6fd3ac..e36f26e 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -189,6 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small) > error = security_path_truncate(&f.file->f_path); > if (!error) > error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file); > + if (!error) > + fsnotify_modify(f.file); > sb_end_write(inode->i_sb); > out_putf: > fdput(f); > -- > 2.1.0 > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] ftruncate: create FAN_MODIFY and IN_MODIFY events 2014-10-06 13:24 ` Jan Kara @ 2014-10-06 20:09 ` Heinrich Schuchardt 2014-10-07 19:23 ` Jan Kara 0 siblings, 1 reply; 12+ messages in thread From: Heinrich Schuchardt @ 2014-10-06 20:09 UTC (permalink / raw) To: Jan Kara Cc: Alexander Viro, linux-fsdevel, linux-kernel, Eric Paris, Michael Kerrisk, John McCutchan, Robert Love On 06.10.2014 15:24, Jan Kara wrote: > On Fri 03-10-14 20:16:30, Heinrich Schuchardt wrote: >> The fanotify and the inotify API can be used to monitor changes of the file >> system. >> >> System call ftruncate modifies files. Hence it should trigger the corresponding >> fanotify (FAN_MODIFY) and inotify (IN_MODIFY) events. > Hum, I would think that the appropriate event gets generated by > fsnotify_change() called from notify_change()? > > Honza Hello Jan, thank you for reviewing. fsnotify_change() calls fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0); fsnotify_modify() calls fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0); Only with FSNOTIFY_EVENT_PATH a mount is determined in fsnotify(). So a FAN_MODIFY event for a mount cannot occur. But I also did not see a FAN_MODIFY event when watching a file either. IN_MODIFY events are actually created. So would it be better to do the change in do_truncate and replace the call to notify_change by a call to fsnotify_modify? I just had a look at http://lxr.free-electrons.com/ident?i=notify_change There seem to be some other usages of notify_change that deserve consideration, e.g. in ecryptfs_truncate(). Best regards Heinrich Schuchardt >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> fs/open.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/open.c b/fs/open.c >> index d6fd3ac..e36f26e 100644 >> --- a/fs/open.c >> +++ b/fs/open.c >> @@ -189,6 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small) >> error = security_path_truncate(&f.file->f_path); >> if (!error) >> error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file); >> + if (!error) >> + fsnotify_modify(f.file); >> sb_end_write(inode->i_sb); >> out_putf: >> fdput(f); >> -- >> 2.1.0 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] ftruncate: create FAN_MODIFY and IN_MODIFY events 2014-10-06 20:09 ` Heinrich Schuchardt @ 2014-10-07 19:23 ` Jan Kara 2014-10-23 21:35 ` [PATCH v2 1/1] ftruncate, truncate: create fanotify events Heinrich Schuchardt 0 siblings, 1 reply; 12+ messages in thread From: Jan Kara @ 2014-10-07 19:23 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Jan Kara, Alexander Viro, linux-fsdevel, linux-kernel, Eric Paris, Michael Kerrisk, John McCutchan, Robert Love On Mon 06-10-14 22:09:12, Heinrich Schuchardt wrote: > On 06.10.2014 15:24, Jan Kara wrote: > >On Fri 03-10-14 20:16:30, Heinrich Schuchardt wrote: > >>The fanotify and the inotify API can be used to monitor changes of the file > >>system. > >> > >>System call ftruncate modifies files. Hence it should trigger the corresponding > >>fanotify (FAN_MODIFY) and inotify (IN_MODIFY) events. > > Hum, I would think that the appropriate event gets generated by > >fsnotify_change() called from notify_change()? > > > > Honza > > Hello Jan, > > thank you for reviewing. > > fsnotify_change() calls > fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0); > > fsnotify_modify() calls > fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0); > > Only with FSNOTIFY_EVENT_PATH a mount is determined in fsnotify(). > > So a FAN_MODIFY event for a mount cannot occur. But I also did not > see a FAN_MODIFY event when watching a file either. Yeah, fanotify processes only FSNOTIFY_EVENT_PATH events (without a path it cannot create an fd to pass to userspace). > IN_MODIFY events are actually created. > > So would it be better to do the change in do_truncate and replace > the call to notify_change by a call to fsnotify_modify? > > I just had a look at > http://lxr.free-electrons.com/ident?i=notify_change > > There seem to be some other usages of notify_change that deserve > consideration, e.g. in ecryptfs_truncate(). Yeah, so the reason why we don't generate FSNOTIFY_EVENT_PATH in notify_change() is because we have only dentry available there. OTOH from a quick look it doesn't look impossible to pass path there from the callers. So I'd rather propagate path to notify_change() and generate also fanotify events there than generating truncate events for fanotify separately somewhere else... Honza > >>Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >>--- > >> fs/open.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >>diff --git a/fs/open.c b/fs/open.c > >>index d6fd3ac..e36f26e 100644 > >>--- a/fs/open.c > >>+++ b/fs/open.c > >>@@ -189,6 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small) > >> error = security_path_truncate(&f.file->f_path); > >> if (!error) > >> error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file); > >>+ if (!error) > >>+ fsnotify_modify(f.file); > >> sb_end_write(inode->i_sb); > >> out_putf: > >> fdput(f); > >>-- > >>2.1.0 > >> > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/1] ftruncate, truncate: create fanotify events 2014-10-07 19:23 ` Jan Kara @ 2014-10-23 21:35 ` Heinrich Schuchardt 2014-11-10 20:30 ` Jan Kara 0 siblings, 1 reply; 12+ messages in thread From: Heinrich Schuchardt @ 2014-10-23 21:35 UTC (permalink / raw) To: Jan Kara Cc: Eric Paris, Alexander Viro, OGAWA Hirofumi, Jeff Layton, J. Bruce Fields, linux-fsdevel, linux-kernel, Heinrich Schuchardt :: On Tue, 7 Oct 2014 21:23:30 Jan Kara wrote: :: :: Yeah, so the reason why we don't generate FSNOTIFY_EVENT_PATH in :: notify_change() is because we have only dentry available there. OTOH from a :: quick look it doesn't look impossible to pass path there from the callers. :: So I'd rather propagate path to notify_change() and generate also fanotify :: events there than generating truncate events for fanotify separately :: somewhere else... The attached second version of the patch follows this idea of Jan. The fanotify and fanotify API can be used to monitor changes of the file system. System calls ftruncate and truncate modify files. Hence they should trigger the corresponding fanotify (FAN_MODIFY) event. Prior to the patch only a inotify (IN_MODIFY) event is triggered because the path information is not passed to the notification framework. The patch changes the interface of fsnotify_change to allow path information to be passed. Fsnotify_change is only called by notify_change and by fat_ioctl_set_attributes. notify_change is called by a larger number of callers. Not all of these have access to path object. To limit the number of changes the patch renames notify_change to do_notify_change and adds a path parameter. The patch further adds two wrappers: notify_change for callers that cannot provide a path and notify_change_path for callers that can provide a path. The patch changes do_truncate to accept a path parameter passed to notify_change_path and adjusts all callers of do_truncate. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- fs/attr.c | 35 ++++++++++++++++++++++++++++++++--- fs/fat/file.c | 2 +- fs/namei.c | 2 +- fs/open.c | 10 ++++++---- include/linux/fs.h | 3 ++- include/linux/fsnotify.h | 12 +++++++++--- 6 files changed, 51 insertions(+), 13 deletions(-) diff --git a/fs/attr.c b/fs/attr.c index 6530ced..894967e 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -168,13 +168,17 @@ void setattr_copy(struct inode *inode, const struct iattr *attr) EXPORT_SYMBOL(setattr_copy); /** - * notify_change - modify attributes of a filesytem object + * do_notify_change - modify attributes of a filesytem object + * @path: path of object affected * @dentry: object affected * @iattr: new attributes * @delegated_inode: returns inode, if the inode is delegated * * The caller must hold the i_mutex on the affected object. * + * If NULL is passed in attribute path, fanotify events cannot be + * created. + * * If notify_change discovers a delegation in need of breaking, * it will return -EWOULDBLOCK and return a reference to the inode in * delegated_inode. The caller should then break the delegation and @@ -187,7 +191,8 @@ EXPORT_SYMBOL(setattr_copy); * the file open for write, as there can be no conflicting delegation in * that case. */ -int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **delegated_inode) +int do_notify_change(struct path *path, struct dentry *dentry, + struct iattr *attr, struct inode **delegated_inode) { struct inode *inode = dentry->d_inode; umode_t mode = inode->i_mode; @@ -268,11 +273,35 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de error = simple_setattr(dentry, attr); if (!error) { - fsnotify_change(dentry, ia_valid); + fsnotify_change(path, dentry, ia_valid); ima_inode_post_setattr(dentry); evm_inode_post_setattr(dentry, ia_valid); } return error; } + +/* + * notify_change - modify attributes of a filesytem object + * + * This call will not create fanotify events. It should be replaced by + * notify_change_path where possible. + */ +int notify_change(struct dentry *dentry, struct iattr *attr, + struct inode **delegated_inode) +{ + return do_notify_change(NULL, dentry, attr, delegated_inode); +} EXPORT_SYMBOL(notify_change); + +/* + * notify_change_path - modify attributes of a filesytem object + * + * This call is a replacement for notify_change. It creates fanotify events. + */ +int notify_change_path(struct path *path, struct iattr *attr, + struct inode **delegated_inode) +{ + return do_notify_change(path, path->dentry, attr, delegated_inode); +} +EXPORT_SYMBOL(notify_change_path); diff --git a/fs/fat/file.c b/fs/fat/file.c index f2c73ae..d293a0b 100644 --- a/fs/fat/file.c +++ b/fs/fat/file.c @@ -101,7 +101,7 @@ static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr) if (err) goto out_unlock_inode; - fsnotify_change(file->f_path.dentry, ia.ia_valid); + fsnotify_change(&file->f_path, file->f_path.dentry, ia.ia_valid); if (sbi->options.sys_immutable) { if (attr & ATTR_SYS) inode->i_flags |= S_IMMUTABLE; diff --git a/fs/namei.c b/fs/namei.c index 43927d1..0a1f3b7 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2603,7 +2603,7 @@ static int handle_truncate(struct file *filp) if (!error) error = security_path_truncate(path); if (!error) { - error = do_truncate(path->dentry, 0, + error = do_truncate(path, 0, ATTR_MTIME|ATTR_CTIME|ATTR_OPEN, filp); } diff --git a/fs/open.c b/fs/open.c index d6fd3ac..390e322 100644 --- a/fs/open.c +++ b/fs/open.c @@ -34,11 +34,12 @@ #include "internal.h" -int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs, +int do_truncate(struct path *path, loff_t length, unsigned int time_attrs, struct file *filp) { int ret; struct iattr newattrs; + struct dentry *dentry = path->dentry; /* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */ if (length < 0) @@ -58,7 +59,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs, mutex_lock(&dentry->d_inode->i_mutex); /* Note any delegations or leases have already been broken: */ - ret = notify_change(dentry, &newattrs, NULL); + ret = notify_change_path(path, &newattrs, NULL); mutex_unlock(&dentry->d_inode->i_mutex); return ret; } @@ -104,7 +105,7 @@ long vfs_truncate(struct path *path, loff_t length) if (!error) error = security_path_truncate(path); if (!error) - error = do_truncate(path->dentry, length, 0, NULL); + error = do_truncate(path, length, 0, NULL); put_write_and_out: put_write_access(inode); @@ -188,7 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small) if (!error) error = security_path_truncate(&f.file->f_path); if (!error) - error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file); + error = do_truncate(&f.file->f_path, length, + ATTR_MTIME|ATTR_CTIME, f.file); sb_end_write(inode->i_sb); out_putf: fdput(f); diff --git a/include/linux/fs.h b/include/linux/fs.h index 4111ee0..e5b0b05 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2031,7 +2031,7 @@ struct filename { }; extern long vfs_truncate(struct path *, loff_t); -extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs, +extern int do_truncate(struct path *, loff_t start, unsigned int time_attrs, struct file *filp); extern int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len); @@ -2253,6 +2253,7 @@ extern void emergency_remount(void); extern sector_t bmap(struct inode *, sector_t); #endif extern int notify_change(struct dentry *, struct iattr *, struct inode **); +extern int notify_change_path(struct path *, struct iattr *, struct inode **); extern int inode_permission(struct inode *, int); extern int generic_permission(struct inode *, int); diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index 1c804b0..0fd594b 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -276,7 +276,8 @@ static inline void fsnotify_xattr(struct dentry *dentry) * fsnotify_change - notify_change event. file was modified and/or metadata * was changed. */ -static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid) +static inline void fsnotify_change(struct path *path, struct dentry *dentry, + unsigned int ia_valid) { struct inode *inode = dentry->d_inode; __u32 mask = 0; @@ -303,8 +304,13 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid) if (S_ISDIR(inode->i_mode)) mask |= FS_ISDIR; - fsnotify_parent(NULL, dentry, mask); - fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0); + fsnotify_parent(path, dentry, mask); + if (path == NULL) + fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, + NULL, 0); + else + fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, + NULL, 0); } } -- 2.1.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] ftruncate, truncate: create fanotify events 2014-10-23 21:35 ` [PATCH v2 1/1] ftruncate, truncate: create fanotify events Heinrich Schuchardt @ 2014-11-10 20:30 ` Jan Kara 2014-11-10 23:10 ` Heinrich Schuchardt 2014-11-11 7:34 ` Christoph Hellwig 0 siblings, 2 replies; 12+ messages in thread From: Jan Kara @ 2014-11-10 20:30 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Jan Kara, Eric Paris, Alexander Viro, OGAWA Hirofumi, Jeff Layton, J. Bruce Fields, linux-fsdevel, linux-kernel On Thu 23-10-14 23:35:07, Heinrich Schuchardt wrote: > :: On Tue, 7 Oct 2014 21:23:30 Jan Kara wrote: > :: > :: Yeah, so the reason why we don't generate FSNOTIFY_EVENT_PATH in > :: notify_change() is because we have only dentry available there. OTOH from a > :: quick look it doesn't look impossible to pass path there from the callers. > :: So I'd rather propagate path to notify_change() and generate also fanotify > :: events there than generating truncate events for fanotify separately > :: somewhere else... > > The attached second version of the patch follows this idea of Jan. > > The fanotify and fanotify API can be used to monitor changes of the file > system. > > System calls ftruncate and truncate modify files. Hence they should trigger > the corresponding fanotify (FAN_MODIFY) event. > > Prior to the patch only a inotify (IN_MODIFY) event is triggered because > the path information is not passed to the notification framework. > > The patch changes the interface of fsnotify_change to allow path information > to be passed. Fsnotify_change is only called by notify_change and by > fat_ioctl_set_attributes. > > notify_change is called by a larger number of callers. Not all of these > have access to path object. To limit the number of changes the patch renames > notify_change to do_notify_change and adds a path parameter. The patch further > adds two wrappers: notify_change for callers that cannot provide a path > and notify_change_path for callers that can provide a path. > > The patch changes do_truncate to accept a path parameter passed to > notify_change_path and adjusts all callers of do_truncate. So what I somewhat dislike about this patch is that notify_change() is sometimes called with dentry and sometimes with path. That way it's not completely clear when fanotify events will be generated and when not. Sadly it isn't easy to provide struct path in all the places where we are calling notify_change() so I'm not sure what would a better solution look like either :( Honza > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > fs/attr.c | 35 ++++++++++++++++++++++++++++++++--- > fs/fat/file.c | 2 +- > fs/namei.c | 2 +- > fs/open.c | 10 ++++++---- > include/linux/fs.h | 3 ++- > include/linux/fsnotify.h | 12 +++++++++--- > 6 files changed, 51 insertions(+), 13 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index 6530ced..894967e 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -168,13 +168,17 @@ void setattr_copy(struct inode *inode, const struct iattr *attr) > EXPORT_SYMBOL(setattr_copy); > > /** > - * notify_change - modify attributes of a filesytem object > + * do_notify_change - modify attributes of a filesytem object > + * @path: path of object affected > * @dentry: object affected > * @iattr: new attributes > * @delegated_inode: returns inode, if the inode is delegated > * > * The caller must hold the i_mutex on the affected object. > * > + * If NULL is passed in attribute path, fanotify events cannot be > + * created. > + * > * If notify_change discovers a delegation in need of breaking, > * it will return -EWOULDBLOCK and return a reference to the inode in > * delegated_inode. The caller should then break the delegation and > @@ -187,7 +191,8 @@ EXPORT_SYMBOL(setattr_copy); > * the file open for write, as there can be no conflicting delegation in > * that case. > */ > -int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **delegated_inode) > +int do_notify_change(struct path *path, struct dentry *dentry, > + struct iattr *attr, struct inode **delegated_inode) > { > struct inode *inode = dentry->d_inode; > umode_t mode = inode->i_mode; > @@ -268,11 +273,35 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de > error = simple_setattr(dentry, attr); > > if (!error) { > - fsnotify_change(dentry, ia_valid); > + fsnotify_change(path, dentry, ia_valid); > ima_inode_post_setattr(dentry); > evm_inode_post_setattr(dentry, ia_valid); > } > > return error; > } > + > +/* > + * notify_change - modify attributes of a filesytem object > + * > + * This call will not create fanotify events. It should be replaced by > + * notify_change_path where possible. > + */ > +int notify_change(struct dentry *dentry, struct iattr *attr, > + struct inode **delegated_inode) > +{ > + return do_notify_change(NULL, dentry, attr, delegated_inode); > +} > EXPORT_SYMBOL(notify_change); > + > +/* > + * notify_change_path - modify attributes of a filesytem object > + * > + * This call is a replacement for notify_change. It creates fanotify events. > + */ > +int notify_change_path(struct path *path, struct iattr *attr, > + struct inode **delegated_inode) > +{ > + return do_notify_change(path, path->dentry, attr, delegated_inode); > +} > +EXPORT_SYMBOL(notify_change_path); > diff --git a/fs/fat/file.c b/fs/fat/file.c > index f2c73ae..d293a0b 100644 > --- a/fs/fat/file.c > +++ b/fs/fat/file.c > @@ -101,7 +101,7 @@ static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr) > if (err) > goto out_unlock_inode; > > - fsnotify_change(file->f_path.dentry, ia.ia_valid); > + fsnotify_change(&file->f_path, file->f_path.dentry, ia.ia_valid); > if (sbi->options.sys_immutable) { > if (attr & ATTR_SYS) > inode->i_flags |= S_IMMUTABLE; > diff --git a/fs/namei.c b/fs/namei.c > index 43927d1..0a1f3b7 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -2603,7 +2603,7 @@ static int handle_truncate(struct file *filp) > if (!error) > error = security_path_truncate(path); > if (!error) { > - error = do_truncate(path->dentry, 0, > + error = do_truncate(path, 0, > ATTR_MTIME|ATTR_CTIME|ATTR_OPEN, > filp); > } > diff --git a/fs/open.c b/fs/open.c > index d6fd3ac..390e322 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -34,11 +34,12 @@ > > #include "internal.h" > > -int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs, > +int do_truncate(struct path *path, loff_t length, unsigned int time_attrs, > struct file *filp) > { > int ret; > struct iattr newattrs; > + struct dentry *dentry = path->dentry; > > /* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */ > if (length < 0) > @@ -58,7 +59,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs, > > mutex_lock(&dentry->d_inode->i_mutex); > /* Note any delegations or leases have already been broken: */ > - ret = notify_change(dentry, &newattrs, NULL); > + ret = notify_change_path(path, &newattrs, NULL); > mutex_unlock(&dentry->d_inode->i_mutex); > return ret; > } > @@ -104,7 +105,7 @@ long vfs_truncate(struct path *path, loff_t length) > if (!error) > error = security_path_truncate(path); > if (!error) > - error = do_truncate(path->dentry, length, 0, NULL); > + error = do_truncate(path, length, 0, NULL); > > put_write_and_out: > put_write_access(inode); > @@ -188,7 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small) > if (!error) > error = security_path_truncate(&f.file->f_path); > if (!error) > - error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file); > + error = do_truncate(&f.file->f_path, length, > + ATTR_MTIME|ATTR_CTIME, f.file); > sb_end_write(inode->i_sb); > out_putf: > fdput(f); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 4111ee0..e5b0b05 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2031,7 +2031,7 @@ struct filename { > }; > > extern long vfs_truncate(struct path *, loff_t); > -extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs, > +extern int do_truncate(struct path *, loff_t start, unsigned int time_attrs, > struct file *filp); > extern int do_fallocate(struct file *file, int mode, loff_t offset, > loff_t len); > @@ -2253,6 +2253,7 @@ extern void emergency_remount(void); > extern sector_t bmap(struct inode *, sector_t); > #endif > extern int notify_change(struct dentry *, struct iattr *, struct inode **); > +extern int notify_change_path(struct path *, struct iattr *, struct inode **); > extern int inode_permission(struct inode *, int); > extern int generic_permission(struct inode *, int); > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > index 1c804b0..0fd594b 100644 > --- a/include/linux/fsnotify.h > +++ b/include/linux/fsnotify.h > @@ -276,7 +276,8 @@ static inline void fsnotify_xattr(struct dentry *dentry) > * fsnotify_change - notify_change event. file was modified and/or metadata > * was changed. > */ > -static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid) > +static inline void fsnotify_change(struct path *path, struct dentry *dentry, > + unsigned int ia_valid) > { > struct inode *inode = dentry->d_inode; > __u32 mask = 0; > @@ -303,8 +304,13 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid) > if (S_ISDIR(inode->i_mode)) > mask |= FS_ISDIR; > > - fsnotify_parent(NULL, dentry, mask); > - fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0); > + fsnotify_parent(path, dentry, mask); > + if (path == NULL) > + fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, > + NULL, 0); > + else > + fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, > + NULL, 0); > } > } > > -- > 2.1.1 > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] ftruncate, truncate: create fanotify events 2014-11-10 20:30 ` Jan Kara @ 2014-11-10 23:10 ` Heinrich Schuchardt 2014-11-11 7:34 ` Christoph Hellwig 1 sibling, 0 replies; 12+ messages in thread From: Heinrich Schuchardt @ 2014-11-10 23:10 UTC (permalink / raw) To: Andrew Morton Cc: Jan Kara, Eric Paris, Alexander Viro, OGAWA Hirofumi, Jeff Layton, J. Bruce Fields, linux-fsdevel, linux-kernel Hello Andrew, please, consider the patch in https://lkml.org/lkml/2014/10/23/611 for inclusion in the MM tree. It will ensure that FAN_MODIFY events are created for truncate() and ftruncate(). To my knowledge these are the last two system calls changing files directly that are not creating fanotify events. As Jan Kara correctly points there a few other sources of changes to the file system for which we still have to analyze if they should create fanotify events. E.g. meta-filesystems like ecryptfs and overlayfs trigger changes of the underlying file systems. Best regards Heinrich Schuchardt On 10.11.2014 21:30, Jan Kara wrote: > On Thu 23-10-14 23:35:07, Heinrich Schuchardt wrote: >> :: On Tue, 7 Oct 2014 21:23:30 Jan Kara wrote: >> :: >> :: Yeah, so the reason why we don't generate FSNOTIFY_EVENT_PATH in >> :: notify_change() is because we have only dentry available there. OTOH from a >> :: quick look it doesn't look impossible to pass path there from the callers. >> :: So I'd rather propagate path to notify_change() and generate also fanotify >> :: events there than generating truncate events for fanotify separately >> :: somewhere else... >> >> The attached second version of the patch follows this idea of Jan. >> >> The fanotify and fanotify API can be used to monitor changes of the file >> system. >> >> System calls ftruncate and truncate modify files. Hence they should trigger >> the corresponding fanotify (FAN_MODIFY) event. >> >> Prior to the patch only a inotify (IN_MODIFY) event is triggered because >> the path information is not passed to the notification framework. >> >> The patch changes the interface of fsnotify_change to allow path information >> to be passed. Fsnotify_change is only called by notify_change and by >> fat_ioctl_set_attributes. >> >> notify_change is called by a larger number of callers. Not all of these >> have access to path object. To limit the number of changes the patch renames >> notify_change to do_notify_change and adds a path parameter. The patch further >> adds two wrappers: notify_change for callers that cannot provide a path >> and notify_change_path for callers that can provide a path. >> >> The patch changes do_truncate to accept a path parameter passed to >> notify_change_path and adjusts all callers of do_truncate. > So what I somewhat dislike about this patch is that notify_change() is > sometimes called with dentry and sometimes with path. That way it's not > completely clear when fanotify events will be generated and when not. > Sadly it isn't easy to provide struct path in all the places where we are > calling notify_change() so I'm not sure what would a better solution look > like either :( > > Honza >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> fs/attr.c | 35 ++++++++++++++++++++++++++++++++--- >> fs/fat/file.c | 2 +- >> fs/namei.c | 2 +- >> fs/open.c | 10 ++++++---- >> include/linux/fs.h | 3 ++- >> include/linux/fsnotify.h | 12 +++++++++--- >> 6 files changed, 51 insertions(+), 13 deletions(-) >> >> diff --git a/fs/attr.c b/fs/attr.c >> index 6530ced..894967e 100644 >> --- a/fs/attr.c >> +++ b/fs/attr.c >> @@ -168,13 +168,17 @@ void setattr_copy(struct inode *inode, const struct iattr *attr) >> EXPORT_SYMBOL(setattr_copy); >> >> /** >> - * notify_change - modify attributes of a filesytem object >> + * do_notify_change - modify attributes of a filesytem object >> + * @path: path of object affected >> * @dentry: object affected >> * @iattr: new attributes >> * @delegated_inode: returns inode, if the inode is delegated >> * >> * The caller must hold the i_mutex on the affected object. >> * >> + * If NULL is passed in attribute path, fanotify events cannot be >> + * created. >> + * >> * If notify_change discovers a delegation in need of breaking, >> * it will return -EWOULDBLOCK and return a reference to the inode in >> * delegated_inode. The caller should then break the delegation and >> @@ -187,7 +191,8 @@ EXPORT_SYMBOL(setattr_copy); >> * the file open for write, as there can be no conflicting delegation in >> * that case. >> */ >> -int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **delegated_inode) >> +int do_notify_change(struct path *path, struct dentry *dentry, >> + struct iattr *attr, struct inode **delegated_inode) >> { >> struct inode *inode = dentry->d_inode; >> umode_t mode = inode->i_mode; >> @@ -268,11 +273,35 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de >> error = simple_setattr(dentry, attr); >> >> if (!error) { >> - fsnotify_change(dentry, ia_valid); >> + fsnotify_change(path, dentry, ia_valid); >> ima_inode_post_setattr(dentry); >> evm_inode_post_setattr(dentry, ia_valid); >> } >> >> return error; >> } >> + >> +/* >> + * notify_change - modify attributes of a filesytem object >> + * >> + * This call will not create fanotify events. It should be replaced by >> + * notify_change_path where possible. >> + */ >> +int notify_change(struct dentry *dentry, struct iattr *attr, >> + struct inode **delegated_inode) >> +{ >> + return do_notify_change(NULL, dentry, attr, delegated_inode); >> +} >> EXPORT_SYMBOL(notify_change); >> + >> +/* >> + * notify_change_path - modify attributes of a filesytem object >> + * >> + * This call is a replacement for notify_change. It creates fanotify events. >> + */ >> +int notify_change_path(struct path *path, struct iattr *attr, >> + struct inode **delegated_inode) >> +{ >> + return do_notify_change(path, path->dentry, attr, delegated_inode); >> +} >> +EXPORT_SYMBOL(notify_change_path); >> diff --git a/fs/fat/file.c b/fs/fat/file.c >> index f2c73ae..d293a0b 100644 >> --- a/fs/fat/file.c >> +++ b/fs/fat/file.c >> @@ -101,7 +101,7 @@ static int fat_ioctl_set_attributes(struct file *file, u32 __user *user_attr) >> if (err) >> goto out_unlock_inode; >> >> - fsnotify_change(file->f_path.dentry, ia.ia_valid); >> + fsnotify_change(&file->f_path, file->f_path.dentry, ia.ia_valid); >> if (sbi->options.sys_immutable) { >> if (attr & ATTR_SYS) >> inode->i_flags |= S_IMMUTABLE; >> diff --git a/fs/namei.c b/fs/namei.c >> index 43927d1..0a1f3b7 100644 >> --- a/fs/namei.c >> +++ b/fs/namei.c >> @@ -2603,7 +2603,7 @@ static int handle_truncate(struct file *filp) >> if (!error) >> error = security_path_truncate(path); >> if (!error) { >> - error = do_truncate(path->dentry, 0, >> + error = do_truncate(path, 0, >> ATTR_MTIME|ATTR_CTIME|ATTR_OPEN, >> filp); >> } >> diff --git a/fs/open.c b/fs/open.c >> index d6fd3ac..390e322 100644 >> --- a/fs/open.c >> +++ b/fs/open.c >> @@ -34,11 +34,12 @@ >> >> #include "internal.h" >> >> -int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs, >> +int do_truncate(struct path *path, loff_t length, unsigned int time_attrs, >> struct file *filp) >> { >> int ret; >> struct iattr newattrs; >> + struct dentry *dentry = path->dentry; >> >> /* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */ >> if (length < 0) >> @@ -58,7 +59,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs, >> >> mutex_lock(&dentry->d_inode->i_mutex); >> /* Note any delegations or leases have already been broken: */ >> - ret = notify_change(dentry, &newattrs, NULL); >> + ret = notify_change_path(path, &newattrs, NULL); >> mutex_unlock(&dentry->d_inode->i_mutex); >> return ret; >> } >> @@ -104,7 +105,7 @@ long vfs_truncate(struct path *path, loff_t length) >> if (!error) >> error = security_path_truncate(path); >> if (!error) >> - error = do_truncate(path->dentry, length, 0, NULL); >> + error = do_truncate(path, length, 0, NULL); >> >> put_write_and_out: >> put_write_access(inode); >> @@ -188,7 +189,8 @@ static long do_sys_ftruncate(unsigned int fd, loff_t length, int small) >> if (!error) >> error = security_path_truncate(&f.file->f_path); >> if (!error) >> - error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, f.file); >> + error = do_truncate(&f.file->f_path, length, >> + ATTR_MTIME|ATTR_CTIME, f.file); >> sb_end_write(inode->i_sb); >> out_putf: >> fdput(f); >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index 4111ee0..e5b0b05 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -2031,7 +2031,7 @@ struct filename { >> }; >> >> extern long vfs_truncate(struct path *, loff_t); >> -extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs, >> +extern int do_truncate(struct path *, loff_t start, unsigned int time_attrs, >> struct file *filp); >> extern int do_fallocate(struct file *file, int mode, loff_t offset, >> loff_t len); >> @@ -2253,6 +2253,7 @@ extern void emergency_remount(void); >> extern sector_t bmap(struct inode *, sector_t); >> #endif >> extern int notify_change(struct dentry *, struct iattr *, struct inode **); >> +extern int notify_change_path(struct path *, struct iattr *, struct inode **); >> extern int inode_permission(struct inode *, int); >> extern int generic_permission(struct inode *, int); >> >> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h >> index 1c804b0..0fd594b 100644 >> --- a/include/linux/fsnotify.h >> +++ b/include/linux/fsnotify.h >> @@ -276,7 +276,8 @@ static inline void fsnotify_xattr(struct dentry *dentry) >> * fsnotify_change - notify_change event. file was modified and/or metadata >> * was changed. >> */ >> -static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid) >> +static inline void fsnotify_change(struct path *path, struct dentry *dentry, >> + unsigned int ia_valid) >> { >> struct inode *inode = dentry->d_inode; >> __u32 mask = 0; >> @@ -303,8 +304,13 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid) >> if (S_ISDIR(inode->i_mode)) >> mask |= FS_ISDIR; >> >> - fsnotify_parent(NULL, dentry, mask); >> - fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0); >> + fsnotify_parent(path, dentry, mask); >> + if (path == NULL) >> + fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, >> + NULL, 0); >> + else >> + fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, >> + NULL, 0); >> } >> } >> >> -- >> 2.1.1 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] ftruncate, truncate: create fanotify events 2014-11-10 20:30 ` Jan Kara 2014-11-10 23:10 ` Heinrich Schuchardt @ 2014-11-11 7:34 ` Christoph Hellwig 2014-11-11 11:09 ` Jan Kara 1 sibling, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2014-11-11 7:34 UTC (permalink / raw) To: Jan Kara Cc: Heinrich Schuchardt, Eric Paris, Alexander Viro, OGAWA Hirofumi, Jeff Layton, J. Bruce Fields, linux-fsdevel, linux-kernel On Mon, Nov 10, 2014 at 09:30:29PM +0100, Jan Kara wrote: > So what I somewhat dislike about this patch is that notify_change() is > sometimes called with dentry and sometimes with path. That way it's not > completely clear when fanotify events will be generated and when not. > Sadly it isn't easy to provide struct path in all the places where we are > calling notify_change() so I'm not sure what would a better solution look > like either :( I suspect the right thing to do is to split out the truncate path from notify_change, as it's fairly different anyway. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] ftruncate, truncate: create fanotify events 2014-11-11 7:34 ` Christoph Hellwig @ 2014-11-11 11:09 ` Jan Kara 2014-11-11 19:55 ` Heinrich Schuchardt 0 siblings, 1 reply; 12+ messages in thread From: Jan Kara @ 2014-11-11 11:09 UTC (permalink / raw) To: Christoph Hellwig Cc: Jan Kara, Heinrich Schuchardt, Eric Paris, Alexander Viro, OGAWA Hirofumi, Jeff Layton, J. Bruce Fields, linux-fsdevel, linux-kernel On Mon 10-11-14 23:34:15, Christoph Hellwig wrote: > On Mon, Nov 10, 2014 at 09:30:29PM +0100, Jan Kara wrote: > > So what I somewhat dislike about this patch is that notify_change() is > > sometimes called with dentry and sometimes with path. That way it's not > > completely clear when fanotify events will be generated and when not. > > Sadly it isn't easy to provide struct path in all the places where we are > > calling notify_change() so I'm not sure what would a better solution look > > like either :( > > I suspect the right thing to do is to split out the truncate path > from notify_change, as it's fairly different anyway. Yeah, that would make sense. I wanted to say it's quite a lot of work to change all the filesystems (where the separation of truncate path makes sense as well IMHO) but actually it's possible to just do the separation at the VFS level and still call ->setattr() fs callback for now. Heinrich will you look into this? Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] ftruncate, truncate: create fanotify events 2014-11-11 11:09 ` Jan Kara @ 2014-11-11 19:55 ` Heinrich Schuchardt 2014-11-14 10:01 ` Jan Kara 0 siblings, 1 reply; 12+ messages in thread From: Heinrich Schuchardt @ 2014-11-11 19:55 UTC (permalink / raw) To: Jan Kara, Christoph Hellwig Cc: Eric Paris, Alexander Viro, OGAWA Hirofumi, Jeff Layton, J. Bruce Fields, linux-fsdevel, linux-kernel On 11.11.2014 12:09, Jan Kara wrote: > On Mon 10-11-14 23:34:15, Christoph Hellwig wrote: >> On Mon, Nov 10, 2014 at 09:30:29PM +0100, Jan Kara wrote: >>> So what I somewhat dislike about this patch is that notify_change() is >>> sometimes called with dentry and sometimes with path. That way it's not >>> completely clear when fanotify events will be generated and when not. No. With my patch notify_change is still only called with dentry. It is only new function notify_change_path which will be called with a path. And this function will only be called from do_truncate up to now. >>> Sadly it isn't easy to provide struct path in all the places where we are >>> calling notify_change() so I'm not sure what would a better solution look >>> like either :( We only want to create FAN_MODIFY events for ATTR_SIZE. So only for these events we need a path. To my knowledge notify_change is called with ATTR_SIZE from do_truncate(), ecryptfs_truncate() and will be called with ATTR_SIZE from ovl_setattr() for a truncation. ecryptfs_dentry_to_lower_path() could be used in ecryptfs_truncate() to obtain a path. ovl_path_upper() could be used in ovl_setattr() to obtain a path. >> >> I suspect the right thing to do is to split out the truncate path >> from notify_change, as it's fairly different anyway. > Yeah, that would make sense. I wanted to say it's quite a lot of work to > change all the filesystems (where the separation of truncate path makes > sense as well IMHO) but actually it's possible to just do the separation at > the VFS level and still call ->setattr() fs callback for now. Heinrich will > you look into this? You seem to agree that struct path has to passed to do_truncate() and further to the notification layer. Currently do_truncate() calls notify_change() which does not accept a path argument. Here the size change is implemented as an attribute change. Furthermore time attributes are changed. What do you exactly mean by > just do the separation at the virtual file system level > and still call ->setattr() fs callback for now. Do you want to duplicate the logic of notify_change() to a new function notify_truncate() which will at least have to handle ATTR_SIZE, ATTR_FORCE, ATTR_KILL_SUID, ATTR_KILL_SGID, ATTR_FILE and time attributes? And from notify_truncate() call new function fsnotify_truncate() with a logic like fsnotify_modify() but accepting a path? This would result in a lot of code duplication. In which respect would such a patch be preferable? Best regards Heinrich Schuchardt ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] ftruncate, truncate: create fanotify events 2014-11-11 19:55 ` Heinrich Schuchardt @ 2014-11-14 10:01 ` Jan Kara 2014-11-14 21:30 ` Heinrich Schuchardt 0 siblings, 1 reply; 12+ messages in thread From: Jan Kara @ 2014-11-14 10:01 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Jan Kara, Christoph Hellwig, Eric Paris, Alexander Viro, OGAWA Hirofumi, Jeff Layton, J. Bruce Fields, linux-fsdevel, linux-kernel On Tue 11-11-14 20:55:26, Heinrich Schuchardt wrote: > On 11.11.2014 12:09, Jan Kara wrote: > >On Mon 10-11-14 23:34:15, Christoph Hellwig wrote: > >>On Mon, Nov 10, 2014 at 09:30:29PM +0100, Jan Kara wrote: > >>> So what I somewhat dislike about this patch is that notify_change() is > >>>sometimes called with dentry and sometimes with path. That way it's not > >>>completely clear when fanotify events will be generated and when not. > > No. With my patch notify_change is still only called with dentry. > It is only new function notify_change_path which will be called with a path. > And this function will only be called from do_truncate up to now. > > >>>Sadly it isn't easy to provide struct path in all the places where we are > >>>calling notify_change() so I'm not sure what would a better solution look > >>>like either :( > > We only want to create FAN_MODIFY events for ATTR_SIZE. So only for > these events we need a path. > > To my knowledge notify_change is called with ATTR_SIZE from > do_truncate(), ecryptfs_truncate() and will be called with ATTR_SIZE > from ovl_setattr() for a truncation. There's also a call in fs/cachefiles/interface.c: cachefiles_attr_changed() and fs/hpfs/namei.c: hpfs_unlink() and nfs plays with ATTR_SIZE although I wasn't able to track down whether it actually passes it to notify_change(). So I agree you have covered almost all the cases. But it's just prone to errors (as you can see when you missed some cases) because it isn't clear to callers of notify_change() whether and why they should provide the path via notify_change_path() or not. But see below. > ecryptfs_dentry_to_lower_path() could be used in ecryptfs_truncate() > to obtain a path. > ovl_path_upper() could be used in ovl_setattr() to obtain a path. Agreed, this can and should be done. > >>I suspect the right thing to do is to split out the truncate path > >>from notify_change, as it's fairly different anyway. > > Yeah, that would make sense. I wanted to say it's quite a lot of work to > >change all the filesystems (where the separation of truncate path makes > >sense as well IMHO) but actually it's possible to just do the separation at > >the VFS level and still call ->setattr() fs callback for now. Heinrich will > >you look into this? > > You seem to agree that struct path has to passed to do_truncate() > and further to the notification layer. Yes. > Currently do_truncate() calls notify_change() which does not accept > a path argument. > > Here the size change is implemented as an attribute change. > Furthermore time attributes are changed. > > What do you exactly mean by > > just do the separation at the virtual file system level > > and still call ->setattr() fs callback for now. > > Do you want to duplicate the logic of notify_change() to a > new function notify_truncate() which will at least have to handle > ATTR_SIZE, ATTR_FORCE, ATTR_KILL_SUID, ATTR_KILL_SGID, ATTR_FILE and > time attributes? > > And from notify_truncate() call new function fsnotify_truncate() > with a logic like fsnotify_modify() but accepting a path? > > This would result in a lot of code duplication. Yes, I forgot about other changes coupled with truncate and you're right that duplicating them would do more bad than good. But what we could do is to just have __notify_change() which takes both path & dentry as you did it. notify_change() which takes just dentry and verifies ATTR_SIZE is not set (WARN_ON_ONCE if it is). notify_change_truncate() which takes path. This still isn't pretty but at least makes errors harder to do. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] ftruncate, truncate: create fanotify events 2014-11-14 10:01 ` Jan Kara @ 2014-11-14 21:30 ` Heinrich Schuchardt 0 siblings, 0 replies; 12+ messages in thread From: Heinrich Schuchardt @ 2014-11-14 21:30 UTC (permalink / raw) To: Jan Kara Cc: Christoph Hellwig, Eric Paris, Alexander Viro, OGAWA Hirofumi, Jeff Layton, J. Bruce Fields, linux-fsdevel, linux-kernel Hello Jan, thank you for the review. On 14.11.2014 11:01, Jan Kara wrote: >> >> We only want to create FAN_MODIFY events for ATTR_SIZE. So only for >> these events we need a path. >> >> To my knowledge notify_change is called with ATTR_SIZE from >> do_truncate(), ecryptfs_truncate() and will be called with ATTR_SIZE >> from ovl_setattr() for a truncation. > There's also a call in fs/cachefiles/interface.c: cachefiles_attr_changed() > and fs/hpfs/namei.c: hpfs_unlink() and nfs plays with ATTR_SIZE although I > wasn't able to track down whether it actually passes it to notify_change(). cachefiles_attr_changed() contains two calls to notify_change(). Both calls pass ATTR_SIZE as sole attribute. In cachefiles_attr_changed() we can create a path object with path.dentry = object->backer; path.mnt = cache->mnt; In hpfs_unlink() a truncation occurs only if deletion fails. I do not see how to access a path here. notify_change() is not called in directory fs/nfs. grep -GHrn notify_change fs/nfs/ This does not test for any indirect call. Best regards Heinrich Schuchardt ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-11-14 21:36 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-10-03 18:16 [PATCH 1/1] ftruncate: create FAN_MODIFY and IN_MODIFY events Heinrich Schuchardt 2014-10-06 13:24 ` Jan Kara 2014-10-06 20:09 ` Heinrich Schuchardt 2014-10-07 19:23 ` Jan Kara 2014-10-23 21:35 ` [PATCH v2 1/1] ftruncate, truncate: create fanotify events Heinrich Schuchardt 2014-11-10 20:30 ` Jan Kara 2014-11-10 23:10 ` Heinrich Schuchardt 2014-11-11 7:34 ` Christoph Hellwig 2014-11-11 11:09 ` Jan Kara 2014-11-11 19:55 ` Heinrich Schuchardt 2014-11-14 10:01 ` Jan Kara 2014-11-14 21:30 ` Heinrich Schuchardt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).