On Mon, Feb 5, 2024 at 8:27 PM Jan Kara <jack@xxxxxxx> wrote: > > I'm sorry for the delay. The last week was busy and this fell through the > cracks. > No worries, I was busy as well. I did already rebase fan_pre_content & fan_errno [1] (over v6.8-rc2) last week, made the small changes I mention here and ran some basic tests, but did not complete writing tests. Hoping to switch back to it this week. [1] https://github.com/amir73il/linux/commits/fan_errno > On Mon 29-01-24 20:30:34, Amir Goldstein wrote: > > On Mon, Dec 18, 2023 at 5:53 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > In the HttpDirFS HSM demo, I used FAN_OPEN_PERM on a mount mark > > > to deny open of file during the short time that it's content is being > > > punched out [1]. > > > It is quite complicated to explain, but I only used it for denying access, > > > not to fill content and not to write anything to filesystem. > > > It's worth noting that returning EBUSY in that case would be more meaningful > > > to users. > > > > > > That's one case in favor of allowing FAN_DENY_ERRNO for FAN_OPEN_PERM, > > > but mainly I do not have a proof that people will not need it. > > > > > > OTOH, I am a bit concerned that this will encourage developer to use > > > FAN_OPEN_PERM as a trigger to filling file content and then we are back to > > > deadlock risk zone. > > > > > > Not sure which way to go. > > > > > > Anyway, I think we agree that there is no reason to merge FAN_DENY_ERRNO > > > before FAN_PRE_* events, so we can continue this discussion later when > > > I post FAN_PRE_* patches - not for this cycle. > > > > I started to prepare the pre-content events patches for posting and got back > > to this one as well. > > > > Since we had this discussion I have learned of another use case that > > requires filling file content in FAN_OPEN_PERM hook, FAN_OPEN_EXEC_PERM > > to be exact. > > > > The reason is that unless an executable content is filled at execve() time, > > there is no other opportunity to fill its content without getting -ETXTBSY. > > Yes, I've been scratching my head over this usecase for a few days. I was > thinking whether we could somehow fill in executable (and executed) files on > access but it all seemed too hacky so I agree that we probably have to fill > them in on open. > Normally, I think there will not be a really huge executable(?) If there were huge executables, they would have likely been broken down into smaller loadable libraries which should allow more granular content filling, but I guess there will always be worst case exceptions. > > So to keep things more flexible, I decided to add -ETXTBSY to the > > allowed errors with FAN_DENY_ERRNO() and to decided to allow > > FAN_DENY_ERRNO() with all permission events. > > > > To keep FAN_DENY_ERRNO() a bit more focused on HSM, I have > > added a limitation that FAN_DENY_ERRNO() is allowed only for > > FAN_CLASS_PRE_CONTENT groups. > > I have no problem with adding -ETXTBSY to the set of allowed errors. That > makes sense. Adding FAN_DENY_ERRNO() to all permission events in > FAN_CLASS_PRE_CONTENT groups - OK, done that. I am still not very happy about FAN_OPEN_PERM being part of HSM event family when I know that O_TRUCT and O_CREAT call this hook with sb writers held. The irony, is that there is no chance that O_TRUNC will require filling content, same if the file is actually being created by O_CREAT, so the cases where sb writers is actually needed and the case where content filling is needed do not overlap, but I cannot figure out how to get those cases out of the HSM risk zone. Ideas? > if we don't find anything better - I > wanted to hash out another possibility here: Currently all permission > events (and thus also the events we plan to use for HSM AFAIU) are using > 'fd' to identify file where the event happened. This is used as identifier > for response, can be used to fill in file contents for HSM but it also > causes issues such as the problem with exec(2) occasionally failing if this > fd happens to get closed only after exec(2) gets to checking > deny_write_access(). So what if we implemented events needed for HSM as FID > events (we'd have think how to match replies to events)? Then the app would > open the file for filling in using FID as well as it would naturally close > the handle before replying so problems with exec(2) would not arise. These The two things are independent IMO. We can use an event->key instead of event->fd, which I like, but we may still leave event->fd as a way to get an FMODE_NONOTIFY fd as long as the user closes event->fd before responding or we can implement Sargun's suggestion of the FAN_CLOSE_FD response flag. If a user needs to open an FMODE_NONOTIFY fd from fid, we will need to provide a way to do that. My WIP pre-lookup event patches [2] implements inheritance of FMODE_NONOTIFY from dirfd used for openat(). Perhaps we can do the same for open_by_handle_at() and inherit FMODE_NONOTIFY from mount_fd to implement your suggestion? [2] https://github.com/amir73il/linux/commits/fan_lookup_perm > would be essentially new events (so far we didn't allow permission events > in FID groups) so allowing FAN_DENY_ERRNO() replies for them would be > natural. Overall it would seem like a cleaner "clean room implementation" > API? I like the idea of a clean slate. Looking a head, for the PRE_PATH events (e.g. lookup,create) I was planning to use FAN_EVENT_INFO_TYPE_DFID_NAME to carry the last component lookup name, but then also have event->fd as the dirfd of lookup/create. That's a bit ugly duplicity and also it does not cover rename(), because if we use FAN_EVENT_INFO_TYPE_{OLD,NEW}_DFID_NAME to report names, where would newdirfd, olddirfd be reported? Your suggestion solves both these questions elegantly and if you agree to adapting open_by_handle_at() to cater fanotify needs, then we have a plan to propose. The bad side of clean slate is that it reduces the chances of me being able to get pre-content events ready in time for 6.9, which is a shame, but we got to do what we got to do ;) Thanks, Amir.
