[PATCH] splice: report related fsnotify events

[PATCH] splice: report related fsnotify events

[Chung-Chiang Cheng]

The fsnotify ACCESS and MODIFY event are missing when manipulating a file
with splice(2).

Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
---
 fs/splice.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/splice.c b/fs/splice.c
index 5969b7a1d353..9cadcaf52a3e 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -30,6 +30,7 @@
 #include <linux/export.h>
 #include <linux/syscalls.h>
 #include <linux/uio.h>
+#include <linux/fsnotify.h>
 #include <linux/security.h>
 #include <linux/gfp.h>
 #include <linux/socket.h>
@@ -1074,6 +1075,9 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
 		ret = do_splice_from(ipipe, out, &offset, len, flags);
 		file_end_write(out);
 
+		if (ret > 0)
+			fsnotify_modify(out);
+
 		if (!off_out)
 			out->f_pos = offset;
 		else
@@ -1097,6 +1101,10 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
 			flags |= SPLICE_F_NONBLOCK;
 
 		ret = splice_file_to_pipe(in, opipe, &offset, len, flags);
+
+		if (ret > 0)
+			fsnotify_access(in);
+
 		if (!off_in)
 			in->f_pos = offset;
 		else
-- 
2.34.1

Re: [PATCH] splice: report related fsnotify events

[Amir Goldstein]

On Wed, Mar 22, 2023 at 8:51 AM Chung-Chiang Cheng <cccheng@synology.com> wrote:
>
> The fsnotify ACCESS and MODIFY event are missing when manipulating a file
> with splice(2).
>
> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>

Looks good.

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

> ---
>  fs/splice.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/fs/splice.c b/fs/splice.c
> index 5969b7a1d353..9cadcaf52a3e 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -30,6 +30,7 @@
>  #include <linux/export.h>
>  #include <linux/syscalls.h>
>  #include <linux/uio.h>
> +#include <linux/fsnotify.h>
>  #include <linux/security.h>
>  #include <linux/gfp.h>
>  #include <linux/socket.h>
> @@ -1074,6 +1075,9 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
>                 ret = do_splice_from(ipipe, out, &offset, len, flags);
>                 file_end_write(out);
>
> +               if (ret > 0)
> +                       fsnotify_modify(out);
> +
>                 if (!off_out)
>                         out->f_pos = offset;
>                 else
> @@ -1097,6 +1101,10 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
>                         flags |= SPLICE_F_NONBLOCK;
>
>                 ret = splice_file_to_pipe(in, opipe, &offset, len, flags);
> +
> +               if (ret > 0)
> +                       fsnotify_access(in);
> +
>                 if (!off_in)
>                         in->f_pos = offset;
>                 else
> --
> 2.34.1
>

Re: [PATCH] splice: report related fsnotify events

[Christian Brauner]

On Wed, 22 Mar 2023 14:25:19 +0800, Chung-Chiang Cheng wrote:
> The fsnotify ACCESS and MODIFY event are missing when manipulating a file
> with splice(2).
> 
> 

I've picked this up,

tree: git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git
branch: fs.misc
[1/1] splice: report related fsnotify events
      commit: 4f61a69edcf33a66269f65500434b584ee8d405e

Re: [PATCH] splice: report related fsnotify events

[Amir Goldstein]

On Wed, Mar 22, 2023 at 9:08 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Mar 22, 2023 at 8:51 AM Chung-Chiang Cheng <cccheng@synology.com> wrote:
> >
> > The fsnotify ACCESS and MODIFY event are missing when manipulating a file
> > with splice(2).
> >

Jens, Jan,

FYI, I've audited aio routines and found that
fsnotify_access()/modify() are also missing in aio_complete_rw()
and in io_complete_rw_iopoll() (io_req_io_end() should be called?).

I am not using/testing aio/io_uring usually, so I wasn't planning on sending
a patch any time soon. I'll get to it someday.
Just wanted to bring this to public attention in case someone is
interested in testing/fixing.

Thanks,
Amir.

