| Message ID | 20190501205541.GC30899@veci.piliscsaba.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] network fs notification | expand |
On Wed 01-05-19 16:55:41, Miklos Szeredi wrote: > This is a really really trivial first iteration, but I think it's enough to > try out CIFS notification support. Doesn't deal with mark deletion, but > that's best effort anyway: fsnotify() will filter out unneeded events. > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > fs/notify/fanotify/fanotify_user.c | 6 +++++- > fs/notify/inotify/inotify_user.c | 2 ++ > include/linux/fs.h | 1 + > 3 files changed, 8 insertions(+), 1 deletion(-) > > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1041,9 +1041,13 @@ static int do_fanotify_mark(int fanotify > else if (mark_type == FAN_MARK_FILESYSTEM) > ret = fanotify_add_sb_mark(group, mnt->mnt_sb, mask, > flags, fsid); > - else > + else { > ret = fanotify_add_inode_mark(group, inode, mask, > flags, fsid); > + > + if (!ret && inode->i_op->notify_update) > + inode->i_op->notify_update(inode); > + } Yeah, so I had something like this in mind but I wanted to inform the filesystem about superblock and mountpoint marks as well. And I'd pass the 'mask' as well as presumably filesystem could behave differently depending on whether we are looking for create vs unlink vs file change events etc... Honza
On Thu, May 2, 2019 at 10:39 AM Jan Kara <jack@suse.cz> wrote: > > On Wed 01-05-19 16:55:41, Miklos Szeredi wrote: > > This is a really really trivial first iteration, but I think it's enough to > > try out CIFS notification support. Doesn't deal with mark deletion, but > > that's best effort anyway: fsnotify() will filter out unneeded events. > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > --- > > fs/notify/fanotify/fanotify_user.c | 6 +++++- > > fs/notify/inotify/inotify_user.c | 2 ++ > > include/linux/fs.h | 1 + > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -1041,9 +1041,13 @@ static int do_fanotify_mark(int fanotify > > else if (mark_type == FAN_MARK_FILESYSTEM) > > ret = fanotify_add_sb_mark(group, mnt->mnt_sb, mask, > > flags, fsid); > > - else > > + else { > > ret = fanotify_add_inode_mark(group, inode, mask, > > flags, fsid); > > + > > + if (!ret && inode->i_op->notify_update) > > + inode->i_op->notify_update(inode); > > + } > > Yeah, so I had something like this in mind but I wanted to inform the > filesystem about superblock and mountpoint marks as well. And I'd pass the > 'mask' as well as presumably filesystem could behave differently depending > on whether we are looking for create vs unlink vs file change events etc... > It probably wouldn't hurt to update fs about mount marks, but in the context of "remote" fs, the changes are most certainly being done on a different mount, a different machine most likely... Thanks, Amir.
On Thu 02-05-19 11:08:41, Amir Goldstein wrote: > On Thu, May 2, 2019 at 10:39 AM Jan Kara <jack@suse.cz> wrote: > > > > On Wed 01-05-19 16:55:41, Miklos Szeredi wrote: > > > This is a really really trivial first iteration, but I think it's enough to > > > try out CIFS notification support. Doesn't deal with mark deletion, but > > > that's best effort anyway: fsnotify() will filter out unneeded events. > > > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > > --- > > > fs/notify/fanotify/fanotify_user.c | 6 +++++- > > > fs/notify/inotify/inotify_user.c | 2 ++ > > > include/linux/fs.h | 1 + > > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > > > --- a/fs/notify/fanotify/fanotify_user.c > > > +++ b/fs/notify/fanotify/fanotify_user.c > > > @@ -1041,9 +1041,13 @@ static int do_fanotify_mark(int fanotify > > > else if (mark_type == FAN_MARK_FILESYSTEM) > > > ret = fanotify_add_sb_mark(group, mnt->mnt_sb, mask, > > > flags, fsid); > > > - else > > > + else { > > > ret = fanotify_add_inode_mark(group, inode, mask, > > > flags, fsid); > > > + > > > + if (!ret && inode->i_op->notify_update) > > > + inode->i_op->notify_update(inode); > > > + } > > > > Yeah, so I had something like this in mind but I wanted to inform the > > filesystem about superblock and mountpoint marks as well. And I'd pass the > > 'mask' as well as presumably filesystem could behave differently depending > > on whether we are looking for create vs unlink vs file change events etc... > > > > It probably wouldn't hurt to update fs about mount marks, > but in the context of "remote" fs, the changes are most certainly > being done on a different mount, a different machine most likely... I agree. I guess I'm missing your point :) What I understood from Steve is that e.g. cifs could ask the server to provide the notifications. E.g. FUSE could propagate this information to userspace daemon which could place appropriate fsnotify marks on underlying objects and then transform the events to events on the FUSE filesystem? At least that's what I was imagining, didn't think too much about it. Honza
On Thu, May 2, 2019 at 11:41 AM Jan Kara <jack@suse.cz> wrote: > > On Thu 02-05-19 11:08:41, Amir Goldstein wrote: > > On Thu, May 2, 2019 at 10:39 AM Jan Kara <jack@suse.cz> wrote: > > > > > > On Wed 01-05-19 16:55:41, Miklos Szeredi wrote: > > > > This is a really really trivial first iteration, but I think it's enough to > > > > try out CIFS notification support. Doesn't deal with mark deletion, but > > > > that's best effort anyway: fsnotify() will filter out unneeded events. > > > > > > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > > > --- > > > > fs/notify/fanotify/fanotify_user.c | 6 +++++- > > > > fs/notify/inotify/inotify_user.c | 2 ++ > > > > include/linux/fs.h | 1 + > > > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > > > > > --- a/fs/notify/fanotify/fanotify_user.c > > > > +++ b/fs/notify/fanotify/fanotify_user.c > > > > @@ -1041,9 +1041,13 @@ static int do_fanotify_mark(int fanotify > > > > else if (mark_type == FAN_MARK_FILESYSTEM) > > > > ret = fanotify_add_sb_mark(group, mnt->mnt_sb, mask, > > > > flags, fsid); > > > > - else > > > > + else { > > > > ret = fanotify_add_inode_mark(group, inode, mask, > > > > flags, fsid); > > > > + > > > > + if (!ret && inode->i_op->notify_update) > > > > + inode->i_op->notify_update(inode); > > > > + } > > > > > > Yeah, so I had something like this in mind but I wanted to inform the > > > filesystem about superblock and mountpoint marks as well. And I'd pass the > > > 'mask' as well as presumably filesystem could behave differently depending > > > on whether we are looking for create vs unlink vs file change events etc... Hmm. I'm not sure we need to pass the mask, since it's in the inode filesystem can read it. But this code is completely racy in this respect and doesn't even include hooks in the delete notification. So it's basically just to try it out. > > > > It probably wouldn't hurt to update fs about mount marks, > > but in the context of "remote" fs, the changes are most certainly > > being done on a different mount, a different machine most likely... > > I agree. I guess I'm missing your point :) What I understood from Steve is > that e.g. cifs could ask the server to provide the notifications. E.g. FUSE > could propagate this information to userspace daemon which could place > appropriate fsnotify marks on underlying objects and then transform the > events to events on the FUSE filesystem? At least that's what I was > imagining, didn't think too much about it. Exactly. For inode and superblock notification that's clear (don't know if CIFS can do superblock notifications or not). However, mount notification is something that possibly means: notify me of any changes made through this mount only. I.e. if there's a bind mount and the filesystem is modified through that, then we don't get the mount notification for the original mount. So I guess it makes sense to say: if you want remote notifications, just use the superblock notification. Thanks, Miklos
On Wed, May 01, 2019 at 04:55:41PM -0400, Miklos Szeredi wrote: > This is a really really trivial first iteration, but I think it's enough to > try out CIFS notification support. Doesn't deal with mark deletion, but > that's best effort anyway: fsnotify() will filter out unneeded events. And this one actually does something for inotify. I haven't tested fanotify yet, but that one looks okay. Note: FAN_MARK_MOUNT doesn't work yet, and we are not sure if it should work or not. FAN_MARK_FILESYSTEM would be a better candidate for remote notification, since remote accesses are not associated with any particular local mount of the filesystem. But perhaps we need to turn on whole file notification for remote due to the fact that applications rely on FAN_MARK_MOUNT already... Btw, does the smb protocol support whole filesystem (or subtree) notifications? Thanks, Miklos --- fs/notify/fanotify/fanotify_user.c | 6 +++++- fs/notify/inotify/inotify_user.c | 2 ++ include/linux/fs.h | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -1041,9 +1041,13 @@ static int do_fanotify_mark(int fanotify else if (mark_type == FAN_MARK_FILESYSTEM) ret = fanotify_add_sb_mark(group, mnt->mnt_sb, mask, flags, fsid); - else + else { ret = fanotify_add_inode_mark(group, inode, mask, flags, fsid); + + if (!ret && inode->i_op->notify_update) + inode->i_op->notify_update(inode); + } break; case FAN_MARK_REMOVE: if (mark_type == FAN_MARK_MOUNT) --- a/fs/notify/inotify/inotify_user.c +++ b/fs/notify/inotify/inotify_user.c @@ -754,6 +754,8 @@ SYSCALL_DEFINE3(inotify_add_watch, int, /* create/update an inode mark */ ret = inotify_update_watch(group, inode, mask); + if (ret >= 0 && inode->i_op->notify_update) + inode->i_op->notify_update(inode); path_put(&path); fput_and_out: fdput(f); --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1852,6 +1852,7 @@ struct inode_operations { umode_t create_mode); int (*tmpfile) (struct inode *, struct dentry *, umode_t); int (*set_acl)(struct inode *, struct posix_acl *, int); + void (*notify_update)(struct inode *inode); } ____cacheline_aligned; static inline ssize_t call_read_iter(struct file *file, struct kiocb *kio,
--- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -1041,9 +1041,13 @@ static int do_fanotify_mark(int fanotify else if (mark_type == FAN_MARK_FILESYSTEM) ret = fanotify_add_sb_mark(group, mnt->mnt_sb, mask, flags, fsid); - else + else { ret = fanotify_add_inode_mark(group, inode, mask, flags, fsid); + + if (!ret && inode->i_op->notify_update) + inode->i_op->notify_update(inode); + } break; case FAN_MARK_REMOVE: if (mark_type == FAN_MARK_MOUNT) --- a/fs/notify/inotify/inotify_user.c +++ b/fs/notify/inotify/inotify_user.c @@ -754,6 +754,8 @@ SYSCALL_DEFINE3(inotify_add_watch, int, /* create/update an inode mark */ ret = inotify_update_watch(group, inode, mask); + if (!ret && inode->i_op->notify_update) + inode->i_op->notify_update(inode); path_put(&path); fput_and_out: fdput(f); --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1852,6 +1852,7 @@ struct inode_operations { umode_t create_mode); int (*tmpfile) (struct inode *, struct dentry *, umode_t); int (*set_acl)(struct inode *, struct posix_acl *, int); + void (*notify_update)(struct inode *inode); } ____cacheline_aligned; static inline ssize_t call_read_iter(struct file *file, struct kiocb *kio,
This is a really really trivial first iteration, but I think it's enough to try out CIFS notification support. Doesn't deal with mark deletion, but that's best effort anyway: fsnotify() will filter out unneeded events. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/notify/fanotify/fanotify_user.c | 6 +++++- fs/notify/inotify/inotify_user.c | 2 ++ include/linux/fs.h | 1 + 3 files changed, 8 insertions(+), 1 deletion(-)