| Message ID | 20211014213646.1139469-19-krisman@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | file system-wide error monitoring | expand |
On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > > Error reporting needs to be done in an atomic context. This patch > introduces a group-wide mempool of error events, shared by all > marks in this group. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > fs/notify/fanotify/fanotify.c | 3 +++ > fs/notify/fanotify/fanotify.h | 11 +++++++++++ > fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++++++- > include/linux/fsnotify_backend.h | 2 ++ > 4 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 8f152445d75c..01d68dfc74aa 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -819,6 +819,9 @@ static void fanotify_free_group_priv(struct fsnotify_group *group) > if (group->fanotify_data.ucounts) > dec_ucount(group->fanotify_data.ucounts, > UCOUNT_FANOTIFY_GROUPS); > + > + if (mempool_initialized(&group->fanotify_data.error_events_pool)) > + mempool_exit(&group->fanotify_data.error_events_pool); > } > > static void fanotify_free_path_event(struct fanotify_event *event) > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > index c42cf8fd7d79..a577e87fac2b 100644 > --- a/fs/notify/fanotify/fanotify.h > +++ b/fs/notify/fanotify/fanotify.h > @@ -141,6 +141,7 @@ enum fanotify_event_type { > FANOTIFY_EVENT_TYPE_PATH, > FANOTIFY_EVENT_TYPE_PATH_PERM, > FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */ > + FANOTIFY_EVENT_TYPE_FS_ERROR, /* struct fanotify_error_event */ > __FANOTIFY_EVENT_TYPE_NUM > }; > > @@ -196,6 +197,16 @@ FANOTIFY_NE(struct fanotify_event *event) > return container_of(event, struct fanotify_name_event, fae); > } > > +struct fanotify_error_event { > + struct fanotify_event fae; > +}; > + > +static inline struct fanotify_error_event * > +FANOTIFY_EE(struct fanotify_event *event) > +{ > + return container_of(event, struct fanotify_error_event, fae); > +} > + > static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event) > { > if (event->type == FANOTIFY_EVENT_TYPE_FID) > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 66ee3c2805c7..f1cf863d6f9f 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -30,6 +30,7 @@ > #define FANOTIFY_DEFAULT_MAX_EVENTS 16384 > #define FANOTIFY_OLD_DEFAULT_MAX_MARKS 8192 > #define FANOTIFY_DEFAULT_MAX_GROUPS 128 > +#define FANOTIFY_DEFAULT_FEE_POOL 32 > We can probably start with a more generous pool (128?) It doesn't cost that much. But anyway, I think this pool needs to auto-grow (up to a maximum size) instead of having a rigid arbitrary limit. Thanks, Amir.
On Fri, Oct 15, 2021 at 9:19 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi > <krisman@collabora.com> wrote: > > > > Error reporting needs to be done in an atomic context. This patch > > introduces a group-wide mempool of error events, shared by all > > marks in this group. > > > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > > --- > > fs/notify/fanotify/fanotify.c | 3 +++ > > fs/notify/fanotify/fanotify.h | 11 +++++++++++ > > fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++++++- > > include/linux/fsnotify_backend.h | 2 ++ > > 4 files changed, 41 insertions(+), 1 deletion(-) > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > index 8f152445d75c..01d68dfc74aa 100644 > > --- a/fs/notify/fanotify/fanotify.c > > +++ b/fs/notify/fanotify/fanotify.c > > @@ -819,6 +819,9 @@ static void fanotify_free_group_priv(struct fsnotify_group *group) > > if (group->fanotify_data.ucounts) > > dec_ucount(group->fanotify_data.ucounts, > > UCOUNT_FANOTIFY_GROUPS); > > + > > + if (mempool_initialized(&group->fanotify_data.error_events_pool)) > > + mempool_exit(&group->fanotify_data.error_events_pool); > > } > > > > static void fanotify_free_path_event(struct fanotify_event *event) > > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > > index c42cf8fd7d79..a577e87fac2b 100644 > > --- a/fs/notify/fanotify/fanotify.h > > +++ b/fs/notify/fanotify/fanotify.h > > @@ -141,6 +141,7 @@ enum fanotify_event_type { > > FANOTIFY_EVENT_TYPE_PATH, > > FANOTIFY_EVENT_TYPE_PATH_PERM, > > FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */ > > + FANOTIFY_EVENT_TYPE_FS_ERROR, /* struct fanotify_error_event */ > > __FANOTIFY_EVENT_TYPE_NUM > > }; > > > > @@ -196,6 +197,16 @@ FANOTIFY_NE(struct fanotify_event *event) > > return container_of(event, struct fanotify_name_event, fae); > > } > > > > +struct fanotify_error_event { > > + struct fanotify_event fae; > > +}; > > + > > +static inline struct fanotify_error_event * > > +FANOTIFY_EE(struct fanotify_event *event) > > +{ > > + return container_of(event, struct fanotify_error_event, fae); > > +} > > + > > static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event) > > { > > if (event->type == FANOTIFY_EVENT_TYPE_FID) > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index 66ee3c2805c7..f1cf863d6f9f 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -30,6 +30,7 @@ > > #define FANOTIFY_DEFAULT_MAX_EVENTS 16384 > > #define FANOTIFY_OLD_DEFAULT_MAX_MARKS 8192 > > #define FANOTIFY_DEFAULT_MAX_GROUPS 128 > > +#define FANOTIFY_DEFAULT_FEE_POOL 32 > > > > We can probably start with a more generous pool (128?) > It doesn't cost that much. > But anyway, I think this pool needs to auto-grow (up to a maximum size) > instead of having a rigid arbitrary limit. > As long as the pool grows, I don't mind if it start at size 32, but I just noticed that mempools cannot be accounted to memcg?? Then surely the maximum size need to be kept pretty low. Thanks, Amir.
On Thu 14-10-21 18:36:36, Gabriel Krisman Bertazi wrote: > Error reporting needs to be done in an atomic context. This patch So this is a requirement I was advocating to remove because although atomic context is nice for filesystems, it is rather difficult for fanotify. Instead I was advocating that we relax this and require filesystems to report errors from a context where using GFP_NOFS allocations is fine. At least for ext4 and xfs and other filesystems I know it isn't really that much harder... If this proves to be a problem in some specific case, there are also other options (like doing notification from a workqueue, events are async anyway) for the filesystems to take. > introduces a group-wide mempool of error events, shared by all > marks in this group. ... "shared by all FS_ERROR marks", right? > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> The code otherwise looks OK to me. Honza > --- > fs/notify/fanotify/fanotify.c | 3 +++ > fs/notify/fanotify/fanotify.h | 11 +++++++++++ > fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++++++- > include/linux/fsnotify_backend.h | 2 ++ > 4 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 8f152445d75c..01d68dfc74aa 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -819,6 +819,9 @@ static void fanotify_free_group_priv(struct fsnotify_group *group) > if (group->fanotify_data.ucounts) > dec_ucount(group->fanotify_data.ucounts, > UCOUNT_FANOTIFY_GROUPS); > + > + if (mempool_initialized(&group->fanotify_data.error_events_pool)) > + mempool_exit(&group->fanotify_data.error_events_pool); > } > > static void fanotify_free_path_event(struct fanotify_event *event) > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > index c42cf8fd7d79..a577e87fac2b 100644 > --- a/fs/notify/fanotify/fanotify.h > +++ b/fs/notify/fanotify/fanotify.h > @@ -141,6 +141,7 @@ enum fanotify_event_type { > FANOTIFY_EVENT_TYPE_PATH, > FANOTIFY_EVENT_TYPE_PATH_PERM, > FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */ > + FANOTIFY_EVENT_TYPE_FS_ERROR, /* struct fanotify_error_event */ > __FANOTIFY_EVENT_TYPE_NUM > }; > > @@ -196,6 +197,16 @@ FANOTIFY_NE(struct fanotify_event *event) > return container_of(event, struct fanotify_name_event, fae); > } > > +struct fanotify_error_event { > + struct fanotify_event fae; > +}; > + > +static inline struct fanotify_error_event * > +FANOTIFY_EE(struct fanotify_event *event) > +{ > + return container_of(event, struct fanotify_error_event, fae); > +} > + > static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event) > { > if (event->type == FANOTIFY_EVENT_TYPE_FID) > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 66ee3c2805c7..f1cf863d6f9f 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -30,6 +30,7 @@ > #define FANOTIFY_DEFAULT_MAX_EVENTS 16384 > #define FANOTIFY_OLD_DEFAULT_MAX_MARKS 8192 > #define FANOTIFY_DEFAULT_MAX_GROUPS 128 > +#define FANOTIFY_DEFAULT_FEE_POOL 32 > > /* > * Legacy fanotify marks limits (8192) is per group and we introduced a tunable > @@ -1054,6 +1055,15 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, > return ERR_PTR(ret); > } > > +static int fanotify_group_init_error_pool(struct fsnotify_group *group) > +{ > + if (mempool_initialized(&group->fanotify_data.error_events_pool)) > + return 0; > + > + return mempool_init_kmalloc_pool(&group->fanotify_data.error_events_pool, > + FANOTIFY_DEFAULT_FEE_POOL, > + sizeof(struct fanotify_error_event)); > +} > > static int fanotify_add_mark(struct fsnotify_group *group, > fsnotify_connp_t *connp, unsigned int type, > @@ -1062,6 +1072,7 @@ static int fanotify_add_mark(struct fsnotify_group *group, > { > struct fsnotify_mark *fsn_mark; > __u32 added; > + int ret = 0; > > mutex_lock(&group->mark_mutex); > fsn_mark = fsnotify_find_mark(connp, group); > @@ -1072,13 +1083,26 @@ static int fanotify_add_mark(struct fsnotify_group *group, > return PTR_ERR(fsn_mark); > } > } > + > + /* > + * Error events are pre-allocated per group, only if strictly > + * needed (i.e. FAN_FS_ERROR was requested). > + */ > + if (!(flags & FAN_MARK_IGNORED_MASK) && (mask & FAN_FS_ERROR)) { > + ret = fanotify_group_init_error_pool(group); > + if (ret) > + goto out; > + } > + > added = fanotify_mark_add_to_mask(fsn_mark, mask, flags); > if (added & ~fsnotify_conn_mask(fsn_mark->connector)) > fsnotify_recalc_mask(fsn_mark->connector); > + > +out: > mutex_unlock(&group->mark_mutex); > > fsnotify_put_mark(fsn_mark); > - return 0; > + return ret; > } > > static int fanotify_add_vfsmount_mark(struct fsnotify_group *group, > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h > index a378a314e309..9941c06b8c8a 100644 > --- a/include/linux/fsnotify_backend.h > +++ b/include/linux/fsnotify_backend.h > @@ -19,6 +19,7 @@ > #include <linux/atomic.h> > #include <linux/user_namespace.h> > #include <linux/refcount.h> > +#include <linux/mempool.h> > > /* > * IN_* from inotfy.h lines up EXACTLY with FS_*, this is so we can easily > @@ -245,6 +246,7 @@ struct fsnotify_group { > int flags; /* flags from fanotify_init() */ > int f_flags; /* event_f_flags from fanotify_init() */ > struct ucounts *ucounts; > + mempool_t error_events_pool; > } fanotify_data; > #endif /* CONFIG_FANOTIFY */ > }; > -- > 2.33.0 >
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 8f152445d75c..01d68dfc74aa 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -819,6 +819,9 @@ static void fanotify_free_group_priv(struct fsnotify_group *group) if (group->fanotify_data.ucounts) dec_ucount(group->fanotify_data.ucounts, UCOUNT_FANOTIFY_GROUPS); + + if (mempool_initialized(&group->fanotify_data.error_events_pool)) + mempool_exit(&group->fanotify_data.error_events_pool); } static void fanotify_free_path_event(struct fanotify_event *event) diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index c42cf8fd7d79..a577e87fac2b 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -141,6 +141,7 @@ enum fanotify_event_type { FANOTIFY_EVENT_TYPE_PATH, FANOTIFY_EVENT_TYPE_PATH_PERM, FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */ + FANOTIFY_EVENT_TYPE_FS_ERROR, /* struct fanotify_error_event */ __FANOTIFY_EVENT_TYPE_NUM }; @@ -196,6 +197,16 @@ FANOTIFY_NE(struct fanotify_event *event) return container_of(event, struct fanotify_name_event, fae); } +struct fanotify_error_event { + struct fanotify_event fae; +}; + +static inline struct fanotify_error_event * +FANOTIFY_EE(struct fanotify_event *event) +{ + return container_of(event, struct fanotify_error_event, fae); +} + static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event) { if (event->type == FANOTIFY_EVENT_TYPE_FID) diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 66ee3c2805c7..f1cf863d6f9f 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -30,6 +30,7 @@ #define FANOTIFY_DEFAULT_MAX_EVENTS 16384 #define FANOTIFY_OLD_DEFAULT_MAX_MARKS 8192 #define FANOTIFY_DEFAULT_MAX_GROUPS 128 +#define FANOTIFY_DEFAULT_FEE_POOL 32 /* * Legacy fanotify marks limits (8192) is per group and we introduced a tunable @@ -1054,6 +1055,15 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, return ERR_PTR(ret); } +static int fanotify_group_init_error_pool(struct fsnotify_group *group) +{ + if (mempool_initialized(&group->fanotify_data.error_events_pool)) + return 0; + + return mempool_init_kmalloc_pool(&group->fanotify_data.error_events_pool, + FANOTIFY_DEFAULT_FEE_POOL, + sizeof(struct fanotify_error_event)); +} static int fanotify_add_mark(struct fsnotify_group *group, fsnotify_connp_t *connp, unsigned int type, @@ -1062,6 +1072,7 @@ static int fanotify_add_mark(struct fsnotify_group *group, { struct fsnotify_mark *fsn_mark; __u32 added; + int ret = 0; mutex_lock(&group->mark_mutex); fsn_mark = fsnotify_find_mark(connp, group); @@ -1072,13 +1083,26 @@ static int fanotify_add_mark(struct fsnotify_group *group, return PTR_ERR(fsn_mark); } } + + /* + * Error events are pre-allocated per group, only if strictly + * needed (i.e. FAN_FS_ERROR was requested). + */ + if (!(flags & FAN_MARK_IGNORED_MASK) && (mask & FAN_FS_ERROR)) { + ret = fanotify_group_init_error_pool(group); + if (ret) + goto out; + } + added = fanotify_mark_add_to_mask(fsn_mark, mask, flags); if (added & ~fsnotify_conn_mask(fsn_mark->connector)) fsnotify_recalc_mask(fsn_mark->connector); + +out: mutex_unlock(&group->mark_mutex); fsnotify_put_mark(fsn_mark); - return 0; + return ret; } static int fanotify_add_vfsmount_mark(struct fsnotify_group *group, diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index a378a314e309..9941c06b8c8a 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -19,6 +19,7 @@ #include <linux/atomic.h> #include <linux/user_namespace.h> #include <linux/refcount.h> +#include <linux/mempool.h> /* * IN_* from inotfy.h lines up EXACTLY with FS_*, this is so we can easily @@ -245,6 +246,7 @@ struct fsnotify_group { int flags; /* flags from fanotify_init() */ int f_flags; /* event_f_flags from fanotify_init() */ struct ucounts *ucounts; + mempool_t error_events_pool; } fanotify_data; #endif /* CONFIG_FANOTIFY */ };
Error reporting needs to be done in an atomic context. This patch introduces a group-wide mempool of error events, shared by all marks in this group. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/notify/fanotify/fanotify.c | 3 +++ fs/notify/fanotify/fanotify.h | 11 +++++++++++ fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++++++- include/linux/fsnotify_backend.h | 2 ++ 4 files changed, 41 insertions(+), 1 deletion(-)