Re: [PATCH] splice: report related fsnotify events

[Jens Axboe]

On 4/3/23 11:00?AM, Amir Goldstein wrote:
> On Wed, Mar 22, 2023 at 9:08?AM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Wed, Mar 22, 2023 at 8:51?AM Chung-Chiang Cheng <cccheng@synology.com> wrote:
>>>
>>> The fsnotify ACCESS and MODIFY event are missing when manipulating a file
>>> with splice(2).
>>>
> 
> Jens, Jan,
> 
> FYI, I've audited aio routines and found that
> fsnotify_access()/modify() are also missing in aio_complete_rw()
> and in io_complete_rw_iopoll() (io_req_io_end() should be called?).
> 
> I am not using/testing aio/io_uring usually, so I wasn't planning on sending
> a patch any time soon. I'll get to it someday.
> Just wanted to bring this to public attention in case someone is
> interested in testing/fixing.

aio has never done fsnotify, but I think that's probably an oversight.
io_uring does do it for non-polled IO, I don't think there's much point
in adding it to IOPOLL however. Not really seeing any use cases where
that would make sense.

-- 
Jens Axboe

Re: [PATCH] splice: report related fsnotify events

[Amir Goldstein]

On Mon, Apr 3, 2023 at 8:03 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 4/3/23 11:00?AM, Amir Goldstein wrote:
> > On Wed, Mar 22, 2023 at 9:08?AM Amir Goldstein <amir73il@gmail.com> wrote:
> >>
> >> On Wed, Mar 22, 2023 at 8:51?AM Chung-Chiang Cheng <cccheng@synology.com> wrote:
> >>>
> >>> The fsnotify ACCESS and MODIFY event are missing when manipulating a file
> >>> with splice(2).
> >>>
> >
> > Jens, Jan,
> >
> > FYI, I've audited aio routines and found that
> > fsnotify_access()/modify() are also missing in aio_complete_rw()
> > and in io_complete_rw_iopoll() (io_req_io_end() should be called?).
> >
> > I am not using/testing aio/io_uring usually, so I wasn't planning on sending
> > a patch any time soon. I'll get to it someday.
> > Just wanted to bring this to public attention in case someone is
> > interested in testing/fixing.
>
> aio has never done fsnotify, but I think that's probably an oversight.

I know. and I am not keen either on fixing something that nobody
complained about.

> io_uring does do it for non-polled IO, I don't think there's much point
> in adding it to IOPOLL however. Not really seeing any use cases where
> that would make sense.
>

Users subscribe to fsnotify because they want to be notified of changes/
access to a file.
Why do you think that polled IO should be exempt?

Again, not keep on fixing something nobody complained about.
I'm asking out of curiosity.

Thanks,
Amir.

Re: [PATCH] splice: report related fsnotify events

[Jens Axboe]

On 4/3/23 11:15?AM, Amir Goldstein wrote:
>> On 4/3/23 11:00?AM, Amir Goldstein wrote:
>>> On Wed, Mar 22, 2023 at 9:08?AM Amir Goldstein <amir73il@gmail.com> wrote:
>>>>
>>>> On Wed, Mar 22, 2023 at 8:51?AM Chung-Chiang Cheng <cccheng@synology.com> wrote:
>>>>>
>>>>> The fsnotify ACCESS and MODIFY event are missing when manipulating a file
>>>>> with splice(2).
>>>>>
>>>
>>> Jens, Jan,
>>>
>>> FYI, I've audited aio routines and found that
>>> fsnotify_access()/modify() are also missing in aio_complete_rw()
>>> and in io_complete_rw_iopoll() (io_req_io_end() should be called?).
>>>
>>> I am not using/testing aio/io_uring usually, so I wasn't planning on sending
>>> a patch any time soon. I'll get to it someday.
>>> Just wanted to bring this to public attention in case someone is
>>> interested in testing/fixing.
>>
>> aio has never done fsnotify, but I think that's probably an oversight.
> 
> I know. and I am not keen either on fixing something that nobody
> complained about.

