Re: [PATCH 1/2] NFSD: Fix duplicate fsnotify access events for iterator READs

Re: [PATCH 1/2] NFSD: Fix duplicate fsnotify access events for iterator READs

[Amir Goldstein]

On Wed, Aug 19, 2026 at 4:04 PM Chuck Lever <cel@kernel.org> wrote:
>
>
>
> On Wed, Aug 19, 2026, at 2:02 AM, Christoph Hellwig wrote:
> > Well, that is the underlying bug here.  ->splice_read should not
> > skip fsnotify events and nfsd should not work around this as
> > fsnotify is not the business of the users of VFS APIs.
>
> I asked for the duplicate event to be split out of Ameer's larger
> series as a backportable fix because there is clearly a bug here.
> But NFSD might not be the correct place to address it.
>
> Today the fsnotify event comes from the system call implementations,
> not from the splice helpers. do_sendfile() calls fsnotify_access()
> once do_splice_direct() returns, and do_splice() does the same for
> splice(2), while vfs_splice_read() and splice_direct_to_actor()
> emit nothing. vfs_iocb_iter_read() is the outlier, emitting from
> inside the helper.
>
> NFSD calls splice_direct_to_actor() directly, so on that path it
> acts like do_sendfile() and emits the event itself. That is why
> the fsnotify event counts differ between NFSD's two read paths.
> Moving the fsnotify call site down into ->splice_read would double
> up sendfile events unless the system call implementations stop
> emitting it at the same time.
>
> Jan, Amir, what are your thoughts?
>

Here are some thoughts and points for consideration.

(1) In this series
https://lore.kernel.org/linux-fsdevel/20231122122715.2561213-1-amir73il@gmail.com/
we intentionally moved the permission hook outside
of the splice iterators because we wanted to avoid calling them
with freeze protection held and also there were some duplicate calls
for this work.

At this point in time, the fsnotify_{access,modify} post hooks are
usually called
from the same context as the matching permission/security hooks.
It doesn't have to be this way, but it's a good mental model IMO.

(2) emitting many READ events from an iterator instead of one event for
the user's READ request is more noisy and serves no purpose to users.
In most cases (but not always) those events could be merged, but at the
cost of futile CPU cycles.

From a quick inspection of the code, it looks like:
- fsnotify_access() is missing in vfs_splice_read()
- the naming convention for splice_ do_splice_ vfs_splice_ is a horror
- we could make the low level splice_direct_to_actor() static and possibly
  rename it to splice_direct_to_actor_sd() or something
- we could export vfs_splice_direct_to_actor() for nfsd which wraps
  splice_direct_to_actor() with permission hook and fsnotify_access

Whether or not this is prettier than nfsd adding the fsnotify hooks
is a matter of taste, but I can live with either.

Thanks,
Amir.

Re: [PATCH 1/2] NFSD: Fix duplicate fsnotify access events for iterator READs

[Chuck Lever]

On Sat, Aug 22, 2026, at 6:21 AM, Amir Goldstein wrote:
> (1) In this series
> https://lore.kernel.org/linux-fsdevel/20231122122715.2561213-1-amir73il@gmail.com/
> we intentionally moved the permission hook outside
> of the splice iterators because we wanted to avoid calling them
> with freeze protection held and also there were some duplicate calls
> for this work.
>
> At this point in time, the fsnotify_{access,modify} post hooks are
> usually called
> from the same context as the matching permission/security hooks.
> It doesn't have to be this way, but it's a good mental model IMO.
>
> (2) emitting many READ events from an iterator instead of one event for
> the user's READ request is more noisy and serves no purpose to users.
> In most cases (but not always) those events could be merged, but at the
> cost of futile CPU cycles.
>
> From a quick inspection of the code, it looks like:
> - fsnotify_access() is missing in vfs_splice_read()
> - the naming convention for splice_ do_splice_ vfs_splice_ is a horror
> - we could make the low level splice_direct_to_actor() static and possibly
>   rename it to splice_direct_to_actor_sd() or something
> - we could export vfs_splice_direct_to_actor() for nfsd which wraps
>   splice_direct_to_actor() with permission hook and fsnotify_access

IIUC this last bullet seems like clean layering to me. Do
you want to propose a patch or shall I?


-- 
Chuck Lever

Re: [PATCH 1/2] NFSD: Fix duplicate fsnotify access events for iterator READs

[Amir Goldstein]

On Sun, Aug 23, 2026 at 6:21 PM Chuck Lever <cel@kernel.org> wrote:
>
>
>
> On Sat, Aug 22, 2026, at 6:21 AM, Amir Goldstein wrote:
> > (1) In this series
> > https://lore.kernel.org/linux-fsdevel/20231122122715.2561213-1-amir73il@gmail.com/
> > we intentionally moved the permission hook outside
> > of the splice iterators because we wanted to avoid calling them
> > with freeze protection held and also there were some duplicate calls
> > for this work.
> >
> > At this point in time, the fsnotify_{access,modify} post hooks are
> > usually called
> > from the same context as the matching permission/security hooks.
> > It doesn't have to be this way, but it's a good mental model IMO.
> >
> > (2) emitting many READ events from an iterator instead of one event for
> > the user's READ request is more noisy and serves no purpose to users.
> > In most cases (but not always) those events could be merged, but at the
> > cost of futile CPU cycles.
> >
> > From a quick inspection of the code, it looks like:
> > - fsnotify_access() is missing in vfs_splice_read()
> > - the naming convention for splice_ do_splice_ vfs_splice_ is a horror
> > - we could make the low level splice_direct_to_actor() static and possibly
> >   rename it to splice_direct_to_actor_sd() or something
> > - we could export vfs_splice_direct_to_actor() for nfsd which wraps
> >   splice_direct_to_actor() with permission hook and fsnotify_access
>
> IIUC this last bullet seems like clean layering to me. Do
> you want to propose a patch or shall I?

