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;
--
I don't know if this needs to be further clarified to avoid confusion
when reading this code in the future?
> > I would point out that FAN_ACCESS_PERM already works
> > for directories and in effect provides (almost) the exact same
> > functionality as FAN_PRE_ACCESS without range info.
> >
> > But in order to get the FAN_ACCESS_PERM events on directories
> > listener would also be forced to get FAN_ACCESS_PERM on
> > special files and regular files
> > and assuming that this user is an HSM, it cannot request
> > FAN_ACCESS_PERM|FAN_ONDIR in the same mask as
> > FAN_PRE_ACCESS (which it needs for files) so it will need to
> > open another group for populating directories.
> >
> > So that's why I would maybe consider this a last minute fix to the new API.
>
> Yeah, this would be really a desperate way to get the functionality :)
Indeed :)
Thanks,
Amir.