Nobody does buffered IO with aio (as it doesn't work), which is probably
why nobody complained.

>> io_uring does do it for non-polled IO, I don't think there's much point
>> in adding it to IOPOLL however. Not really seeing any use cases where
>> that would make sense.
>>
> 
> Users subscribe to fsnotify because they want to be notified of changes/
> access to a file.
> Why do you think that polled IO should be exempt?

Because it's a drastically different use case. If you're doing high
performance polled IO, then you'd never rely on something as slow as
fsnotify to tell you of any changes that happened to a device or file.
That would be counter productive.

-- 
Jens Axboe

Re: [PATCH] splice: report related fsnotify events

[Jan Kara]

On Mon 03-04-23 11:23:25, Jens Axboe wrote:
> On 4/3/23 11:15?AM, Amir Goldstein wrote:
> >> On 4/3/23 11:00?AM, Amir Goldstein wrote:
> >> io_uring does do it for non-polled IO, I don't think there's much point
> >> in adding it to IOPOLL however. Not really seeing any use cases where
> >> that would make sense.
> >>
> > 
> > Users subscribe to fsnotify because they want to be notified of changes/
> > access to a file.
> > Why do you think that polled IO should be exempt?
> 
> Because it's a drastically different use case. If you're doing high
> performance polled IO, then you'd never rely on something as slow as
> fsnotify to tell you of any changes that happened to a device or file.
> That would be counter productive.

Well, I guess Amir wanted to say that the application using fsnotify is not
necessarily the one doing high performance polled IO. You could have e.g.
data mirroring application A tracking files that need mirroring to another
host using fsnotify and if some application B uses high performance polled
IO to modify a file, application A could miss the modified file.

That being said if I look at exact details, currently I don't see a very
realistic usecase that would have problems (people don't depend on
FS_MODIFY or FS_ACCESS events too much, usually they just use FS_OPEN /
FS_CLOSE), which is likely why nobody reported these issues yet :).

								Honza


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

Re: [PATCH] splice: report related fsnotify events

[Amir Goldstein]

On Tue, Apr 4, 2023 at 12:21 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 03-04-23 11:23:25, Jens Axboe wrote:
> > On 4/3/23 11:15?AM, Amir Goldstein wrote:
> > >> On 4/3/23 11:00?AM, Amir Goldstein wrote:
> > >> io_uring does do it for non-polled IO, I don't think there's much point
> > >> in adding it to IOPOLL however. Not really seeing any use cases where
> > >> that would make sense.
> > >>
> > >
> > > Users subscribe to fsnotify because they want to be notified of changes/
> > > access to a file.
> > > Why do you think that polled IO should be exempt?
> >
> > Because it's a drastically different use case. If you're doing high
> > performance polled IO, then you'd never rely on something as slow as
> > fsnotify to tell you of any changes that happened to a device or file.
> > That would be counter productive.
>
> Well, I guess Amir wanted to say that the application using fsnotify is not
> necessarily the one doing high performance polled IO. You could have e.g.
> data mirroring application A tracking files that need mirroring to another
> host using fsnotify and if some application B uses high performance polled
> IO to modify a file, application A could miss the modified file.
>
> That being said if I look at exact details, currently I don't see a very
> realistic usecase that would have problems (people don't depend on
> FS_MODIFY or FS_ACCESS events too much, usually they just use FS_OPEN /
> FS_CLOSE), which is likely why nobody reported these issues yet :).
>

I guess so.
Our monitoring application also does not rely on FS_MODIFY/FS_ACCESS
as they could be too noisy.

The thing that I find missing is being able to tell if a file was *actually*
accessed/modified in between open and close.
This information could be provided with FS_CLOSE event

Thanks,
Amir.

Re: [PATCH] splice: report related fsnotify events

[Jens Axboe]

