On Fri 04-04-25 12:44:11, Amir Goldstein wrote:
> On Fri, Apr 4, 2025 at 11:53 AM Jan Kara <jack@xxxxxxx> wrote:
> >
> > On Thu 03-04-25 19:24:57, Amir Goldstein wrote:
> > > On Thu, Apr 3, 2025 at 7:10 PM Jan Kara <jack@xxxxxxx> wrote:
> > > >
> > > > On Wed 02-04-25 08:27:07, Amir Goldstein wrote:
> > > > > Like files, a FAN_PRE_ACCESS event will be generated before every
> > > > > read access to directory, that is on readdir(3).
> > > > >
> > > > > Unlike files, there will be no range info record following a
> > > > > FAN_PRE_ACCESS event, because the range of access on a directory
> > > > > is not well defined.
> > > > >
> > > > > FAN_PRE_ACCESS events on readdir are only generated when user opts-in
> > > > > with FAN_ONDIR request in event mask and the FAN_PRE_ACCESS events on
> > > > > readdir report the FAN_ONDIR flag, so user can differentiate them from
> > > > > event on read.
> > > > >
> > > > > An HSM service is expected to use those events to populate directories
> > > > > from slower tier on first readdir access. Having to range info means
> > > > > that the entire directory will need to be populated on the first
> > > > > readdir() call.
> > > > >
> > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > > > ---
> > > > >
> > > > > Jan,
> > > > >
> > > > > IIRC, the reason we did not allow FAN_ONDIR with FAN_PRE_ACCESS event
> > > > > in initial API version was due to uncertainty around reporting range info.
> > > > >
> > > > > Circling back to this, I do not see any better options other than not
> > > > > reporting range info and reporting the FAN_ONDIR flag.
> > > > >
> > > > > HSM only option is to populate the entire directory on first access.
> > > > > Doing a partial range populate for directories does not seem practical
> > > > > with exising POSIX semantics.
> > > >
> > > > I agree that range info for directory events doesn't make sense (or better
> > > > there's no way to have a generic implementation since everything is pretty
> > > > fs specific). If I remember our past discussion, filling in directory
> > > > content on open has unnecessarily high overhead because the user may then
> > > > just do e.g. lookup in the opened directory and not full readdir. That's
> > > > why you want to generate it on readdir. Correct?
> > > >
> > >
> > > Right.
> > >
> > > > > If you accept this claim, please consider fast tracking this change into
> > > > > 6.14.y.
> > > >
> > > > Hum, why the rush? It is just additional feature to allow more efficient
> > > > filling in of directory entries...
> > > >
> > >
> > > Well, no rush really.
> > >
> > > My incentive is not having to confuse users with documentation that
> > > version X supports FAN_PRE_ACCESS but only version Y supports
> > > it with FAN_ONDIR.
> > >
> > > It's not a big deal, but if we have no reason to delay this, I'd just
> > > treat it as a fix to the new api (removing unneeded limitations).
> >
> > The patch is easy enough so I guess we may push it for rc2. When testing
> > it, I've noticed a lot of LTP test cases fail (e.g. fanotify02) because they
> > get unexpected event. So likely the patch actually breaks something in
> > reporting of other events. I don't have time to analyze it right now so I'm
> > just reporting it in case you have time to have a look...
> >
>
> Damn I should have tested that.
> Patch seemed so irrelevant for non pre-content events that it eluded me.
>
> It took me a good amount of staring time until I realized that both
> FANOTIFY_OUTGOING_EVENTS and FANOTIFY_EVENT_FLAGS
> include the FAN_ONDIR flag.
>
> This diff fixes the failing tests:
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 531c038eed7c..f90598044ec9 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -373,6 +373,8 @@ static u32 fanotify_group_event_mask(struct
> fsnotify_group *group,
> user_mask |= FANOTIFY_EVENT_FLAGS;
> } else if (test_mask & FANOTIFY_PRE_CONTENT_EVENTS) {
> user_mask |= FAN_ONDIR;
> + } else {
> + user_mask &= ~FANOTIFY_EVENT_FLAGS;
> }
>
> return test_mask & user_mask;
Thanks. What I actually ended up with is below. It passes all the tests and
I think that returning FANOTIFY_EVENT_ON_CHILD in the mask is actually more
consistent? Definitely less cases to consider... But please holler if you
see any problem with that.
Honza
