On Tue 24-09-24 18:15:51, Linus Torvalds wrote: > On Tue, 24 Sept 2024 at 17:17, Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx> 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@xxxxxxxx> SUSE Labs, CR