On 4/4/23 3:21?AM, Jan Kara wrote:
> On Mon 03-04-23 11:23:25, Jens Axboe wrote:
>> On 4/3/23 11:15?AM, Amir Goldstein wrote:
>>>> On 4/3/23 11:00?AM, Amir Goldstein wrote:
>>>> io_uring does do it for non-polled IO, I don't think there's much point
>>>> in adding it to IOPOLL however. Not really seeing any use cases where
>>>> that would make sense.
>>>>
>>>
>>> Users subscribe to fsnotify because they want to be notified of changes/
>>> access to a file.
>>> Why do you think that polled IO should be exempt?
>>
>> Because it's a drastically different use case. If you're doing high
>> performance polled IO, then you'd never rely on something as slow as
>> fsnotify to tell you of any changes that happened to a device or file.
>> That would be counter productive.
> 
> Well, I guess Amir wanted to say that the application using fsnotify
> is not necessarily the one doing high performance polled IO. You could
> have e.g. data mirroring application A tracking files that need
> mirroring to another host using fsnotify and if some application B
> uses high performance polled IO to modify a file, application A could
> miss the modified file.

Sure, but what I'm trying to say is that if you're using polled IO,
you're doing a custom setup anyway and the likelihood of needing
fsnotify is slim to none. It'd certainly be in your best interesting to
NOT rely on that, for performance reasons. And the latter is presumably
why you'd using polled IO to begin with.

> That being said if I look at exact details, currently I don't see a
> very realistic usecase that would have problems (people don't depend
> on FS_MODIFY or FS_ACCESS events too much, usually they just use
> FS_OPEN / FS_CLOSE), which is likely why nobody reported these issues
> yet :).

The only report I got on io_uring and fsnotify was someone writing a
buffered log with it, and noticing that tail doesn't work if you don't
have fsnotify access notifications. Open/close would not be enough
there.

I'd much rather make fsnotify opt-in for io_uring than have it on by
default, as it's quite the cycler consumer for IRQ based workloads right
now. And that's without even having any watches on the file...

-- 
Jens Axboe

Re: [PATCH] splice: report related fsnotify events

[Jens Axboe]

On 4/4/23 7:45?AM, Amir Goldstein wrote:
> On Tue, Apr 4, 2023 at 12:21?PM Jan Kara <jack@suse.cz> wrote:
>>
>> On Mon 03-04-23 11:23:25, Jens Axboe wrote:
>>> On 4/3/23 11:15?AM, Amir Goldstein wrote:
>>>>> On 4/3/23 11:00?AM, Amir Goldstein wrote:
>>>>> io_uring does do it for non-polled IO, I don't think there's much point
>>>>> in adding it to IOPOLL however. Not really seeing any use cases where
>>>>> that would make sense.
>>>>>
>>>>
>>>> Users subscribe to fsnotify because they want to be notified of changes/
>>>> access to a file.
>>>> Why do you think that polled IO should be exempt?
>>>
>>> Because it's a drastically different use case. If you're doing high
>>> performance polled IO, then you'd never rely on something as slow as
>>> fsnotify to tell you of any changes that happened to a device or file.
>>> That would be counter productive.
>>
>> Well, I guess Amir wanted to say that the application using fsnotify is not
>> necessarily the one doing high performance polled IO. You could have e.g.
>> data mirroring application A tracking files that need mirroring to another
>> host using fsnotify and if some application B uses high performance polled
>> IO to modify a file, application A could miss the modified file.
>>
>> That being said if I look at exact details, currently I don't see a very
>> realistic usecase that would have problems (people don't depend on
>> FS_MODIFY or FS_ACCESS events too much, usually they just use FS_OPEN /
>> FS_CLOSE), which is likely why nobody reported these issues yet :).
>>
> 
> I guess so.
> Our monitoring application also does not rely on FS_MODIFY/FS_ACCESS
> as they could be too noisy.
> 
> The thing that I find missing is being able to tell if a file was *actually*
> accessed/modified in between open and close.
> This information could be provided with FS_CLOSE event

Agree, it's not a good fit for a lot of common use cases.

-- 
Jens Axboe