On Mon, Apr 7, 2025 at 4:39 PM Jan Kara <jack@xxxxxxx> wrote: > > On Mon 31-03-25 18:52:31, Amir Goldstein wrote: > > In addition to FAN_PRE_ACCESS event before any access to a file range, > > also report the FAN_PRE_MODIFY event in case of a write access. > > > > This will allow userspace to subscribe only to pre-write access > > notifications and to respond with error codes associated with write > > operations using the FAN_DENY_ERRNO macro. > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > --- > > > > Jan, > > > > I was looking on th list for the reason we decided to drop FAN_PRE_MODIFY > > from the pre-content patch set and I couldn't find it. It may have been > > related to complications ot page fault hooks that are not not relevant? > > Two reasons really: > 1) Defining semantics of FAN_PRE_MODIFY when it is generated on page fault > was hard and was making the changes even more complex. > > 2) Without a real user of the event in userspace, I have doubts we'll get > the semantics right. > Good now we have the reasons documented :) > > I did find the decision to generate FAN_PRE_ACCESS on both read()/write(), > > so maybe we thought there was no clear value for the FAN_PRE_MODIFY event? > > > > In any case, I realized that we allowed returning custom errors with > > FAN_DENY_ERRNO(ENOSPC), but that chosing the right error does require > > knowing whether the call was read() or write(). > > I see your point but if someone wants to read a file, your HSM server > fetches the data and wants to store them in the filesystem and gets ENOSPC, > then what do you want to return? I agree returning ENOSPC is confusing but > OTOH anything else will be confusing even more (in the sense "why did I get > this error?")? > Good question. One option (ENOSPC) may confuse applications and another option (EIO) will lack the information of the real underlying reason. I guess the only way would be to leave this decision to userspace, only userspace cannot make that decision now :-/ But overall, I agree that this is probably not a very urgent issue and that we can deal with it later and backport FAN_PRE_MODIFY if people really need it. I mostly wanted to make sure that we all understood why FAN_PRE_MODIFY was postponed for now and that it wasn't just a page fault hook left over. > > Becaue mmap() cannot return write() errors like ENOSPC, I decided not > > to generate FAN_PRE_MODIFY for writably-shared maps, but maybe we should > > consider this. > > Generally, the semantics of events for mmap needs a careful thinking so > that we provide all the needed events while maintaining sensible > performance. And for that I think we need a better formulated understanding > of what is the event going to be used for? For example for FAN_PRE_ACCESS > event we now generate the event on mmap(2). Now what should userspace do if > it wants to move some file to "slow tier" and get the event again when the > file is used again? Is that even something we plan to support? I don't think that we should. My HSM POC takes an exclusive write lease on a file before evicting its content so any read/write refcount on the inode would keep the content on the file pinned. I did have a problem with another use case - for change tracking, generating FAN_PRE_MODIFY on mmap() of writably shared mem does not quite cut it. For that use case, it may make more sense to generate one FAN_PRE_ACCESS range event on mmap() and page range FAN_PRE_MODIFY on write faults. I am starting to remember why we dropped FAN_PRE_MODIFY... Thanks, Amir.
