Re: [GIT PULL] Fsnotify changes for 6.12-rc1

[Jan Kara <jack@suse.cz>]

On Tue 24-09-24 18:15:51, Linus Torvalds wrote:
> On Tue, 24 Sept 2024 at 17:17, Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> >
> > Just side note: I think `generic_file_vm_ops` already prepares this
> > feature, so generic_file_mmap users also have fault around behaviors.
> 
> Hmm. Maybe. But it doesn't really change the fundamental issue - the
> code in question seems to be just *random*.
> 
> And I mean that in a very real and very immediate sense: the
> fault-around code and filemap_map_pages() only maps in pages that are
> uptodate, so it literally DEPENDS ON TIMING whether some previous IO
> has completed or not, and thus on whether the page fault is handled by
> the fault-around in filemap_map_pages() or by the filemap_fault()
> code.
> 
> In other words - I think this is all completely broken.
> 
> Put another way: explain to me why random IO timing details should
> matter for the whether we do __filemap_fsnotify_fault() on a page
> fault or not?

I agree that with the fault-around code, there's dependency on IO
completion time. So we can still be generating PRE_ACCESS events while the
data is being loaded from the disk. This was a compromise we ended up with
after quite some discussions about possible solutions. Generally the
options I see for page faults are:

1) Generate PRE_ACCESS event whenever a page is being mapped into page
tables (in fact Josef originally had this in his patch). This would
provide the determinism at the cost of performance (events generated even
for cached pages). If this is OK with you, we can do that but from what I
gather from your previous emails you don't really like this either.

2) We could generate event in filemap_fault() only if we don't find
uptodate folio there. I'm happy to do this if it looks better to you
although I don't think there's a practical difference from the current
state (the same raciness with IO completion applies, just the window is
smaller).

3) We could generate event in filemap_fault() only if we didn't find a
folio in the page cache or we found it, locked it, but it still was not
uptodate. This will make sure we generate the event only if we really need
to pull in the data into the page cache. Doable. The case when we found &
locked a page that's not uptodate in the end will be a bit ugly but not too
bad and this case should be rare. I actually like this option the most I
guess. 

> So no. I'm not taking this pull request. It makes absolutely zero
> sense to me, and I don't think it has sane semantics.  The argument
> that it is already used by people is not an argument.

I mentioned that to show that there's practical interest in this kind of
functionality. I think people involved understand things are pretty much in
flux until they are merged upstream.

> The new fsnotify hooks need to make SENSE - not be in random locations
> that give some kind of random data.

Thanks for feedback. So 1) and 3) from the above options make sense to me
(in the sense that it is relatively easy to explain when events are
generated). I'd prefer 3) for performance reasons so can we settle on that?

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