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.
>
> > > 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.
>
> >
> > > @@ -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.
The challenge is that all internal fsnotify code uses __u32 masks
and so do {i,sb,mnt}_fsnotify_mask.
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.
WDYT?
Thanks,
Amir.
From 01295be4df1053b83a33c80fda138ec5734ac858 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Thu, 12 Dec 2024 15:11:28 +0100
Subject: [PATCH] fsnotify: demo send all mount events to mount watchers
Should be changed to send mount event to all groups with mntns
marks on the respective mntns.
---
fs/notify/fanotify/fanotify.c | 23 +++++++++++++++++------
fs/notify/fsnotify.c | 16 +++++++++-------
include/linux/fsnotify_backend.h | 8 +++++---
3 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 95646f7c46ca..d1c05e2a12da 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -296,7 +296,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
*/
static u32 fanotify_group_event_mask(struct fsnotify_group *group,
struct fsnotify_iter_info *iter_info,
- u32 *match_mask, u32 event_mask,
+ u32 *match_mask, u64 event_mask,
const void *data, int data_type,
struct inode *dir)
{
@@ -309,10 +309,14 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
bool ondir = event_mask & FAN_ONDIR;
int type;
- pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
+ pr_debug("%s: report_mask=%x mask=%llx data=%p data_type=%d\n",
__func__, iter_info->report_mask, event_mask, data, data_type);
- if (!fid_mode) {
+ if (event_mask & FS_MOUNT_EVENTS) {
+ /* Mount events are not about a specific path */
+ if (path)
+ return 0;
+ } else if (!fid_mode) {
/* Do we have path to open a file descriptor? */
if (!path)
return 0;
@@ -326,6 +330,8 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
}
fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
+ if (type == FSNOTIFY_ITER_TYPE_VFSMOUNT)
+ marks_mask |= FS_MOUNT_EVENTS;
/*
* Apply ignore mask depending on event flags in ignore mask.
*/
@@ -720,7 +726,7 @@ static struct fanotify_event *fanotify_alloc_error_event(
static struct fanotify_event *fanotify_alloc_event(
struct fsnotify_group *group,
- u32 mask, const void *data, int data_type,
+ u64 mask, const void *data, int data_type,
struct inode *dir, const struct qstr *file_name,
__kernel_fsid_t *fsid, u32 match_mask)
{
@@ -826,6 +832,11 @@ static struct fanotify_event *fanotify_alloc_event(
moved, &hash, gfp);
} else if (fid_mode) {
event = fanotify_alloc_fid_event(id, fsid, &hash, gfp);
+ } else if (mask & FS_MOUNT_EVENTS) {
+ mask <<= 32;
+ struct path null_path = {};
+ /* XXX: allocate mount event */
+ event = fanotify_alloc_path_event(&null_path, &hash, gfp);
} else {
event = fanotify_alloc_path_event(path, &hash, gfp);
}
@@ -892,7 +903,7 @@ static void fanotify_insert_event(struct fsnotify_group *group,
hlist_add_head(&event->merge_list, hlist);
}
-static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
+static int fanotify_handle_event(struct fsnotify_group *group, u64 mask,
const void *data, int data_type,
struct inode *dir,
const struct qstr *file_name, u32 cookie,
@@ -934,7 +945,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
if (!mask)
return 0;
- pr_debug("%s: group=%p mask=%x report_mask=%x\n", __func__,
+ pr_debug("%s: group=%p mask=%llx report_mask=%x\n", __func__,
group, mask, match_mask);
if (fanotify_is_perm_event(mask)) {
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 8ee495a58d0a..e1833d7b9b3c 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -372,13 +372,13 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
dir, name, cookie);
}
-static int send_to_group(__u32 mask, const void *data, int data_type,
+static int send_to_group(__u64 mask, const void *data, int data_type,
struct inode *dir, const struct qstr *file_name,
u32 cookie, struct fsnotify_iter_info *iter_info)
{
struct fsnotify_group *group = NULL;
- __u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS);
- __u32 marks_mask = 0;
+ __u64 test_mask = (mask & ALL_FSNOTIFY_EVENTS);
+ __u64 marks_mask = 0;
__u32 marks_ignore_mask = 0;
bool is_dir = mask & FS_ISDIR;
struct fsnotify_mark *mark;
@@ -398,13 +398,15 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
/* Are any of the group marks interested in this event? */
fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
+ if (type == FSNOTIFY_ITER_TYPE_VFSMOUNT)
+ marks_mask |= FS_MOUNT_EVENTS;
group = mark->group;
marks_mask |= mark->mask;
marks_ignore_mask |=
fsnotify_effective_ignore_mask(mark, is_dir, type);
}
- pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
+ pr_debug("%s: group=%p mask=%llx marks_mask=%llx marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
__func__, group, mask, marks_mask, marks_ignore_mask,
data, data_type, dir, cookie);
@@ -533,7 +535,7 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
* reported to both.
* @cookie: inotify rename cookie
*/
-int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
+int fsnotify(__u64 mask, const void *data, int data_type, struct inode *dir,
const struct qstr *file_name, struct inode *inode, u32 cookie)
{
const struct path *path = fsnotify_data_path(data, data_type);
@@ -545,7 +547,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
struct dentry *moved;
int inode2_type;
int ret = 0;
- __u32 test_mask, marks_mask;
+ __u64 test_mask, marks_mask;
if (path)
mnt = real_mount(path->mnt);
@@ -583,7 +585,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
marks_mask = READ_ONCE(sb->s_fsnotify_mask);
if (mnt)
- marks_mask |= READ_ONCE(mnt->mnt_fsnotify_mask);
+ marks_mask |= READ_ONCE(mnt->mnt_fsnotify_mask) | FS_MOUNT_EVENTS;
if (inode)
marks_mask |= READ_ONCE(inode->i_fsnotify_mask);
if (inode2)
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 0d24a21a8e60..58b5ac75f5f4 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -106,6 +106,8 @@
*/
#define FS_EVENTS_POSS_TO_PARENT (FS_EVENTS_POSS_ON_CHILD)
+#define FS_MOUNT_EVENTS (0xfUL << 32)
+
/* Events that can be reported to backends */
#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \
FS_EVENTS_POSS_ON_CHILD | \
@@ -162,7 +164,7 @@ struct mem_cgroup;
* userspace messages that marks have been removed.
*/
struct fsnotify_ops {
- int (*handle_event)(struct fsnotify_group *group, u32 mask,
+ int (*handle_event)(struct fsnotify_group *group, u64 mask,
const void *data, int data_type, struct inode *dir,
const struct qstr *file_name, u32 cookie,
struct fsnotify_iter_info *iter_info);
@@ -605,7 +607,7 @@ struct fsnotify_mark {
/* called from the vfs helpers */
/* main fsnotify call to send events */
-extern int fsnotify(__u32 mask, const void *data, int data_type,
+extern int fsnotify(__u64 mask, const void *data, int data_type,
struct inode *dir, const struct qstr *name,
struct inode *inode, u32 cookie);
extern int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
@@ -906,7 +908,7 @@ static inline int fsnotify_pre_content(const struct path *path,
return 0;
}
-static inline int fsnotify(__u32 mask, const void *data, int data_type,
+static inline int fsnotify(__u64 mask, const void *data, int data_type,
struct inode *dir, const struct qstr *name,
struct inode *inode, u32 cookie)
{
--
2.34.1