Be my guest.
Maybe Ameer will want to post it for v2.

Thanks,
Amir.

Re: [PATCH 1/2] NFSD: Fix duplicate fsnotify access events for iterator READs

[Christoph Hellwig]

On Sat, Aug 22, 2026 at 12:21:12PM +0200, Amir Goldstein wrote:
> (1) In this series
> https://lore.kernel.org/linux-fsdevel/20231122122715.2561213-1-amir73il@gmail.com/
> we intentionally moved the permission hook outside
> of the splice iterators because we wanted to avoid calling them
> with freeze protection held and also there were some duplicate calls
> for this work.

This got me into a little rathole of looking into the other
do_splice_direct_actor callers.  And I still don't understand why
taking file_start_write outside the main splice machinery is fine
for splice_file_range callers, but not for do_splice_direct callers,
and what consideration exists for potential new callers.
(and yes, the naming does not help)

> (2) emitting many READ events from an iterator instead of one event for
> the user's READ request is more noisy and serves no purpose to users.
> In most cases (but not always) those events could be merged, but at the
> cost of futile CPU cycles.

Yes.

> >From a quick inspection of the code, it looks like:
> - fsnotify_access() is missing in vfs_splice_read()

Yes.  Then again I don't really understand vfs_splice_read, it basically
just forward ->splice_read.  I guess for backing_file this is
expected and matches what do_backing_file_read_iter does. for
code it looks weird as the context doesn't change at all.

> - the naming convention for splice_ do_splice_ vfs_splice_ is a horror

The entire cascade of do_*, *actor* and the whole structure of the
splіce code is horrible unfortunately.  Part of that is due to the
mess of inflicting a fake pipe for the fastpath callers that don't
need it, but paet of it is just self-inflicted bad naming.

> - we could make the low level splice_direct_to_actor() static and possibly
>   rename it to splice_direct_to_actor_sd() or something
> - we could export vfs_splice_direct_to_actor() for nfsd which wraps
>   splice_direct_to_actor() with permission hook and fsnotify_access

The latter is the right thing to do.

Re: [PATCH 1/2] NFSD: Fix duplicate fsnotify access events for iterator READs

[Amir Goldstein]

On Mon, Aug 24, 2026 at 6:57 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sat, Aug 22, 2026 at 12:21:12PM +0200, Amir Goldstein wrote:
> > (1) In this series
> > https://lore.kernel.org/linux-fsdevel/20231122122715.2561213-1-amir73il@gmail.com/
> > we intentionally moved the permission hook outside
> > of the splice iterators because we wanted to avoid calling them
> > with freeze protection held and also there were some duplicate calls
> > for this work.
>
> This got me into a little rathole of looking into the other
> do_splice_direct_actor callers.  And I still don't understand why
> taking file_start_write outside the main splice machinery is fine
> for splice_file_range callers, but not for do_splice_direct callers,
> and what consideration exists for potential new callers.

Tough question.
If I can retrace my steps:
The main consideration in the "Tidy up file permission hooks"
work was to move permission hooks outside of sb_start_write()
to avoid "first order deadlocks" from pre-content events (and LSMs),
but I think we also tried best effort to avoid holding
file_start_write(out) while
performing read on file in, to avoid "second order deadlocks" with
weird setups like:
https://lore.kernel.org/linux-fsdevel/5lz5jq7gzoejbywmh56ayfkdiuqsjd2s5pl5uvlflfxc5lq4rr@thr4hrkw67d2/

For the first order deadlocks, both flavors are fine:
1. permission + file_start_write() in ceph_copy_file_range() +
    splice_file_range()
2. permission + do_splice_direct() in vfs_copy_file_range()

->copy_file_range() are called from vfs_copy_file_range() with
file_start_write() held so ceph_copy_file_range() needs to use
the first flavor.

But at least it's holding file_start_write() on ceph fs while reading
a file from cephfs (maybe not the same sb though).

The use cases of do_splice_direct() from ovl copy up
nfsd/ksmbd copy_file_range (*) have more potential of hitting the
second order deadlocks, so they try to avoid them with more
granular file_start_write().

TBH, I don't think that we proved to what extent this helps avoid
the second order deadlocks because obviously, those deadlocks
are still possible.

(*) In the past copy_file_range(2) across different fs was also
a use case, but we stopped supporting that use case.

Anyway, there could definitely be other reasons for the
do_splice_direct/splice_file_range split which I do not remember,
but hey, at least the kerneldoc for these helpers is pretty clear...

Thanks,
Amir.