[PATCH] fs: don't remove inotify watchers from alive inode-s

[PATCH] fs: don't remove inotify watchers from alive inode-s

[Andrey Vagin]

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

Re: [PATCH] fs: don't remove inotify watchers from alive inode-s

[Cyrill Gorcunov]

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>

Re: [PATCH] fs: don't remove inotify watchers from alive inode-s

[Jan Kara]

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

Re: [PATCH] fs: don't remove inotify watchers from alive inode-s

[Al Viro]

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.

Re: [PATCH] fs: don't remove inotify watchers from alive inode-s

[Jan Kara]

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

Re: [PATCH] fs: don't remove inotify watchers from alive inode-s

[Andrew Vagin]

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

Re: [PATCH] fs: don't remove inotify watchers from alive inode-s

[Heinrich Schuchardt]

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

Re: [PATCH] fs: don't remove inotify watchers from alive inode-s

[Jan Kara]

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

Re: [PATCH] fs: don't remove inotify watchers from alive inode-s

[Andrew Vagin]

[-- 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

Re: [PATCH] fs: don't remove inotify watchers from alive inode-s

[Jan Kara]

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