[PATCH v2] fs: Do not check if there is a fsnotify watcher on pseudo inodes

[PATCH v2] fs: Do not check if there is a fsnotify watcher on pseudo inodes

[Mel Gorman]

Changelog since v1
o Updated changelog

The kernel uses internal mounts created by kern_mount() and populated
with files with no lookup path by alloc_file_pseudo for a variety of
reasons. An example of such a mount is for anonymous pipes. For pipes,
every vfs_write regardless of filesystem, fsnotify_modify() is called to
notify of any changes which incurs a small amount of overhead in fsnotify
even when there are no watchers. It can also trigger for reads and readv
and writev, it was simply vfs_write() that was noticed first.

A patch is pending that reduces, but does not eliminte, the overhead of
fsnotify but for files that cannot be looked up via a path, even that
small overhead is unnecessary. The user API for fanotify is based on
the pathname and a dirfd and proc entries appear to be the only visible
representation of the files. Proc does not have the same pathname as the
internal entry and the proc inode is not the same as the internal inode
so even if fanotify is used on a file under /proc/XX/fd, no useful events
are notified.

This patch changes alloc_file_pseudo() to always opt out of fsnotify by
setting FMODE_NONOTIFY flag so that no check is made for fsnotify watchers
on pseudo files. This should be safe as the underlying helper for the
dentry is d_alloc_pseudo which explicitly states that no lookups are ever
performed meaning that fanotify should have nothing useful to attach to.

The test motivating this was "perf bench sched messaging --pipe". On
a single-socket machine using threads the difference of the patch was
as follows.

                              5.7.0                  5.7.0
                            vanilla        nofsnotify-v1r1
Amean     1       1.3837 (   0.00%)      1.3547 (   2.10%)
Amean     3       3.7360 (   0.00%)      3.6543 (   2.19%)
Amean     5       5.8130 (   0.00%)      5.7233 *   1.54%*
Amean     7       8.1490 (   0.00%)      7.9730 *   2.16%*
Amean     12     14.6843 (   0.00%)     14.1820 (   3.42%)
Amean     18     21.8840 (   0.00%)     21.7460 (   0.63%)
Amean     24     28.8697 (   0.00%)     29.1680 (  -1.03%)
Amean     30     36.0787 (   0.00%)     35.2640 *   2.26%*
Amean     32     38.0527 (   0.00%)     38.1223 (  -0.18%)

