On Thu, Dec 12, 2024 at 5:04 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Thu 12-12-24 16:02:16, Amir Goldstein wrote:
> > On Thu, Dec 12, 2024 at 1:45 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > >
> > > On Thu, 12 Dec 2024 at 12:27, Jan Kara <jack@xxxxxxx> wrote:
> > >
> > > > Why not:
> > > > if (p->prev_ns == p->mnt_ns) {
> > > > fsnotify_mnt_move(p->mnt_ns, &p->mnt);
> > > > return;
> > > > }
> > >
> > > I don't really care, but I think this fails both as an optimization
> > > (zero chance of actually making a difference) and as a readability
> > > improvement.
>
> I was just staring at the code trying to understand why you special-case
> the situations with non-existent prev / current ns until I understood
> there's no real reason. But I agree it's a matter of a taste so I'm fine
> with keeping things as you have them.
>
> > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > > > index 24c7c5df4998..a9dc004291bf 100644
> > > > > --- a/fs/notify/fanotify/fanotify.c
> > > > > +++ b/fs/notify/fanotify/fanotify.c
> > > > > @@ -166,6 +166,8 @@ static bool fanotify_should_merge(struct fanotify_event *old,
> > > > > case FANOTIFY_EVENT_TYPE_FS_ERROR:
> > > > > return fanotify_error_event_equal(FANOTIFY_EE(old),
> > > > > FANOTIFY_EE(new));
> > > > > + case FANOTIFY_EVENT_TYPE_MNT:
> > > > > + return false;
> > > >
> > > > Perhaps instead of handling this in fanotify_should_merge(), we could
> > > > modify fanotify_merge() directly to don't even try if the event is of type
> > > > FANOTIFY_EVENT_TYPE_MNT? Similarly as we do it there for permission events.
> > >
> > > Okay.
> >
> > Actually, I disagree.
> > For permission events there is a conceptual reason not to merge,
> > but this is not true for mount events.
> >
> > Miklos said that he is going to add a FAN_MOUNT_MODIFY event
> > for changing mount properties and we should very much merge multiple
> > mount modify events.
> >
> > Furthermore, I wrote my comment about not merging mount events
> > back when the mount event info included the parent mntid.
> > Now that the mount event includes only the mount's mntid itself,
> > multiple mount moves *could* actually be merged to a single move
> > and a detach + attach could be merged to a move.
> > Do we want to merge mount move events? that is a different question
> > I guess we don't, but any case this means that the check should remain
> > where it is now, so that we can check for specific mount events in the
> > mask to decide whether or not to merge them.
>
> Ok, fair enough. What triggered this request was that currently we just
> look at each event in the queue, ask for each one "can we merge" only to
> get "cannot" answer back. Which seemed dumb. But if we are going to add
> events that can be merged, this reason obviously doesn't hold anymore. So
> I'm retracting my objection :)
>
> > > > > @@ -303,7 +305,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > > > > pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
> > > > > __func__, iter_info->report_mask, event_mask, data, data_type);
> > > > >
> > > > > - if (!fid_mode) {
> > > > > + if (FAN_GROUP_FLAG(group, FAN_REPORT_MNT))
> > > > > + {
> > > >
> > > > Unusual style here..
> > >
> > > Yeah, fixed.
> > >
> > > > Now if we expect these mount notification groups will not have more than
> > > > these two events, then probably it isn't worth the hassle. If we expect
> > > > more event types may eventually materialize, it may be worth it. What do
> > > > people think?
> > >
> > > I have a bad feeling about just overloading mask values. How about
> > > reserving a single mask bit for all mount events? I.e.
> > >
> > > #define FAN_MNT_ATTACH 0x00100001
> > > #define FAN_MNT_DETACH 0x00100002
> >
> > This is problematic.
> > Because the bits reported in event->mask are often masked
> > using this model makes assumptions that are not less risky
> > that the risk of overloading 0x1 0x2 IMO.
> >
> > I was contemplating deferring the decision about overloading for a while
> > by using high bits for mount events.
> > fanotify_mark() mask is already 64bit with high bits reserved
> > and fanotify_event_metadata->mask is also 64bit.
>
> Oh, right, fanotify API actually has a plenty of bits. I forgot that the
> type is different from the generic one in fsnotify. Thanks for reminding
> me!
>
> > The challenge is that all internal fsnotify code uses __u32 masks
> > and so do {i,sb,mnt}_fsnotify_mask.
>
> Yeah, including struct fanotify_event.
>
> > However, as I have already claimed, maintaining the mount event bits
> > in the calculated object mask is not very helpful IMO.
> >
> > Attached demo patch that sends all mount events to group IFF
> > group has a mount mark.
> >
> > This is quite simple, but could also be extended later with a little
> > more work to allow sb/mount mark to actually subscribe to mount events
> > or to ignore mount events for a specific sb/mount, if we think this is useful.
>
> So I like the prospect of internal event type eventually becoming 64-bit
> but I don't think we should tie it to this patch set given we still have 7
> bits left in the internal mask. Also if we do the conversion, I'd like to
> go full 64-bit except for the very few places that have a good reason so
> stay 32-bit. Because otherwise it's very easy to loose the upper bits
> somewhere. So what we could do is to allocate the new FAN_MNT_ constants
> from the upper 32 bits, for now leave FS_MNT_ in the lower 32 bits, and do
> the conversions as I've mentioned. When we really start running out of
> internal mask bits, we can implement the 64-bit event constant handling in
> fsnotify core and move FS_MNT_ constants in the upper bits.
>
Fair enough.
As long as I get to call dibs on the planned HSM event bits ;)
So please shift the FS_MNT_ event bits one nibble left, so we can have this:
#define FS_OPEN_PERM 0x00010000 /* open event in an
permission hook */
#define FS_ACCESS_PERM 0x00020000 /* access event in a
permissions hook */
#define FS_OPEN_EXEC_PERM 0x00040000 /* open/exec event in
a permission hook */
#define FS_PATH_MODIFY 0x00080000 /* Path pre-modify HSM hook */
#define FS_PRE_ACCESS 0x00100000 /* File pre-access HSM hook */
#define FS_PRE_MODIFY 0x00200000 /* File pre-modify HSM hook */
#define FS_PATH_ACCESS 0x00400000 /* Path pre-lookup HSM hook */
#define FS_MNT_ATTACH 0x01000000 /* Mount was attached */
#define FS_MNT_DETACH 0x02000000 /* Mount was detached */
#define FS_MNT_MODIFY 0x04000000 /* Mount was modified */
Thanks,
Amir.
