| Message ID | 20211019000015.1666608-21-krisman@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | file system-wide error monitoring | expand |
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > > Allow the FAN_FS_ERROR group mempool to grow up to an upper limit > dynamically, instead of starting already at the limit. This doesn't > bother resizing on mark removal, but next time a mark is added, the slot > will be either reused or resized. Also, if several marks are being > removed at once, most likely the group is going away anyway. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- > include/linux/fsnotify_backend.h | 1 + > 2 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index f77581c5b97f..a860c286e885 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,12 +1061,24 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, > > static int fanotify_group_init_error_pool(struct fsnotify_group *group) > { > - if (mempool_initialized(&group->fanotify_data.error_events_pool)) > - return 0; > + int ret; > + > + if (group->fanotify_data.error_event_marks >= > + FANOTIFY_DEFAULT_MAX_FEE_POOL) > + return -ENOMEM; > > - return mempool_init_kmalloc_pool(&group->fanotify_data.error_events_pool, > - FANOTIFY_DEFAULT_MAX_FEE_POOL, > - sizeof(struct fanotify_error_event)); > + if (!mempool_initialized(&group->fanotify_data.error_events_pool)) > + ret = mempool_init_kmalloc_pool( > + &group->fanotify_data.error_events_pool, > + 1, sizeof(struct fanotify_error_event)); > + else > + ret = mempool_resize(&group->fanotify_data.error_events_pool, > + group->fanotify_data.error_event_marks + 1); > + > + if (!ret) > + group->fanotify_data.error_event_marks++; > + > + return ret; > } This is not what I had in mind. I was thinking start with ~32 and double each time limit is reached. And also, this code grows the pool to infinity with add/remove mark loop. Anyway, since I clearly did not understand how mempool works and Jan had some different ideas I would leave it to Jan to explain how he wants the mempool init limit and resize to be implemented. Thanks, Amir.
On Tue 19-10-21 08:50:23, Amir Goldstein wrote: > On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi > <krisman@collabora.com> wrote: > > > > Allow the FAN_FS_ERROR group mempool to grow up to an upper limit > > dynamically, instead of starting already at the limit. This doesn't > > bother resizing on mark removal, but next time a mark is added, the slot > > will be either reused or resized. Also, if several marks are being > > removed at once, most likely the group is going away anyway. > > > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > > --- > > fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- > > include/linux/fsnotify_backend.h | 1 + > > 2 files changed, 22 insertions(+), 5 deletions(-) > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index f77581c5b97f..a860c286e885 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,12 +1061,24 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, > > > > static int fanotify_group_init_error_pool(struct fsnotify_group *group) > > { > > - if (mempool_initialized(&group->fanotify_data.error_events_pool)) > > - return 0; > > + int ret; > > + > > + if (group->fanotify_data.error_event_marks >= > > + FANOTIFY_DEFAULT_MAX_FEE_POOL) > > + return -ENOMEM; > > > > - return mempool_init_kmalloc_pool(&group->fanotify_data.error_events_pool, > > - FANOTIFY_DEFAULT_MAX_FEE_POOL, > > - sizeof(struct fanotify_error_event)); > > + if (!mempool_initialized(&group->fanotify_data.error_events_pool)) > > + ret = mempool_init_kmalloc_pool( > > + &group->fanotify_data.error_events_pool, > > + 1, sizeof(struct fanotify_error_event)); > > + else > > + ret = mempool_resize(&group->fanotify_data.error_events_pool, > > + group->fanotify_data.error_event_marks + 1); > > + > > + if (!ret) > > + group->fanotify_data.error_event_marks++; > > + > > + return ret; > > } > > This is not what I had in mind. > I was thinking start with ~32 and double each time limit is reached. Do you mean when number of FS_ERROR marks reaches the number of preallocated events? We could do that but note that due to mempool implementation limits there cannot be more than 255 preallocated events, also mempool_resize() will only update number of slots for preallocated events but these slots will be empty. You have to manually allocate and free events to fill these slots with preallocated events. > And also, this code grows the pool to infinity with add/remove mark loop. I see a cap at FANOTIFY_DEFAULT_MAX_FEE_POOL in the code there. But I don't think there's a good enough reason to hard-limit number of FS_ERROR marks at 128. As I explained in the previous version of the series, in vast majority of cases we will not use even a single preallocated event... > Anyway, since I clearly did not understand how mempool works and > Jan had some different ideas I would leave it to Jan to explain > how he wants the mempool init limit and resize to be implemented. Honestly, I'm for keeping it simple for now. Just 32 preallocated events and try to come up with something more clever only if someone actually complains. Honza
Jan Kara <jack@suse.cz> writes: > On Tue 19-10-21 08:50:23, Amir Goldstein wrote: >> On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi >> <krisman@collabora.com> wrote: >> > >> > Allow the FAN_FS_ERROR group mempool to grow up to an upper limit >> > dynamically, instead of starting already at the limit. This doesn't >> > bother resizing on mark removal, but next time a mark is added, the slot >> > will be either reused or resized. Also, if several marks are being >> > removed at once, most likely the group is going away anyway. >> > >> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> >> > --- >> > fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- >> > include/linux/fsnotify_backend.h | 1 + >> > 2 files changed, 22 insertions(+), 5 deletions(-) >> > >> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c >> > index f77581c5b97f..a860c286e885 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,12 +1061,24 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, >> > >> > static int fanotify_group_init_error_pool(struct fsnotify_group *group) >> > { >> > - if (mempool_initialized(&group->fanotify_data.error_events_pool)) >> > - return 0; >> > + int ret; >> > + >> > + if (group->fanotify_data.error_event_marks >= >> > + FANOTIFY_DEFAULT_MAX_FEE_POOL) >> > + return -ENOMEM; >> > >> > - return mempool_init_kmalloc_pool(&group->fanotify_data.error_events_pool, >> > - FANOTIFY_DEFAULT_MAX_FEE_POOL, >> > - sizeof(struct fanotify_error_event)); >> > + if (!mempool_initialized(&group->fanotify_data.error_events_pool)) >> > + ret = mempool_init_kmalloc_pool( >> > + &group->fanotify_data.error_events_pool, >> > + 1, sizeof(struct fanotify_error_event)); >> > + else >> > + ret = mempool_resize(&group->fanotify_data.error_events_pool, >> > + group->fanotify_data.error_event_marks + 1); >> > + >> > + if (!ret) >> > + group->fanotify_data.error_event_marks++; >> > + >> > + return ret; >> > } >> >> This is not what I had in mind. >> I was thinking start with ~32 and double each time limit is reached. > > Do you mean when number of FS_ERROR marks reaches the number of preallocated > events? We could do that but note that due to mempool implementation limits > there cannot be more than 255 preallocated events, also mempool_resize() > will only update number of slots for preallocated events but these slots > will be empty. You have to manually allocate and free events to fill these > slots with preallocated events. > >> And also, this code grows the pool to infinity with add/remove mark loop. > > I see a cap at FANOTIFY_DEFAULT_MAX_FEE_POOL in the code there. But I don't > think there's a good enough reason to hard-limit number of FS_ERROR marks > at 128. As I explained in the previous version of the series, in vast > majority of cases we will not use even a single preallocated event... > >> Anyway, since I clearly did not understand how mempool works and >> Jan had some different ideas I would leave it to Jan to explain >> how he wants the mempool init limit and resize to be implemented. > > Honestly, I'm for keeping it simple for now. Just 32 preallocated events > and try to come up with something more clever only if someone actually > complains. So, If I understand correctly the conclusion, you are fine if I revert to the version I had in v7: 32 fields pre-allocated, no dynamic growth and just limit the number of FAN_FS_ERROR marks to <= 32? In the future, if this ever becomes a problem, we look into dynamic resizing/increasing the limit? I think either option is fine by me. I thought that growing 1 by 1 like I did here would be ugly, but before sending the patch, I checked and I was quite satisfied with how simple mempool_resize actually is.
On Thu 21-10-21 15:17:33, Gabriel Krisman Bertazi wrote: > Jan Kara <jack@suse.cz> writes: > > > On Tue 19-10-21 08:50:23, Amir Goldstein wrote: > >> On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi > >> <krisman@collabora.com> wrote: > >> > > >> > Allow the FAN_FS_ERROR group mempool to grow up to an upper limit > >> > dynamically, instead of starting already at the limit. This doesn't > >> > bother resizing on mark removal, but next time a mark is added, the slot > >> > will be either reused or resized. Also, if several marks are being > >> > removed at once, most likely the group is going away anyway. > >> > > >> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > >> > --- > >> > fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- > >> > include/linux/fsnotify_backend.h | 1 + > >> > 2 files changed, 22 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > >> > index f77581c5b97f..a860c286e885 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,12 +1061,24 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, > >> > > >> > static int fanotify_group_init_error_pool(struct fsnotify_group *group) > >> > { > >> > - if (mempool_initialized(&group->fanotify_data.error_events_pool)) > >> > - return 0; > >> > + int ret; > >> > + > >> > + if (group->fanotify_data.error_event_marks >= > >> > + FANOTIFY_DEFAULT_MAX_FEE_POOL) > >> > + return -ENOMEM; > >> > > >> > - return mempool_init_kmalloc_pool(&group->fanotify_data.error_events_pool, > >> > - FANOTIFY_DEFAULT_MAX_FEE_POOL, > >> > - sizeof(struct fanotify_error_event)); > >> > + if (!mempool_initialized(&group->fanotify_data.error_events_pool)) > >> > + ret = mempool_init_kmalloc_pool( > >> > + &group->fanotify_data.error_events_pool, > >> > + 1, sizeof(struct fanotify_error_event)); > >> > + else > >> > + ret = mempool_resize(&group->fanotify_data.error_events_pool, > >> > + group->fanotify_data.error_event_marks + 1); > >> > + > >> > + if (!ret) > >> > + group->fanotify_data.error_event_marks++; > >> > + > >> > + return ret; > >> > } > >> > >> This is not what I had in mind. > >> I was thinking start with ~32 and double each time limit is reached. > > > > Do you mean when number of FS_ERROR marks reaches the number of preallocated > > events? We could do that but note that due to mempool implementation limits > > there cannot be more than 255 preallocated events, also mempool_resize() > > will only update number of slots for preallocated events but these slots > > will be empty. You have to manually allocate and free events to fill these > > slots with preallocated events. > > > >> And also, this code grows the pool to infinity with add/remove mark loop. > > > > I see a cap at FANOTIFY_DEFAULT_MAX_FEE_POOL in the code there. But I don't > > think there's a good enough reason to hard-limit number of FS_ERROR marks > > at 128. As I explained in the previous version of the series, in vast > > majority of cases we will not use even a single preallocated event... > > > >> Anyway, since I clearly did not understand how mempool works and > >> Jan had some different ideas I would leave it to Jan to explain > >> how he wants the mempool init limit and resize to be implemented. > > > > Honestly, I'm for keeping it simple for now. Just 32 preallocated events > > and try to come up with something more clever only if someone actually > > complains. > > So, If I understand correctly the conclusion, you are fine if I revert to > the version I had in v7: 32 fields pre-allocated, no dynamic growth and > just limit the number of FAN_FS_ERROR marks to <= 32? Yes to 32 preallocated events, no to FAN_FS_ERROR mark limit - just keep number of marks unlimited. IMO it would be a hard to understand limit for userspace. > In the future, if this ever becomes a problem, we look into dynamic > resizing/increasing the limit? Yes. > I think either option is fine by me. I thought that growing 1 by 1 like > I did here would be ugly, but before sending the patch, I checked and I > was quite satisfied with how simple mempool_resize actually is. Yes, mempool resize is simple, except that you have to care not to resize to more than 255 and also for mempool_resize() to guarantee anything you have to allocate and free events to fill slots at which point things become a bit ugly. Honza
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index f77581c5b97f..a860c286e885 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,12 +1061,24 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, static int fanotify_group_init_error_pool(struct fsnotify_group *group) { - if (mempool_initialized(&group->fanotify_data.error_events_pool)) - return 0; + int ret; + + if (group->fanotify_data.error_event_marks >= + FANOTIFY_DEFAULT_MAX_FEE_POOL) + return -ENOMEM; - return mempool_init_kmalloc_pool(&group->fanotify_data.error_events_pool, - FANOTIFY_DEFAULT_MAX_FEE_POOL, - sizeof(struct fanotify_error_event)); + if (!mempool_initialized(&group->fanotify_data.error_events_pool)) + ret = mempool_init_kmalloc_pool( + &group->fanotify_data.error_events_pool, + 1, sizeof(struct fanotify_error_event)); + else + ret = mempool_resize(&group->fanotify_data.error_events_pool, + group->fanotify_data.error_event_marks + 1); + + if (!ret) + group->fanotify_data.error_event_marks++; + + return ret; } static int fanotify_add_mark(struct fsnotify_group *group, 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 */ };
Allow the FAN_FS_ERROR group mempool to grow up to an upper limit dynamically, instead of starting already at the limit. This doesn't bother resizing on mark removal, but next time a mark is added, the slot will be either reused or resized. Also, if several marks are being removed at once, most likely the group is going away anyway. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- include/linux/fsnotify_backend.h | 1 + 2 files changed, 22 insertions(+), 5 deletions(-)