The difference is small but in some cases it's outside the noise so
while marginal, there is still some small benefit to ignoring fsnotify
for files allocated via alloc_file_pseudo in some cases.

Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 fs/file_table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 30d55c9a1744..0076ccf67a7d 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -229,7 +229,7 @@ struct file *alloc_file_pseudo(struct inode *inode, struct vfsmount *mnt,
 		d_set_d_op(path.dentry, &anon_ops);
 	path.mnt = mntget(mnt);
 	d_instantiate(path.dentry, inode);
-	file = alloc_file(&path, flags, fops);
+	file = alloc_file(&path, flags | FMODE_NONOTIFY, fops);
 	if (IS_ERR(file)) {
 		ihold(inode);
 		path_put(&path);

Re: [PATCH v2] fs: Do not check if there is a fsnotify watcher on pseudo inodes

[Amir Goldstein]

On Mon, Jun 15, 2020 at 3:14 PM Mel Gorman <mgorman@techsingularity.net> wrote:
>
> Changelog since v1
> o Updated changelog

Slipped to commit message

>
> The kernel uses internal mounts created by kern_mount() and populated
> with files with no lookup path by alloc_file_pseudo for a variety of
> reasons. An example of such a mount is for anonymous pipes. For pipes,
> every vfs_write regardless of filesystem, fsnotify_modify() is called to
> notify of any changes which incurs a small amount of overhead in fsnotify
> even when there are no watchers. It can also trigger for reads and readv
> and writev, it was simply vfs_write() that was noticed first.
>
> A patch is pending that reduces, but does not eliminte, the overhead of

typo: eliminte

> fsnotify but for files that cannot be looked up via a path, even that
> small overhead is unnecessary. The user API for fanotify is based on
> the pathname and a dirfd and proc entries appear to be the only visible
> representation of the files. Proc does not have the same pathname as the
> internal entry and the proc inode is not the same as the internal inode
> so even if fanotify is used on a file under /proc/XX/fd, no useful events
> are notified.
>

Note that fanotify is not the only uapi to add marks, but this is fine by me
I suppose if Jan wants to he can make small corrections on commit.

> This patch changes alloc_file_pseudo() to always opt out of fsnotify by
> setting FMODE_NONOTIFY flag so that no check is made for fsnotify watchers
> on pseudo files. This should be safe as the underlying helper for the
> dentry is d_alloc_pseudo which explicitly states that no lookups are ever
> performed meaning that fanotify should have nothing useful to attach to.
>
> The test motivating this was "perf bench sched messaging --pipe". On
> a single-socket machine using threads the difference of the patch was
> as follows.
>
>                               5.7.0                  5.7.0
>                             vanilla        nofsnotify-v1r1
> Amean     1       1.3837 (   0.00%)      1.3547 (   2.10%)
> Amean     3       3.7360 (   0.00%)      3.6543 (   2.19%)
> Amean     5       5.8130 (   0.00%)      5.7233 *   1.54%*
> Amean     7       8.1490 (   0.00%)      7.9730 *   2.16%*
> Amean     12     14.6843 (   0.00%)     14.1820 (   3.42%)
> Amean     18     21.8840 (   0.00%)     21.7460 (   0.63%)
> Amean     24     28.8697 (   0.00%)     29.1680 (  -1.03%)
> Amean     30     36.0787 (   0.00%)     35.2640 *   2.26%*
> Amean     32     38.0527 (   0.00%)     38.1223 (  -0.18%)
>
> The difference is small but in some cases it's outside the noise so
> while marginal, there is still some small benefit to ignoring fsnotify
> for files allocated via alloc_file_pseudo in some cases.
>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.

> ---
>  fs/file_table.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 30d55c9a1744..0076ccf67a7d 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -229,7 +229,7 @@ struct file *alloc_file_pseudo(struct inode *inode, struct vfsmount *mnt,
>                 d_set_d_op(path.dentry, &anon_ops);
>         path.mnt = mntget(mnt);
>         d_instantiate(path.dentry, inode);
> -       file = alloc_file(&path, flags, fops);
> +       file = alloc_file(&path, flags | FMODE_NONOTIFY, fops);
>         if (IS_ERR(file)) {
>                 ihold(inode);
>                 path_put(&path);

Re: [PATCH v2] fs: Do not check if there is a fsnotify watcher on pseudo inodes

[Mel Gorman]

On Mon, Jun 15, 2020 at 07:26:38PM +0300, Amir Goldstein wrote:
> On Mon, Jun 15, 2020 at 3:14 PM Mel Gorman <mgorman@techsingularity.net> wrote:
> >
> > Changelog since v1
> > o Updated changelog
> 
> Slipped to commit message
> 

It's habit, it's the layout I generally use for mm even though others
prefer having it below ---. I wasn't sure of fsnotify's preferred format
for tracking major differences between versions.

> >
> > The kernel uses internal mounts created by kern_mount() and populated
> > with files with no lookup path by alloc_file_pseudo for a variety of
> > reasons. An example of such a mount is for anonymous pipes. For pipes,
> > every vfs_write regardless of filesystem, fsnotify_modify() is called to
> > notify of any changes which incurs a small amount of overhead in fsnotify
> > even when there are no watchers. It can also trigger for reads and readv
> > and writev, it was simply vfs_write() that was noticed first.
> >
> > A patch is pending that reduces, but does not eliminte, the overhead of
> 
> typo: eliminte
> 

Yes.

> > fsnotify but for files that cannot be looked up via a path, even that
> > small overhead is unnecessary. The user API for fanotify is based on
> > the pathname and a dirfd and proc entries appear to be the only visible
> > representation of the files. Proc does not have the same pathname as the
> > internal entry and the proc inode is not the same as the internal inode
> > so even if fanotify is used on a file under /proc/XX/fd, no useful events
> > are notified.
> >
> 
> Note that fanotify is not the only uapi to add marks, but this is fine by me
> I suppose if Jan wants to he can make small corrections on commit.
> 

True but I didn't think inotify was materially different as it also takes
a path. Is that wrong or are there others that matter and can attach to
a file that cannot be looked up via a path?

> > The difference is small but in some cases it's outside the noise so
> > while marginal, there is still some small benefit to ignoring fsnotify
> > for files allocated via alloc_file_pseudo in some cases.
> >
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 

Thanks!

-- 
Mel Gorman
SUSE Labs

Re: [PATCH v2] fs: Do not check if there is a fsnotify watcher on pseudo inodes

[Amir Goldstein]

On Mon, Jun 15, 2020 at 8:25 PM Mel Gorman <mgorman@techsingularity.net> wrote:
>
> On Mon, Jun 15, 2020 at 07:26:38PM +0300, Amir Goldstein wrote:
> > On Mon, Jun 15, 2020 at 3:14 PM Mel Gorman <mgorman@techsingularity.net> wrote:
> > >
> > > Changelog since v1
> > > o Updated changelog
> >
> > Slipped to commit message
> >
>
> It's habit, it's the layout I generally use for mm even though others
> prefer having it below ---. I wasn't sure of fsnotify's preferred format
> for tracking major differences between versions.
>
> > >
> > > The kernel uses internal mounts created by kern_mount() and populated
> > > with files with no lookup path by alloc_file_pseudo for a variety of
> > > reasons. An example of such a mount is for anonymous pipes. For pipes,
> > > every vfs_write regardless of filesystem, fsnotify_modify() is called to
> > > notify of any changes which incurs a small amount of overhead in fsnotify
> > > even when there are no watchers. It can also trigger for reads and readv
> > > and writev, it was simply vfs_write() that was noticed first.
> > >
> > > A patch is pending that reduces, but does not eliminte, the overhead of
> >
> > typo: eliminte
> >
>
> Yes.
>
> > > fsnotify but for files that cannot be looked up via a path, even that
> > > small overhead is unnecessary. The user API for fanotify is based on
> > > the pathname and a dirfd and proc entries appear to be the only visible
> > > representation of the files. Proc does not have the same pathname as the
> > > internal entry and the proc inode is not the same as the internal inode
> > > so even if fanotify is used on a file under /proc/XX/fd, no useful events
> > > are notified.
> > >
> >
> > Note that fanotify is not the only uapi to add marks, but this is fine by me
> > I suppose if Jan wants to he can make small corrections on commit.
> >
>
> True but I didn't think inotify was materially different as it also takes
> a path. Is that wrong or are there others that matter and can attach to
> a file that cannot be looked up via a path?

There are kernel/audit* and nfsd/filecache.c users, but as far as I could
tell, there is no danger from there. I was just pointing out that the fanotify
uapi argument alone is not a full proof.

Thanks,
Amir.

Re: [PATCH v2] fs: Do not check if there is a fsnotify watcher on pseudo inodes

[Jan Kara]

On Mon 15-06-20 19:26:38, Amir Goldstein wrote:
> > This patch changes alloc_file_pseudo() to always opt out of fsnotify by
> > setting FMODE_NONOTIFY flag so that no check is made for fsnotify watchers
> > on pseudo files. This should be safe as the underlying helper for the
> > dentry is d_alloc_pseudo which explicitly states that no lookups are ever
> > performed meaning that fanotify should have nothing useful to attach to.
> >
> > The test motivating this was "perf bench sched messaging --pipe". On
> > a single-socket machine using threads the difference of the patch was
> > as follows.
> >
> >                               5.7.0                  5.7.0
> >                             vanilla        nofsnotify-v1r1
> > Amean     1       1.3837 (   0.00%)      1.3547 (   2.10%)
> > Amean     3       3.7360 (   0.00%)      3.6543 (   2.19%)
> > Amean     5       5.8130 (   0.00%)      5.7233 *   1.54%*
> > Amean     7       8.1490 (   0.00%)      7.9730 *   2.16%*
> > Amean     12     14.6843 (   0.00%)     14.1820 (   3.42%)
> > Amean     18     21.8840 (   0.00%)     21.7460 (   0.63%)
> > Amean     24     28.8697 (   0.00%)     29.1680 (  -1.03%)
> > Amean     30     36.0787 (   0.00%)     35.2640 *   2.26%*
> > Amean     32     38.0527 (   0.00%)     38.1223 (  -0.18%)
> >
> > The difference is small but in some cases it's outside the noise so
> > while marginal, there is still some small benefit to ignoring fsnotify
> > for files allocated via alloc_file_pseudo in some cases.
> >
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks for the patch Mel and for review Amir! I've added the patch to my
tree with small amendments to the changelog.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] fs: Do not check if there is a fsnotify watcher on pseudo inodes

[Eric Dumazet]

On 6/16/20 12:47 AM, Jan Kara wrote:
> On Mon 15-06-20 19:26:38, Amir Goldstein wrote:
>>> This patch changes alloc_file_pseudo() to always opt out of fsnotify by
>>> setting FMODE_NONOTIFY flag so that no check is made for fsnotify watchers
>>> on pseudo files. This should be safe as the underlying helper for the
>>> dentry is d_alloc_pseudo which explicitly states that no lookups are ever
>>> performed meaning that fanotify should have nothing useful to attach to.
>>>
>>> The test motivating this was "perf bench sched messaging --pipe". On
>>> a single-socket machine using threads the difference of the patch was
>>> as follows.
>>>
>>>                               5.7.0                  5.7.0
>>>                             vanilla        nofsnotify-v1r1
>>> Amean     1       1.3837 (   0.00%)      1.3547 (   2.10%)
>>> Amean     3       3.7360 (   0.00%)      3.6543 (   2.19%)
>>> Amean     5       5.8130 (   0.00%)      5.7233 *   1.54%*
>>> Amean     7       8.1490 (   0.00%)      7.9730 *   2.16%*
>>> Amean     12     14.6843 (   0.00%)     14.1820 (   3.42%)
>>> Amean     18     21.8840 (   0.00%)     21.7460 (   0.63%)
>>> Amean     24     28.8697 (   0.00%)     29.1680 (  -1.03%)
>>> Amean     30     36.0787 (   0.00%)     35.2640 *   2.26%*
>>> Amean     32     38.0527 (   0.00%)     38.1223 (  -0.18%)
>>>
>>> The difference is small but in some cases it's outside the noise so
>>> while marginal, there is still some small benefit to ignoring fsnotify
>>> for files allocated via alloc_file_pseudo in some cases.
>>>
>>> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
>>
>> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> Thanks for the patch Mel and for review Amir! I've added the patch to my
> tree with small amendments to the changelog.
> 
> 								Honza
> 

Note this patch broke some user programs (one instance being packetdrill)

Typical legacy packetdrill script has :

// Create a socket and set it to non-blocking.
    0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 fcntl(3, F_GETFL) = 0x2 (flags O_RDWR)
   +0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0


But after this change, fcntl(3, F_GETFL) returns 0x4000002 

FMODE_NONOTIFY was not meant to be visible to user space. (otherwise
there would be a O_NONOTIFY) ?

Re: [PATCH v2] fs: Do not check if there is a fsnotify watcher on pseudo inodes

[Mel Gorman]

On Mon, Jun 29, 2020 at 08:17:02AM -0700, Eric Dumazet wrote:
> 
> 
> On 6/16/20 12:47 AM, Jan Kara wrote:
> > On Mon 15-06-20 19:26:38, Amir Goldstein wrote:
> >>> This patch changes alloc_file_pseudo() to always opt out of fsnotify by
> >>> setting FMODE_NONOTIFY flag so that no check is made for fsnotify watchers
> >>> on pseudo files. This should be safe as the underlying helper for the
> >>> dentry is d_alloc_pseudo which explicitly states that no lookups are ever
> >>> performed meaning that fanotify should have nothing useful to attach to.
> >>>
> >>> The test motivating this was "perf bench sched messaging --pipe". On
> >>> a single-socket machine using threads the difference of the patch was
> >>> as follows.
> >>>
> >>>                               5.7.0                  5.7.0
> >>>                             vanilla        nofsnotify-v1r1
> >>> Amean     1       1.3837 (   0.00%)      1.3547 (   2.10%)
> >>> Amean     3       3.7360 (   0.00%)      3.6543 (   2.19%)
> >>> Amean     5       5.8130 (   0.00%)      5.7233 *   1.54%*
> >>> Amean     7       8.1490 (   0.00%)      7.9730 *   2.16%*
> >>> Amean     12     14.6843 (   0.00%)     14.1820 (   3.42%)
> >>> Amean     18     21.8840 (   0.00%)     21.7460 (   0.63%)
> >>> Amean     24     28.8697 (   0.00%)     29.1680 (  -1.03%)
> >>> Amean     30     36.0787 (   0.00%)     35.2640 *   2.26%*
> >>> Amean     32     38.0527 (   0.00%)     38.1223 (  -0.18%)
> >>>
> >>> The difference is small but in some cases it's outside the noise so
> >>> while marginal, there is still some small benefit to ignoring fsnotify
> >>> for files allocated via alloc_file_pseudo in some cases.
> >>>
> >>> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> >>
> >> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > 
> > Thanks for the patch Mel and for review Amir! I've added the patch to my
> > tree with small amendments to the changelog.
> > 
> > 								Honza
> > 
> 
> Note this patch broke some user programs (one instance being packetdrill)
> 
> Typical legacy packetdrill script has :
> 
> // Create a socket and set it to non-blocking.
>     0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
>    +0 fcntl(3, F_GETFL) = 0x2 (flags O_RDWR)
>    +0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0
> 
> 
> But after this change, fcntl(3, F_GETFL) returns 0x4000002 
> 
> FMODE_NONOTIFY was not meant to be visible to user space. (otherwise
> there would be a O_NONOTIFY) ?
> 

A revert has been sent because it broke chrome as well. It's not visible
on lkml yet because delivery appears to be delayed there for some unknown
reason.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH v2] fs: Do not check if there is a fsnotify watcher on pseudo inodes

[Jan Kara]

On Mon 29-06-20 08:17:02, Eric Dumazet wrote:
> On 6/16/20 12:47 AM, Jan Kara wrote:
> > On Mon 15-06-20 19:26:38, Amir Goldstein wrote:
> >>> This patch changes alloc_file_pseudo() to always opt out of fsnotify by
> >>> setting FMODE_NONOTIFY flag so that no check is made for fsnotify watchers
> >>> on pseudo files. This should be safe as the underlying helper for the
> >>> dentry is d_alloc_pseudo which explicitly states that no lookups are ever
> >>> performed meaning that fanotify should have nothing useful to attach to.
> >>>
> >>> The test motivating this was "perf bench sched messaging --pipe". On
> >>> a single-socket machine using threads the difference of the patch was
> >>> as follows.
> >>>
> >>>                               5.7.0                  5.7.0
> >>>                             vanilla        nofsnotify-v1r1
> >>> Amean     1       1.3837 (   0.00%)      1.3547 (   2.10%)
> >>> Amean     3       3.7360 (   0.00%)      3.6543 (   2.19%)
> >>> Amean     5       5.8130 (   0.00%)      5.7233 *   1.54%*
> >>> Amean     7       8.1490 (   0.00%)      7.9730 *   2.16%*
> >>> Amean     12     14.6843 (   0.00%)     14.1820 (   3.42%)
> >>> Amean     18     21.8840 (   0.00%)     21.7460 (   0.63%)
> >>> Amean     24     28.8697 (   0.00%)     29.1680 (  -1.03%)
> >>> Amean     30     36.0787 (   0.00%)     35.2640 *   2.26%*
> >>> Amean     32     38.0527 (   0.00%)     38.1223 (  -0.18%)
> >>>
> >>> The difference is small but in some cases it's outside the noise so
> >>> while marginal, there is still some small benefit to ignoring fsnotify
> >>> for files allocated via alloc_file_pseudo in some cases.
> >>>
> >>> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> >>
> >> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > 
> > Thanks for the patch Mel and for review Amir! I've added the patch to my
> > tree with small amendments to the changelog.
> > 
> > 								Honza
> > 
> 
> Note this patch broke some user programs (one instance being packetdrill)
> 
> Typical legacy packetdrill script has :
> 
> // Create a socket and set it to non-blocking.
>     0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
>    +0 fcntl(3, F_GETFL) = 0x2 (flags O_RDWR)
>    +0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0
> 
> 
> But after this change, fcntl(3, F_GETFL) returns 0x4000002 
> 
> FMODE_NONOTIFY was not meant to be visible to user space. (otherwise
> there would be a O_NONOTIFY) ?

Interesting. As Mel said the patch is reverted anyway (Linus already
applied the revert) but the question about FMODE_NONOTIFY is interesting.
Userspace certainly cannot set the flag (the kernel enforces this on
open(2) and fcntl(F_SETFL)). But it is visible to userspace via
fcntl(F_GETFL) which may have been an oversight... I'm just not sure
whether some of the fanotify(7) users which legitimately get such file
descriptors don't depend on this flag being visible.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR