* [PATCH] fs: don't remove inotify watchers from alive inode-s @ 2014-09-08 12:01 Andrey Vagin 2014-09-08 12:19 ` Cyrill Gorcunov ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Andrey Vagin @ 2014-09-08 12:01 UTC (permalink / raw) To: linux-fsdevel Cc: linux-kernel, Andrey Vagin, Alexander Viro, John McCutchan, Robert Love, Eric Paris, Cyrill Gorcunov, Pavel Emelyanov Currently watchers are removed in dentry_iput(), if n_link is zero. But other detries can be linked with this inode. For example if we create two hard links, open the first one and set a watcher on the second one. Then if we remove both links, the watcher will be removed. But we will have the alive file descriptor, which allows us to generate more events. With this patch, watchers will be removed, only if nlink is zero and i_dentry list is empty. Look at a following example: fd = inotify_init1(IN_NONBLOCK); deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); link(path, path_link); wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); unlink(path); unlink(path_link); printf(" --- unlink\n"); read_evetns(fd); close(deleted); printf(" --- close\n"); read_evetns(fd); Without this patch: --- unlink 4 (IN_ATTRIB) 400 (IN_DELETE_SELF) 8000 (IN_IGNORED) --- close FAIL With this patch: --- unlink 4 (IN_ATTRIB) 400 (IN_DELETE_SELF) --- close 8 (IN_CLOSE_WRITE) 400 (IN_DELETE_SELF) 8000 (IN_IGNORED) PASS Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: John McCutchan <john@johnmccutchan.com> Cc: Robert Love <rlove@rlove.org> Cc: Eric Paris <eparis@parisplace.org> Cc: Cyrill Gorcunov <gorcunov@openvz.org> Cc: Pavel Emelyanov <xemul@parallels.com> Signed-off-by: Andrey Vagin <avagin@openvz.org> --- fs/dcache.c | 10 ++++++++-- include/linux/fsnotify.h | 5 +++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index d30ce69..fb1ee58 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -279,13 +279,16 @@ static void dentry_iput(struct dentry * dentry) __releases(dentry->d_inode->i_lock) { struct inode *inode = dentry->d_inode; + bool last_dentry; + if (inode) { dentry->d_inode = NULL; hlist_del_init(&dentry->d_alias); + last_dentry = hlist_empty(&inode->i_dentry); spin_unlock(&dentry->d_lock); spin_unlock(&inode->i_lock); if (!inode->i_nlink) - fsnotify_inoderemove(inode); + fsnotify_inoderemove(inode, last_dentry); if (dentry->d_op && dentry->d_op->d_iput) dentry->d_op->d_iput(dentry, inode); else @@ -304,14 +307,17 @@ static void dentry_unlink_inode(struct dentry * dentry) __releases(dentry->d_inode->i_lock) { struct inode *inode = dentry->d_inode; + bool last_dentry; + __d_clear_type(dentry); dentry->d_inode = NULL; hlist_del_init(&dentry->d_alias); dentry_rcuwalk_barrier(dentry); + last_dentry = hlist_empty(&inode->i_dentry); spin_unlock(&dentry->d_lock); spin_unlock(&inode->i_lock); if (!inode->i_nlink) - fsnotify_inoderemove(inode); + fsnotify_inoderemove(inode, last_dentry); if (dentry->d_op && dentry->d_op->d_iput) dentry->d_op->d_iput(dentry, inode); else diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index 1c804b0..63dae9d 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -144,10 +144,11 @@ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir) /* * fsnotify_inoderemove - an inode is going away */ -static inline void fsnotify_inoderemove(struct inode *inode) +static inline void fsnotify_inoderemove(struct inode *inode, bool delete) { fsnotify(inode, FS_DELETE_SELF, inode, FSNOTIFY_EVENT_INODE, NULL, 0); - __fsnotify_inode_delete(inode); + if (delete) + __fsnotify_inode_delete(inode); } /* -- 1.9.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] fs: don't remove inotify watchers from alive inode-s 2014-09-08 12:01 [PATCH] fs: don't remove inotify watchers from alive inode-s Andrey Vagin @ 2014-09-08 12:19 ` Cyrill Gorcunov 2014-09-08 14:45 ` Jan Kara 2014-09-09 1:27 ` Al Viro 2 siblings, 0 replies; 10+ messages in thread From: Cyrill Gorcunov @ 2014-09-08 12:19 UTC (permalink / raw) To: Andrey Vagin Cc: linux-fsdevel, linux-kernel, Alexander Viro, John McCutchan, Robert Love, Eric Paris, Pavel Emelyanov On Mon, Sep 08, 2014 at 04:01:56PM +0400, Andrey Vagin wrote: > Currently watchers are removed in dentry_iput(), if n_link is zero. > But other detries can be linked with this inode. For example if we > create two hard links, open the first one and set a watcher on the > second one. Then if we remove both links, the watcher will be removed. > But we will have the alive file descriptor, which allows us to generate > more events. > > With this patch, watchers will be removed, only if nlink is zero and > i_dentry list is empty. This changes the sequence of notifies userspace will see but I think there were a bug before this patch, so Reviewed-by: Cyrill Gorcunov <gorcunov@openvz.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fs: don't remove inotify watchers from alive inode-s 2014-09-08 12:01 [PATCH] fs: don't remove inotify watchers from alive inode-s Andrey Vagin 2014-09-08 12:19 ` Cyrill Gorcunov @ 2014-09-08 14:45 ` Jan Kara 2014-09-09 1:27 ` Al Viro 2 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2014-09-08 14:45 UTC (permalink / raw) To: Andrey Vagin Cc: linux-fsdevel, linux-kernel, Alexander Viro, John McCutchan, Robert Love, Eric Paris, Cyrill Gorcunov, Pavel Emelyanov On Mon 08-09-14 16:01:56, Andrey Vagin wrote: > Currently watchers are removed in dentry_iput(), if n_link is zero. > But other detries can be linked with this inode. For example if we > create two hard links, open the first one and set a watcher on the > second one. Then if we remove both links, the watcher will be removed. > But we will have the alive file descriptor, which allows us to generate > more events. > > With this patch, watchers will be removed, only if nlink is zero and > i_dentry list is empty. > > Look at a following example: > > fd = inotify_init1(IN_NONBLOCK); > deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); > link(path, path_link); > > wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); > > unlink(path); > unlink(path_link); > > printf(" --- unlink\n"); > read_evetns(fd); > > close(deleted); > printf(" --- close\n"); > read_evetns(fd); > > Without this patch: > --- unlink > 4 (IN_ATTRIB) > 400 (IN_DELETE_SELF) > 8000 (IN_IGNORED) > --- close > FAIL > > With this patch: > --- unlink > 4 (IN_ATTRIB) > 400 (IN_DELETE_SELF) > --- close > 8 (IN_CLOSE_WRITE) > 400 (IN_DELETE_SELF) > 8000 (IN_IGNORED) > PASS > > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: John McCutchan <john@johnmccutchan.com> > Cc: Robert Love <rlove@rlove.org> > Cc: Eric Paris <eparis@parisplace.org> > Cc: Cyrill Gorcunov <gorcunov@openvz.org> > Cc: Pavel Emelyanov <xemul@parallels.com> > Signed-off-by: Andrey Vagin <avagin@openvz.org> OK, looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/dcache.c | 10 ++++++++-- > include/linux/fsnotify.h | 5 +++-- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/fs/dcache.c b/fs/dcache.c > index d30ce69..fb1ee58 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -279,13 +279,16 @@ static void dentry_iput(struct dentry * dentry) > __releases(dentry->d_inode->i_lock) > { > struct inode *inode = dentry->d_inode; > + bool last_dentry; > + > if (inode) { > dentry->d_inode = NULL; > hlist_del_init(&dentry->d_alias); > + last_dentry = hlist_empty(&inode->i_dentry); > spin_unlock(&dentry->d_lock); > spin_unlock(&inode->i_lock); > if (!inode->i_nlink) > - fsnotify_inoderemove(inode); > + fsnotify_inoderemove(inode, last_dentry); > if (dentry->d_op && dentry->d_op->d_iput) > dentry->d_op->d_iput(dentry, inode); > else > @@ -304,14 +307,17 @@ static void dentry_unlink_inode(struct dentry * dentry) > __releases(dentry->d_inode->i_lock) > { > struct inode *inode = dentry->d_inode; > + bool last_dentry; > + > __d_clear_type(dentry); > dentry->d_inode = NULL; > hlist_del_init(&dentry->d_alias); > dentry_rcuwalk_barrier(dentry); > + last_dentry = hlist_empty(&inode->i_dentry); > spin_unlock(&dentry->d_lock); > spin_unlock(&inode->i_lock); > if (!inode->i_nlink) > - fsnotify_inoderemove(inode); > + fsnotify_inoderemove(inode, last_dentry); > if (dentry->d_op && dentry->d_op->d_iput) > dentry->d_op->d_iput(dentry, inode); > else > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > index 1c804b0..63dae9d 100644 > --- a/include/linux/fsnotify.h > +++ b/include/linux/fsnotify.h > @@ -144,10 +144,11 @@ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir) > /* > * fsnotify_inoderemove - an inode is going away > */ > -static inline void fsnotify_inoderemove(struct inode *inode) > +static inline void fsnotify_inoderemove(struct inode *inode, bool delete) > { > fsnotify(inode, FS_DELETE_SELF, inode, FSNOTIFY_EVENT_INODE, NULL, 0); > - __fsnotify_inode_delete(inode); > + if (delete) > + __fsnotify_inode_delete(inode); > } > > /* > -- > 1.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fs: don't remove inotify watchers from alive inode-s 2014-09-08 12:01 [PATCH] fs: don't remove inotify watchers from alive inode-s Andrey Vagin 2014-09-08 12:19 ` Cyrill Gorcunov 2014-09-08 14:45 ` Jan Kara @ 2014-09-09 1:27 ` Al Viro 2014-09-09 8:54 ` Jan Kara 2 siblings, 1 reply; 10+ messages in thread From: Al Viro @ 2014-09-09 1:27 UTC (permalink / raw) To: Andrey Vagin Cc: linux-fsdevel, linux-kernel, John McCutchan, Robert Love, Eric Paris, Cyrill Gorcunov, Pavel Emelyanov On Mon, Sep 08, 2014 at 04:01:56PM +0400, Andrey Vagin wrote: > Currently watchers are removed in dentry_iput(), if n_link is zero. > But other detries can be linked with this inode. For example if we > create two hard links, open the first one and set a watcher on the > second one. Then if we remove both links, the watcher will be removed. > But we will have the alive file descriptor, which allows us to generate > more events. > > With this patch, watchers will be removed, only if nlink is zero and > i_dentry list is empty. It changes user-visible ABI. Worse yet, this "ABI" has no specification, so any (misguided) software using that FPOS has nothing to go by other than "whatever existing kernels do". As the result, inotify behaviour is cast in concrete. IOW, NAK. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fs: don't remove inotify watchers from alive inode-s 2014-09-09 1:27 ` Al Viro @ 2014-09-09 8:54 ` Jan Kara 2014-09-10 9:43 ` Andrew Vagin 0 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2014-09-09 8:54 UTC (permalink / raw) To: Al Viro Cc: Andrey Vagin, linux-fsdevel, linux-kernel, John McCutchan, Robert Love, Eric Paris, Cyrill Gorcunov, Pavel Emelyanov On Tue 09-09-14 02:27:12, Al Viro wrote: > On Mon, Sep 08, 2014 at 04:01:56PM +0400, Andrey Vagin wrote: > > Currently watchers are removed in dentry_iput(), if n_link is zero. > > But other detries can be linked with this inode. For example if we > > create two hard links, open the first one and set a watcher on the > > second one. Then if we remove both links, the watcher will be removed. > > But we will have the alive file descriptor, which allows us to generate > > more events. > > > > With this patch, watchers will be removed, only if nlink is zero and > > i_dentry list is empty. > > It changes user-visible ABI. Worse yet, this "ABI" has no specification, > so any (misguided) software using that FPOS has nothing to go by other than > "whatever existing kernels do". As the result, inotify behaviour is cast > in concrete. I agree that it changes user-visible ABI and I agree the behavior isn't really specified in the manpage. However our position has always been you can change stuff unless someone complains. In this case the current behavior is: If you just unlink opened file, you keep getting inotify events for that inode. Also if you do sequence like: fd = inotify_init1(IN_NONBLOCK); deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); link(path, path_link); wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); unlink(path_link); unlink(path); You will also keep getting inotify events for that inode (the first unlink will naturally just generate ATTRIB event, the second unlink will fail the test: if (dentry->d_lockref.count == 1) { in d_delete() and thus won't call dentry_unlink_inode() which would delete inotify watch. However if you do sequence like: fd = inotify_init1(IN_NONBLOCK); deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); link(path, path_link); wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); unlink(path); /* Note swapped unlink order */ unlink(path_link); You will stop getting inotify events for the inode. That's so stupid and inconsistent (plus the behavior of this already changed in the past as my tests with older kernels show) that I'm convinced noone really depends on this. So I'm for fixing the bug even though it has userspace visible impact. Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fs: don't remove inotify watchers from alive inode-s 2014-09-09 8:54 ` Jan Kara @ 2014-09-10 9:43 ` Andrew Vagin 2014-09-13 16:15 ` Heinrich Schuchardt 0 siblings, 1 reply; 10+ messages in thread From: Andrew Vagin @ 2014-09-10 9:43 UTC (permalink / raw) To: Jan Kara Cc: Al Viro, Andrey Vagin, linux-fsdevel, linux-kernel, John McCutchan, Robert Love, Eric Paris, Cyrill Gorcunov, Pavel Emelyanov On Tue, Sep 09, 2014 at 10:54:39AM +0200, Jan Kara wrote: > On Tue 09-09-14 02:27:12, Al Viro wrote: > > On Mon, Sep 08, 2014 at 04:01:56PM +0400, Andrey Vagin wrote: > > > Currently watchers are removed in dentry_iput(), if n_link is zero. > > > But other detries can be linked with this inode. For example if we > > > create two hard links, open the first one and set a watcher on the > > > second one. Then if we remove both links, the watcher will be removed. > > > But we will have the alive file descriptor, which allows us to generate > > > more events. > > > > > > With this patch, watchers will be removed, only if nlink is zero and > > > i_dentry list is empty. > > > > It changes user-visible ABI. Worse yet, this "ABI" has no specification, > > so any (misguided) software using that FPOS has nothing to go by other than > > "whatever existing kernels do". As the result, inotify behaviour is cast > > in concrete. > I agree that it changes user-visible ABI and I agree the behavior isn't > really specified in the manpage. However our position has always been you can > change stuff unless someone complains. In this case the current behavior > is: If you just unlink opened file, you keep getting inotify events for that > inode. Also if you do sequence like: > fd = inotify_init1(IN_NONBLOCK); > deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); > link(path, path_link); > wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); > > unlink(path_link); > unlink(path); > > You will also keep getting inotify events for that inode (the first unlink > will naturally just generate ATTRIB event, the second unlink will fail the > test: > if (dentry->d_lockref.count == 1) { > in d_delete() and thus won't call dentry_unlink_inode() which would delete > inotify watch. > > However if you do sequence like: > fd = inotify_init1(IN_NONBLOCK); > deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); > link(path, path_link); > wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); > > unlink(path); /* Note swapped unlink order */ > unlink(path_link); > > You will stop getting inotify events for the inode. > > That's so stupid and inconsistent (plus the behavior of this already > changed in the past as my tests with older kernels show) that I'm convinced > noone really depends on this. So I'm for fixing the bug even though it has > userspace visible impact. I want to support this suggestion. This patch fixes the case, which is not widely used. And I think it's a reason why it has not fixed before. In CRIU we have code which is used this behaviour. This code was written based on the common sense and we have surprised to know that it doesn't work. I think if something is not in a specification and it doesn't base on the common sense, it's most likely a bug rather than ABI. I have executed inotify tastcases form LTP test suite. They say nothing wrong here. It's just in case if it has some sense for someone. [root@avagin-fc19-cr inotify]# ./inotify01 inotify01 1 TPASS : get event: wd=1 mask=4 cookie=0 len=0 inotify01 2 TPASS : get event: wd=1 mask=20 cookie=0 len=0 inotify01 3 TPASS : get event: wd=1 mask=1 cookie=0 len=0 inotify01 4 TPASS : get event: wd=1 mask=10 cookie=0 len=0 inotify01 5 TPASS : get event: wd=1 mask=20 cookie=0 len=0 inotify01 6 TPASS : get event: wd=1 mask=2 cookie=0 len=0 inotify01 7 TPASS : get event: wd=1 mask=8 cookie=0 len=0 [root@avagin-fc19-cr inotify]# ./inotify02 inotify02 1 TPASS : get event: wd=1 mask=40000004 cookie=0 len=0 name="" inotify02 2 TPASS : get event: wd=1 mask=100 cookie=0 len=16 name="test_file1" inotify02 3 TPASS : get event: wd=1 mask=20 cookie=0 len=16 name="test_file1" inotify02 4 TPASS : get event: wd=1 mask=8 cookie=0 len=16 name="test_file1" inotify02 5 TPASS : get event: wd=1 mask=40 cookie=1102 len=16 name="test_file1" inotify02 6 TPASS : get event: wd=1 mask=80 cookie=1102 len=16 name="test_file2" inotify02 7 TPASS : get event: wd=1 mask=800 cookie=0 len=0 name="" inotify02 8 TPASS : get event: wd=1 mask=200 cookie=0 len=16 name="test_file2" inotify02 9 TPASS : get event: wd=1 mask=800 cookie=0 len=0 name="" [root@avagin-fc19-cr inotify]# ./inotify03 inotify03 0 TINFO : Found free device '/dev/loop0' inotify03 0 TINFO : Formatting /dev/loop0 with ext2 extra opts='' mke2fs 1.42.8 (20-Jun-2013) inotify03 0 TINFO : mount /dev/loop0 to mntpoint fs_type=ext2 inotify03 0 TINFO : umount /dev/loop0 inotify03 1 TPASS : get event: wd=1 mask=2000 cookie=0 len=0 inotify03 2 TPASS : get event: wd=1 mask=8000 cookie=0 len=0 inotify03 3 TPASS : inotify_rm_watch (3, 1) returned EINVAL [root@avagin-fc19-cr inotify]# ./inotify04 inotify04 1 TPASS : got event: wd=1 mask=400 cookie=0 len=0 name="" inotify04 2 TPASS : got event: wd=1 mask=8000 cookie=0 len=0 name="" inotify04 3 TPASS : got event: wd=2 mask=4 cookie=0 len=0 name="" inotify04 4 TPASS : got event: wd=2 mask=400 cookie=0 len=0 name="" inotify04 5 TPASS : got event: wd=2 mask=8000 cookie=0 len=0 name="" [root@avagin-fc19-cr inotify]# ./inotify05 inotify05 1 TPASS : get event: wd=-1 mask=4000 cookie=0 len=0 Thanks, Andrew > > Honza > -- > Jan Kara <jack@suse.cz> > SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fs: don't remove inotify watchers from alive inode-s 2014-09-10 9:43 ` Andrew Vagin @ 2014-09-13 16:15 ` Heinrich Schuchardt 2014-09-16 21:12 ` Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: Heinrich Schuchardt @ 2014-09-13 16:15 UTC (permalink / raw) To: Andrew Vagin, Jan Kara Cc: Al Viro, Andrey Vagin, linux-fsdevel, linux-kernel, John McCutchan, Robert Love, Eric Paris, Cyrill Gorcunov, Pavel Emelyanov, Michael Kerrisk (man-pages) On Tue 09-09-14 02:27:12, Al Viro wrote: http://lkml.org/lkml/2014/9/8/762 > I agree that it changes user-visible ABI and I agree the behavior > isn't really specified in the manpage. Shouldn't we start with putting the expected behavior into the manpage before patching the code? I am missing a patch for man7/inotify.7. On Mon, Sep 08, 2014 at 04:01:56PM +0400, Andrey Vagin wrote: http://lkml.org/lkml/2014/9/8/219 > > fd = inotify_init1(IN_NONBLOCK); > deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); > link(path, path_link); > > wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); > > unlink(path); > unlink(path_link); > > printf(" --- unlink\n"); > read_evetns(fd); > > close(deleted); > printf(" --- close\n"); > read_evetns(fd); > > Without this patch: > --- unlink > 4 (IN_ATTRIB) > 400 (IN_DELETE_SELF) > 8000 (IN_IGNORED) > --- close > FAIL > > With this patch: > --- unlink > 4 (IN_ATTRIB) > 400 (IN_DELETE_SELF) > --- close > 8 (IN_CLOSE_WRITE) > 400 (IN_DELETE_SELF) > 8000 (IN_IGNORED) > PASS Shouldn't the second IN_DELETE_SELF occur before --- close ? Why is IN_CLOSE_WRITE created? Best regards Heinrich Schuchardt ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fs: don't remove inotify watchers from alive inode-s 2014-09-13 16:15 ` Heinrich Schuchardt @ 2014-09-16 21:12 ` Jan Kara 2014-09-17 21:01 ` Andrew Vagin 0 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2014-09-16 21:12 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Andrew Vagin, Jan Kara, Al Viro, Andrey Vagin, linux-fsdevel, linux-kernel, John McCutchan, Robert Love, Eric Paris, Cyrill Gorcunov, Pavel Emelyanov, Michael Kerrisk (man-pages) On Sat 13-09-14 18:15:09, Heinrich Schuchardt wrote: > On Tue 09-09-14 02:27:12, Al Viro wrote: > http://lkml.org/lkml/2014/9/8/762 > > I agree that it changes user-visible ABI and I agree the behavior > > isn't really specified in the manpage. > > Shouldn't we start with putting the expected behavior into the > manpage before patching the code? I am missing a patch for > man7/inotify.7. Good idea. Thanks for bringing this up. And ideally we should write it down before settling for a solution to this problem. Because when thinking about it again, some details of the behavior are still vague. > On Mon, Sep 08, 2014 at 04:01:56PM +0400, Andrey Vagin wrote: > http://lkml.org/lkml/2014/9/8/219 > > > > fd = inotify_init1(IN_NONBLOCK); > > deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); > > link(path, path_link); > > > > wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); > > > > unlink(path); > > unlink(path_link); > > > > printf(" --- unlink\n"); > > read_evetns(fd); > > > > close(deleted); > > printf(" --- close\n"); > > read_evetns(fd); > > > > Without this patch: > > --- unlink > > 4 (IN_ATTRIB) > > 400 (IN_DELETE_SELF) > > 8000 (IN_IGNORED) > > --- close > > FAIL > > > > With this patch: > > --- unlink > > 4 (IN_ATTRIB) > > 400 (IN_DELETE_SELF) > > --- close > > 8 (IN_CLOSE_WRITE) > > 400 (IN_DELETE_SELF) > > 8000 (IN_IGNORED) > > PASS > > Shouldn't the second IN_DELETE_SELF occur before > --- close ? > Why is IN_CLOSE_WRITE created? So I would like events to be generated until the watched inode really gets deleted. This way simple (non-hardlinked) file behaves and that's what seems "natural". In this light generating IN_CLOSE_WRITE is what we want to do. Generation of IN_DELETE_SELF is less obvious I think. Do we want to generate IN_DELETE_SELF for each hardlink to the inode that gets removed? I don't think so (this actually would be too visible user API change IMHO). To match the single link case I think we want to generate IN_DELETE_SELF when the last link to the file is removed. But then generating it twice like we would do with the above patch is wrong... Opinions? Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] fs: don't remove inotify watchers from alive inode-s 2014-09-16 21:12 ` Jan Kara @ 2014-09-17 21:01 ` Andrew Vagin 2014-09-18 10:00 ` Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: Andrew Vagin @ 2014-09-17 21:01 UTC (permalink / raw) To: Jan Kara Cc: Heinrich Schuchardt, Al Viro, Andrey Vagin, linux-fsdevel, linux-kernel, John McCutchan, Robert Love, Eric Paris, Cyrill Gorcunov, Pavel Emelyanov, Michael Kerrisk (man-pages) [-- Attachment #1: Type: text/plain, Size: 2800 bytes --] On Tue, Sep 16, 2014 at 11:12:11PM +0200, Jan Kara wrote: > On Sat 13-09-14 18:15:09, Heinrich Schuchardt wrote: > > On Tue 09-09-14 02:27:12, Al Viro wrote: > > http://lkml.org/lkml/2014/9/8/762 > > > I agree that it changes user-visible ABI and I agree the behavior > > > isn't really specified in the manpage. > > > > Shouldn't we start with putting the expected behavior into the > > manpage before patching the code? I am missing a patch for > > man7/inotify.7. > Good idea. Thanks for bringing this up. And ideally we should write it > down before settling for a solution to this problem. Because when thinking > about it again, some details of the behavior are still vague. > > > On Mon, Sep 08, 2014 at 04:01:56PM +0400, Andrey Vagin wrote: > > http://lkml.org/lkml/2014/9/8/219 > > > > > > fd = inotify_init1(IN_NONBLOCK); > > > deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); > > > link(path, path_link); > > > > > > wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); > > > > > > unlink(path); > > > unlink(path_link); > > > > > > printf(" --- unlink\n"); > > > read_evetns(fd); > > > > > > close(deleted); > > > printf(" --- close\n"); > > > read_evetns(fd); > > > > > > Without this patch: > > > --- unlink > > > 4 (IN_ATTRIB) > > > 400 (IN_DELETE_SELF) > > > 8000 (IN_IGNORED) > > > --- close > > > FAIL > > > > > > With this patch: > > > --- unlink > > > 4 (IN_ATTRIB) > > > 400 (IN_DELETE_SELF) > > > --- close > > > 8 (IN_CLOSE_WRITE) > > > 400 (IN_DELETE_SELF) > > > 8000 (IN_IGNORED) > > > PASS > > > > Shouldn't the second IN_DELETE_SELF occur before > > --- close ? > > Why is IN_CLOSE_WRITE created? > So I would like events to be generated until the watched inode really > gets deleted. This way simple (non-hardlinked) file behaves and that's what > seems "natural". In this light generating IN_CLOSE_WRITE is what we want to > do. > > Generation of IN_DELETE_SELF is less obvious I think. Do we want to > generate IN_DELETE_SELF for each hardlink to the inode that gets removed? I > don't think so (this actually would be too visible user API change IMHO). > To match the single link case I think we want to generate IN_DELETE_SELF > when the last link to the file is removed. But then generating it twice > like we would do with the above patch is wrong... Opinions? I think you are right. Could you look at the attached patch? Pay your attention to the description. This patch removes differences of behaviours for two equivalent cases. And the resulting behavior is equal for one of these case. If it looks good for you, I will send this patch together with the patch for the man page. Thank you and Heinrich for the comments and advices. Thanks, Andrey > > Honza > -- > Jan Kara <jack@suse.cz> > SUSE Labs, CR [-- Attachment #2: 0001-fs-don-t-remove-inotify-watchers-from-alive-inode-s-.patch --] [-- Type: text/plain, Size: 3861 bytes --] >From c7a79bccca1aac70f5e50fb145942b932eca79ba Mon Sep 17 00:00:00 2001 From: Andrey Vagin <avagin@openvz.org> Date: Sat, 6 Sep 2014 08:35:24 +0400 Subject: [PATCH] fs: don't remove inotify watchers from alive inode-s (v2) Currently watchers are removed in dentry_iput(), if n_link is zero. But other detries can be linked with this inode. For example if we create two hard links, open the first one and set an inotify watcher on one of them. Then if we remove the opened file and then another file, the inotify watcher will be removed. But we will have the alive file descriptor, which allows us to generate more events. And here is another behaviour, if files are removed in another order. The watcher will not be removed and we will keep getting inotify events for that inode. This patch removes difference of behaviours for these cases. Watchers are removed, only if nlink is zero and i_dentry list is empty. The resulting behaviour is the same with what has been described in the second case. Look at a following example: fd = inotify_init1(IN_NONBLOCK); deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); link(path, path_link); wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); unlink(path); unlink(path_link); printf(" --- unlink path, path_link\n"); read_evetns(fd); close(deleted); printf(" --- close\n"); read_evetns(fd); printf(" --- end\n"); We expect to get the same set of events for this case and for the case, when files are deleted in another order. But now we get the different set of events. Without this patch: The first case, when "path" is deleted before "path_link" --- unlink path, path_link 4 (IN_ATTRIB) 400 (IN_DELETE_SELF) 8000 (IN_IGNORED) --- close --- end and for the case, when "path_link" is deleted before "path" --- unlink path_link, path 4 (IN_ATTRIB) --- close 8 (IN_CLOSE_WRITE) 400 (IN_DELETE_SELF) 8000 (IN_IGNORED) --- end With this patch we have the same output for both cases: --- unlink 4 (IN_ATTRIB) --- close 8 (IN_CLOSE_WRITE) 400 (IN_DELETE_SELF) 8000 (IN_IGNORED) --- end PASS v2: generate IN_DELETE_SELF when the last link to the file is removed Cc: Jan Kara <jack@suse.cz> Cc: Heinrich Schuchardt <xypron.debian@gmx.de> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: John McCutchan <john@johnmccutchan.com> Cc: Robert Love <rlove@rlove.org> Cc: Eric Paris <eparis@parisplace.org> Cc: Cyrill Gorcunov <gorcunov@openvz.org> Cc: Pavel Emelyanov <xemul@parallels.com> Signed-off-by: Andrey Vagin <avagin@openvz.org> --- fs/dcache.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/fs/dcache.c b/fs/dcache.c index 7a5b514..3a0e3bc 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -278,12 +278,15 @@ static void dentry_iput(struct dentry * dentry) __releases(dentry->d_inode->i_lock) { struct inode *inode = dentry->d_inode; + bool last_dentry; + if (inode) { dentry->d_inode = NULL; hlist_del_init(&dentry->d_alias); + last_dentry = hlist_empty(&inode->i_dentry); spin_unlock(&dentry->d_lock); spin_unlock(&inode->i_lock); - if (!inode->i_nlink) + if (!inode->i_nlink && last_dentry) fsnotify_inoderemove(inode); if (dentry->d_op && dentry->d_op->d_iput) dentry->d_op->d_iput(dentry, inode); @@ -303,13 +306,16 @@ static void dentry_unlink_inode(struct dentry * dentry) __releases(dentry->d_inode->i_lock) { struct inode *inode = dentry->d_inode; + bool last_dentry; + __d_clear_type(dentry); dentry->d_inode = NULL; hlist_del_init(&dentry->d_alias); dentry_rcuwalk_barrier(dentry); + last_dentry = hlist_empty(&inode->i_dentry); spin_unlock(&dentry->d_lock); spin_unlock(&inode->i_lock); - if (!inode->i_nlink) + if (!inode->i_nlink && last_dentry) fsnotify_inoderemove(inode); if (dentry->d_op && dentry->d_op->d_iput) dentry->d_op->d_iput(dentry, inode); -- 1.9.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] fs: don't remove inotify watchers from alive inode-s 2014-09-17 21:01 ` Andrew Vagin @ 2014-09-18 10:00 ` Jan Kara 0 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2014-09-18 10:00 UTC (permalink / raw) To: Andrew Vagin Cc: Jan Kara, Heinrich Schuchardt, Al Viro, Andrey Vagin, linux-fsdevel, linux-kernel, John McCutchan, Robert Love, Eric Paris, Cyrill Gorcunov, Pavel Emelyanov, Michael Kerrisk (man-pages) On Thu 18-09-14 01:01:22, Andrew Vagin wrote: > From c7a79bccca1aac70f5e50fb145942b932eca79ba Mon Sep 17 00:00:00 2001 > From: Andrey Vagin <avagin@openvz.org> > Date: Sat, 6 Sep 2014 08:35:24 +0400 > Subject: [PATCH] fs: don't remove inotify watchers from alive inode-s (v2) > > Currently watchers are removed in dentry_iput(), if n_link is zero. But > other detries can be linked with this inode. > > For example if we create two hard links, open the first one and set an > inotify watcher on one of them. Then if we remove the opened file and > then another file, the inotify watcher will be removed. But we will have > the alive file descriptor, which allows us to generate more events. > > And here is another behaviour, if files are removed in another order. > The watcher will not be removed and we will keep getting inotify events > for that inode. > > This patch removes difference of behaviours for these cases. Watchers > are removed, only if nlink is zero and i_dentry list is empty. The > resulting behaviour is the same with what has been described in the > second case. > > Look at a following example: > > fd = inotify_init1(IN_NONBLOCK); > deleted = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0666); > link(path, path_link); > > wd_deleted = inotify_add_watch(fd, path_link, IN_ALL_EVENTS); > > unlink(path); > unlink(path_link); > > printf(" --- unlink path, path_link\n"); > read_evetns(fd); > > close(deleted); > printf(" --- close\n"); > read_evetns(fd); > printf(" --- end\n"); > > We expect to get the same set of events for this case and for the > case, when files are deleted in another order. But now we get the > different set of events. > > Without this patch: > The first case, when "path" is deleted before "path_link" > --- unlink path, path_link > 4 (IN_ATTRIB) > 400 (IN_DELETE_SELF) > 8000 (IN_IGNORED) > --- close > --- end > > and for the case, when "path_link" is deleted before "path" > --- unlink path_link, path > 4 (IN_ATTRIB) > --- close > 8 (IN_CLOSE_WRITE) > 400 (IN_DELETE_SELF) > 8000 (IN_IGNORED) > --- end > > With this patch we have the same output for both cases: > --- unlink > 4 (IN_ATTRIB) > --- close > 8 (IN_CLOSE_WRITE) > 400 (IN_DELETE_SELF) > 8000 (IN_IGNORED) > --- end > PASS > > v2: generate IN_DELETE_SELF when the last link to the file is removed The patch looks good to me and the description in changelog is good so feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > Cc: Jan Kara <jack@suse.cz> > Cc: Heinrich Schuchardt <xypron.debian@gmx.de> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: John McCutchan <john@johnmccutchan.com> > Cc: Robert Love <rlove@rlove.org> > Cc: Eric Paris <eparis@parisplace.org> > Cc: Cyrill Gorcunov <gorcunov@openvz.org> > Cc: Pavel Emelyanov <xemul@parallels.com> > Signed-off-by: Andrey Vagin <avagin@openvz.org> > --- > fs/dcache.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/fs/dcache.c b/fs/dcache.c > index 7a5b514..3a0e3bc 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -278,12 +278,15 @@ static void dentry_iput(struct dentry * dentry) > __releases(dentry->d_inode->i_lock) > { > struct inode *inode = dentry->d_inode; > + bool last_dentry; > + > if (inode) { > dentry->d_inode = NULL; > hlist_del_init(&dentry->d_alias); > + last_dentry = hlist_empty(&inode->i_dentry); > spin_unlock(&dentry->d_lock); > spin_unlock(&inode->i_lock); > - if (!inode->i_nlink) > + if (!inode->i_nlink && last_dentry) > fsnotify_inoderemove(inode); > if (dentry->d_op && dentry->d_op->d_iput) > dentry->d_op->d_iput(dentry, inode); > @@ -303,13 +306,16 @@ static void dentry_unlink_inode(struct dentry * dentry) > __releases(dentry->d_inode->i_lock) > { > struct inode *inode = dentry->d_inode; > + bool last_dentry; > + > __d_clear_type(dentry); > dentry->d_inode = NULL; > hlist_del_init(&dentry->d_alias); > dentry_rcuwalk_barrier(dentry); > + last_dentry = hlist_empty(&inode->i_dentry); > spin_unlock(&dentry->d_lock); > spin_unlock(&inode->i_lock); > - if (!inode->i_nlink) > + if (!inode->i_nlink && last_dentry) > fsnotify_inoderemove(inode); > if (dentry->d_op && dentry->d_op->d_iput) > dentry->d_op->d_iput(dentry, inode); > -- > 1.9.3 > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-09-18 10:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-08 12:01 [PATCH] fs: don't remove inotify watchers from alive inode-s Andrey Vagin 2014-09-08 12:19 ` Cyrill Gorcunov 2014-09-08 14:45 ` Jan Kara 2014-09-09 1:27 ` Al Viro 2014-09-09 8:54 ` Jan Kara 2014-09-10 9:43 ` Andrew Vagin 2014-09-13 16:15 ` Heinrich Schuchardt 2014-09-16 21:12 ` Jan Kara 2014-09-17 21:01 ` Andrew Vagin 2014-09-18 10:00 ` Jan Kara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.