On Sun, Jun 29, 2025 at 8:24 AM Ibrahim Jirdeh <ibrahimjirdeh@xxxxxxxx> wrote: > > > > > Do we prefer to scope this change to adding (s32) response id and not add new > > > > event id field yet. > > > > > > > > > Thinking out loud, if we use idr to allocate an event id, as Jan suggested, > > > > > and we do want to allow it along side event->fd, > > > > > then we could also overload event->pid, i.e. the meaning of > > > > > FAN_ERPORT_EVENT_ID would be "event->pid is an event id", > > > > > Similarly to the way that we overloaded event->pid with FAN_REPORT_TID. > > > > > Users that need both event id and pid can use FAN_REPORT_PIDFD. > > > > > > > > > > > > > At least for our usecase, having event->fd along with response id available > > > > would be helpful as we do not use fid mode mentioned above. > > > > > > You cannot use the fid mode mentioned above because it is not yet > > > supported with pre-content events :) > > > > > > My argument goes like this: > > > 1. We are planning to add fid support for pre-content events for other > > > reasons anyway (pre-dir-content events) > > > 2. For this mode, event->fd will (probably) not be reported anyway, > > > so for this mode, we will have to use a different response id > > > 3. Since event->fd will not be used, it would make a lot of sense and > > > very natural to reuse the field for a response id > > > > > > So if we accept the limitation that writing an advanced hsm service > > > that supports non-interrupted restart requires that service to use the > > > new fid mode, we hit two birds with one event field ;) > > > > > > If we take into account that (the way I see it) an advanced hsm service > > > will need to also support pre-dir-content events, then the argument makes > > > even more sense. > > > > > Ah I see this makes sense. And as long as we are still able to open files via > open_handle as the tests you shared below show, then at least for our case I > don't see issue with switching to the new FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID > mode. > > > > The fact that for your current use cases, you are ok with populating the > > > entire directory tree in a non-lazy way, does not mean that the use case > > > will not change in the future to require a lazy population of directory trees. > > > > > > I have another "hidden motive" with the nudge trying to push you over > > > towards pre-content events in fid mode: > > > > > > Allowing pre-content events together with legacy events in the same > > > mark/group brings up some ugly semantic issues that we did not > > > see when we added the API. > > > > > > The flag combination FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID > > > was never supported, so when we support it, we can start fresh with new rules > > > like "only pre-content events are allowed in this group" and that simplifies > > > some of the API questions. > > > > > > While I have your attention I wanted to ask, as possibly the only > > > current user of pre-content events, is any of the Meta use cases > > > setting any other events in the mask along with pre-content events? > > > > > Regarding Meta usages of pre-content events as far as I am aware we are the > only ones, and we don't set other events in the mask, only pre-content. I can > confirm there are no other use cases relying on this and follow up here. > > > > *if* we agree to this course I can post a patch to add support for > > > FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID, temporarily > > > leaving event->fd in use, so that you can later replace it with > > > a response id. > > > > > > > Sounds great. If we do go that route, being able to overload event-fd for now > will definitely make this patch much simpler. > I may have mentioned this before, but I'll bring it up again. If we want to overload event->fd with response id I would consider allocating response_id with idr_alloc_cyclic() that starts at 256 and then set event->fd = -response_id. We want to skip the range -1..-255 because it is used to report fd open errors with FAN_REPORT_FD_ERROR. Beyond a clear distinction of type by the value of the field, this will avert bugs where programers leave old code to close(event->fd) (or LLM coding agent grabs it from man page). Thanks, Amir.
