| Message ID | 20211014213646.1139469-20-krisman@collabora.com |
|---|---|
| State | Not Applicable |
| 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: > > Since FAN_FS_ERROR memory must be pre-allocated, limit a single group > from watching too many file systems at once. The current scheme > guarantees 1 slot per filesystem, so limit the number of marks with > FAN_FS_ERROR per group. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > fs/notify/fanotify/fanotify_user.c | 10 ++++++++++ > include/linux/fsnotify_backend.h | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index f1cf863d6f9f..5324890500fc 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -959,6 +959,10 @@ static int fanotify_remove_mark(struct fsnotify_group *group, > > removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags, > umask, &destroy_mark); > + > + if (removed & FAN_FS_ERROR) > + group->fanotify_data.error_event_marks--; > + > if (removed & fsnotify_conn_mask(fsn_mark->connector)) > fsnotify_recalc_mask(fsn_mark->connector); > if (destroy_mark) > @@ -1057,6 +1061,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, > > static int fanotify_group_init_error_pool(struct fsnotify_group *group) > { > + if (group->fanotify_data.error_event_marks >= FANOTIFY_DEFAULT_FEE_POOL) > + return -ENOMEM; Why not try to mempool_resize()? Also, I did not read the rest of the patches yet, but don't we need two slots per mark? one for alloc-pre-enqueue and one for free-post-dequeue? Thanks, Amir.
Amir Goldstein <amir73il@gmail.com> writes: > On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi > <krisman@collabora.com> wrote: >> >> Since FAN_FS_ERROR memory must be pre-allocated, limit a single group >> from watching too many file systems at once. The current scheme >> guarantees 1 slot per filesystem, so limit the number of marks with >> FAN_FS_ERROR per group. >> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> >> --- >> fs/notify/fanotify/fanotify_user.c | 10 ++++++++++ >> include/linux/fsnotify_backend.h | 1 + >> 2 files changed, 11 insertions(+) >> >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c >> index f1cf863d6f9f..5324890500fc 100644 >> --- a/fs/notify/fanotify/fanotify_user.c >> +++ b/fs/notify/fanotify/fanotify_user.c >> @@ -959,6 +959,10 @@ static int fanotify_remove_mark(struct fsnotify_group *group, >> >> removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags, >> umask, &destroy_mark); >> + >> + if (removed & FAN_FS_ERROR) >> + group->fanotify_data.error_event_marks--; >> + >> if (removed & fsnotify_conn_mask(fsn_mark->connector)) >> fsnotify_recalc_mask(fsn_mark->connector); >> if (destroy_mark) >> @@ -1057,6 +1061,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, >> >> static int fanotify_group_init_error_pool(struct fsnotify_group *group) >> { >> + if (group->fanotify_data.error_event_marks >= FANOTIFY_DEFAULT_FEE_POOL) >> + return -ENOMEM; > > Why not try to mempool_resize()? Jan suggested we might not need to bother with it, but I can do that for the next version. > Also, I did not read the rest of the patches yet, but don't we need two > slots per mark? one for alloc-pre-enqueue and one for free-post-dequeue? I don't understand what you mean by two slots for alloc-pre-enqueue and free-post-dequeue. I suspect it is no longer necessary now that FAN_FS_ERROR is handled like any other event on enqueue/dequeue, but can you confirm or clarify? Thanks,
On Fri, Oct 15, 2021 at 7:53 PM Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > > Amir Goldstein <amir73il@gmail.com> writes: > > > On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi > > <krisman@collabora.com> wrote: > >> > >> Since FAN_FS_ERROR memory must be pre-allocated, limit a single group > >> from watching too many file systems at once. The current scheme > >> guarantees 1 slot per filesystem, so limit the number of marks with > >> FAN_FS_ERROR per group. > >> > >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > >> --- > >> fs/notify/fanotify/fanotify_user.c | 10 ++++++++++ > >> include/linux/fsnotify_backend.h | 1 + > >> 2 files changed, 11 insertions(+) > >> > >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > >> index f1cf863d6f9f..5324890500fc 100644 > >> --- a/fs/notify/fanotify/fanotify_user.c > >> +++ b/fs/notify/fanotify/fanotify_user.c > >> @@ -959,6 +959,10 @@ static int fanotify_remove_mark(struct fsnotify_group *group, > >> > >> removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags, > >> umask, &destroy_mark); > >> + > >> + if (removed & FAN_FS_ERROR) > >> + group->fanotify_data.error_event_marks--; > >> + > >> if (removed & fsnotify_conn_mask(fsn_mark->connector)) > >> fsnotify_recalc_mask(fsn_mark->connector); > >> if (destroy_mark) > >> @@ -1057,6 +1061,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, > >> > >> static int fanotify_group_init_error_pool(struct fsnotify_group *group) > >> { > >> + if (group->fanotify_data.error_event_marks >= FANOTIFY_DEFAULT_FEE_POOL) > >> + return -ENOMEM; > > > > Why not try to mempool_resize()? > > Jan suggested we might not need to bother with it, but I can do that for > the next version. > > > Also, I did not read the rest of the patches yet, but don't we need two > > slots per mark? one for alloc-pre-enqueue and one for free-post-dequeue? > > I don't understand what you mean by two slots for alloc-pre-enqueue and > free-post-dequeue. I suspect it is no longer necessary now that > FAN_FS_ERROR is handled like any other event on enqueue/dequeue, but can > you confirm or clarify? > What I meant was, your code is counting error_event_marks. Every mark accounts for either 1 or 0 queued events, because more errors from the same fs would merge with the single queued event. I thought we could have one more event per fs being copied to user but really we could potentially have many events allocated, before being "merged", so my calculation was wrong. Anyway, as Jan explained, the limited size of mempool does not doom allocations to fail, so we probably have nothing to worry about and there is no need to mempool_resize(). Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index f1cf863d6f9f..5324890500fc 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -959,6 +959,10 @@ static int fanotify_remove_mark(struct fsnotify_group *group, removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags, umask, &destroy_mark); + + if (removed & FAN_FS_ERROR) + group->fanotify_data.error_event_marks--; + if (removed & fsnotify_conn_mask(fsn_mark->connector)) fsnotify_recalc_mask(fsn_mark->connector); if (destroy_mark) @@ -1057,6 +1061,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, static int fanotify_group_init_error_pool(struct fsnotify_group *group) { + if (group->fanotify_data.error_event_marks >= FANOTIFY_DEFAULT_FEE_POOL) + return -ENOMEM; + if (mempool_initialized(&group->fanotify_data.error_events_pool)) return 0; @@ -1098,6 +1105,9 @@ static int fanotify_add_mark(struct fsnotify_group *group, if (added & ~fsnotify_conn_mask(fsn_mark->connector)) fsnotify_recalc_mask(fsn_mark->connector); + if (!(flags & FAN_MARK_IGNORED_MASK) && (mask & FAN_FS_ERROR)) + group->fanotify_data.error_event_marks++; + out: mutex_unlock(&group->mark_mutex); diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index 9941c06b8c8a..96e1d31394ce 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -247,6 +247,7 @@ struct fsnotify_group { int f_flags; /* event_f_flags from fanotify_init() */ struct ucounts *ucounts; mempool_t error_events_pool; + unsigned int error_event_marks; } fanotify_data; #endif /* CONFIG_FANOTIFY */ };
Since FAN_FS_ERROR memory must be pre-allocated, limit a single group from watching too many file systems at once. The current scheme guarantees 1 slot per filesystem, so limit the number of marks with FAN_FS_ERROR per group. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/notify/fanotify/fanotify_user.c | 10 ++++++++++ include/linux/fsnotify_backend.h | 1 + 2 files changed, 11 insertions(+)