| Message ID | 20211014213646.1139469-22-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 events (FAN_FS_ERROR) against the same file system can be merged > by simply iterating the error count. The hash is taken from the fsid, > without considering the FH. This means that only the first error object > is reported. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++--- > fs/notify/fanotify/fanotify.h | 4 +++- > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 9b970359570a..7032083df62a 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1, > return fanotify_info_equal(info1, info2); > } > > +static bool fanotify_error_event_equal(struct fanotify_error_event *fee1, > + struct fanotify_error_event *fee2) > +{ > + /* Error events against the same file system are always merged. */ > + if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid)) > + return false; > + > + return true; > +} > + > static bool fanotify_should_merge(struct fanotify_event *old, > struct fanotify_event *new) > { > @@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old, > case FANOTIFY_EVENT_TYPE_FID_NAME: > return fanotify_name_event_equal(FANOTIFY_NE(old), > FANOTIFY_NE(new)); > + case FANOTIFY_EVENT_TYPE_FS_ERROR: > + return fanotify_error_event_equal(FANOTIFY_EE(old), > + FANOTIFY_EE(new)); > default: > WARN_ON_ONCE(1); > } > @@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old, > return false; > } > > +static void fanotify_merge_error_event(struct fanotify_error_event *dest, > + struct fanotify_error_event *origin) > +{ > + dest->err_count++; > +} > + > +static void fanotify_merge_event(struct fanotify_event *dest, > + struct fanotify_event *origin) > +{ > + dest->mask |= origin->mask; > + > + if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR) > + fanotify_merge_error_event(FANOTIFY_EE(dest), > + FANOTIFY_EE(origin)); > +} > + > /* Limit event merges to limit CPU overhead per event */ > #define FANOTIFY_MAX_MERGE_EVENTS 128 > > @@ -175,7 +204,7 @@ static int fanotify_merge(struct fsnotify_group *group, > if (++i > FANOTIFY_MAX_MERGE_EVENTS) > break; > if (fanotify_should_merge(old, new)) { > - old->mask |= new->mask; > + fanotify_merge_event(old, new); > return 1; > } > } > @@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, > static struct fanotify_event *fanotify_alloc_error_event( > struct fsnotify_group *group, > __kernel_fsid_t *fsid, > - const void *data, int data_type) > + const void *data, int data_type, > + unsigned int *hash) > { > struct fs_error_report *report = > fsnotify_data_error_report(data, data_type); > @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event( > return NULL; > > fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; > + fee->err_count = 1; > + > + *hash ^= fanotify_hash_fsid(fsid); > > return &fee->fae; > } Forgot to store fee->fsid? Thanks, Amir.
On Thu 14-10-21 18:36:39, Gabriel Krisman Bertazi wrote: > Error events (FAN_FS_ERROR) against the same file system can be merged > by simply iterating the error count. The hash is taken from the fsid, > without considering the FH. This means that only the first error object > is reported. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> ... > +static void fanotify_merge_error_event(struct fanotify_error_event *dest, > + struct fanotify_error_event *origin) > +{ > + dest->err_count++; > +} > + > +static void fanotify_merge_event(struct fanotify_event *dest, > + struct fanotify_event *origin) > +{ > + dest->mask |= origin->mask; > + > + if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR) > + fanotify_merge_error_event(FANOTIFY_EE(dest), > + FANOTIFY_EE(origin)); > +} > + > /* Limit event merges to limit CPU overhead per event */ > #define FANOTIFY_MAX_MERGE_EVENTS 128 > > @@ -175,7 +204,7 @@ static int fanotify_merge(struct fsnotify_group *group, > if (++i > FANOTIFY_MAX_MERGE_EVENTS) > break; > if (fanotify_should_merge(old, new)) { > - old->mask |= new->mask; > + fanotify_merge_event(old, new); I guess no need for two functions (fanotify_merge_event(), fanotify_merge_error_event()) for three lines of code? I'd just put those three lines into fanotify_merge(). > @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event( > return NULL; > > fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; > + fee->err_count = 1; > + > + *hash ^= fanotify_hash_fsid(fsid); > > return &fee->fae; > } As Amir mentioned, you should set fsid here. Honza
Amir Goldstein <amir73il@gmail.com> writes: > On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi > <krisman@collabora.com> wrote: >> >> Error events (FAN_FS_ERROR) against the same file system can be merged >> by simply iterating the error count. The hash is taken from the fsid, >> without considering the FH. This means that only the first error object >> is reported. >> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> >> --- >> fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++--- >> fs/notify/fanotify/fanotify.h | 4 +++- >> 2 files changed, 39 insertions(+), 4 deletions(-) >> >> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c >> index 9b970359570a..7032083df62a 100644 >> --- a/fs/notify/fanotify/fanotify.c >> +++ b/fs/notify/fanotify/fanotify.c >> @@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1, >> return fanotify_info_equal(info1, info2); >> } >> >> +static bool fanotify_error_event_equal(struct fanotify_error_event *fee1, >> + struct fanotify_error_event *fee2) >> +{ >> + /* Error events against the same file system are always merged. */ >> + if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid)) >> + return false; >> + >> + return true; >> +} >> + >> static bool fanotify_should_merge(struct fanotify_event *old, >> struct fanotify_event *new) >> { >> @@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old, >> case FANOTIFY_EVENT_TYPE_FID_NAME: >> return fanotify_name_event_equal(FANOTIFY_NE(old), >> FANOTIFY_NE(new)); >> + case FANOTIFY_EVENT_TYPE_FS_ERROR: >> + return fanotify_error_event_equal(FANOTIFY_EE(old), >> + FANOTIFY_EE(new)); >> default: >> WARN_ON_ONCE(1); >> } >> @@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old, >> return false; >> } >> >> +static void fanotify_merge_error_event(struct fanotify_error_event *dest, >> + struct fanotify_error_event *origin) >> +{ >> + dest->err_count++; >> +} >> + >> +static void fanotify_merge_event(struct fanotify_event *dest, >> + struct fanotify_event *origin) >> +{ >> + dest->mask |= origin->mask; >> + >> + if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR) >> + fanotify_merge_error_event(FANOTIFY_EE(dest), >> + FANOTIFY_EE(origin)); >> +} >> + >> /* Limit event merges to limit CPU overhead per event */ >> #define FANOTIFY_MAX_MERGE_EVENTS 128 >> >> @@ -175,7 +204,7 @@ static int fanotify_merge(struct fsnotify_group *group, >> if (++i > FANOTIFY_MAX_MERGE_EVENTS) >> break; >> if (fanotify_should_merge(old, new)) { >> - old->mask |= new->mask; >> + fanotify_merge_event(old, new); >> return 1; >> } >> } >> @@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, >> static struct fanotify_event *fanotify_alloc_error_event( >> struct fsnotify_group *group, >> __kernel_fsid_t *fsid, >> - const void *data, int data_type) >> + const void *data, int data_type, >> + unsigned int *hash) >> { >> struct fs_error_report *report = >> fsnotify_data_error_report(data, data_type); >> @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event( >> return NULL; >> >> fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; >> + fee->err_count = 1; >> + >> + *hash ^= fanotify_hash_fsid(fsid); >> >> return &fee->fae; >> } > > Forgot to store fee->fsid? Not really. this is part of the FID info record support, which is done in patch 23.
On Fri, Oct 15, 2021 at 7:54 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: > >> > >> Error events (FAN_FS_ERROR) against the same file system can be merged > >> by simply iterating the error count. The hash is taken from the fsid, > >> without considering the FH. This means that only the first error object > >> is reported. > >> > >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > >> --- > >> fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++--- > >> fs/notify/fanotify/fanotify.h | 4 +++- > >> 2 files changed, 39 insertions(+), 4 deletions(-) > >> > >> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > >> index 9b970359570a..7032083df62a 100644 > >> --- a/fs/notify/fanotify/fanotify.c > >> +++ b/fs/notify/fanotify/fanotify.c > >> @@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1, > >> return fanotify_info_equal(info1, info2); > >> } > >> > >> +static bool fanotify_error_event_equal(struct fanotify_error_event *fee1, > >> + struct fanotify_error_event *fee2) > >> +{ > >> + /* Error events against the same file system are always merged. */ > >> + if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid)) > >> + return false; > >> + > >> + return true; > >> +} > >> + > >> static bool fanotify_should_merge(struct fanotify_event *old, > >> struct fanotify_event *new) > >> { > >> @@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old, > >> case FANOTIFY_EVENT_TYPE_FID_NAME: > >> return fanotify_name_event_equal(FANOTIFY_NE(old), > >> FANOTIFY_NE(new)); > >> + case FANOTIFY_EVENT_TYPE_FS_ERROR: > >> + return fanotify_error_event_equal(FANOTIFY_EE(old), > >> + FANOTIFY_EE(new)); > >> default: > >> WARN_ON_ONCE(1); > >> } > >> @@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old, > >> return false; > >> } > >> > >> +static void fanotify_merge_error_event(struct fanotify_error_event *dest, > >> + struct fanotify_error_event *origin) > >> +{ > >> + dest->err_count++; > >> +} > >> + > >> +static void fanotify_merge_event(struct fanotify_event *dest, > >> + struct fanotify_event *origin) > >> +{ > >> + dest->mask |= origin->mask; > >> + > >> + if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR) > >> + fanotify_merge_error_event(FANOTIFY_EE(dest), > >> + FANOTIFY_EE(origin)); > >> +} > >> + > >> /* Limit event merges to limit CPU overhead per event */ > >> #define FANOTIFY_MAX_MERGE_EVENTS 128 > >> > >> @@ -175,7 +204,7 @@ static int fanotify_merge(struct fsnotify_group *group, > >> if (++i > FANOTIFY_MAX_MERGE_EVENTS) > >> break; > >> if (fanotify_should_merge(old, new)) { > >> - old->mask |= new->mask; > >> + fanotify_merge_event(old, new); > >> return 1; > >> } > >> } > >> @@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, > >> static struct fanotify_event *fanotify_alloc_error_event( > >> struct fsnotify_group *group, > >> __kernel_fsid_t *fsid, > >> - const void *data, int data_type) > >> + const void *data, int data_type, > >> + unsigned int *hash) > >> { > >> struct fs_error_report *report = > >> fsnotify_data_error_report(data, data_type); > >> @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event( > >> return NULL; > >> > >> fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; > >> + fee->err_count = 1; > >> + > >> + *hash ^= fanotify_hash_fsid(fsid); > >> > >> return &fee->fae; > >> } > > > > Forgot to store fee->fsid? > > Not really. this is part of the FID info record support, which is done > in patch 23. > Sure, it does not really matter for bisection when FS_ERROR is not yet wired, but it is weird to compare event fsid's when they have not been initialized. Logically, storing the fsid in this patch would be better. Thanks, Amir.
Amir Goldstein <amir73il@gmail.com> writes: > On Fri, Oct 15, 2021 at 7:54 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: >> >> >> >> Error events (FAN_FS_ERROR) against the same file system can be merged >> >> by simply iterating the error count. The hash is taken from the fsid, >> >> without considering the FH. This means that only the first error object >> >> is reported. >> >> >> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> >> >> --- >> >> fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++--- >> >> fs/notify/fanotify/fanotify.h | 4 +++- >> >> 2 files changed, 39 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c >> >> index 9b970359570a..7032083df62a 100644 >> >> --- a/fs/notify/fanotify/fanotify.c >> >> +++ b/fs/notify/fanotify/fanotify.c >> >> @@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1, >> >> return fanotify_info_equal(info1, info2); >> >> } >> >> >> >> +static bool fanotify_error_event_equal(struct fanotify_error_event *fee1, >> >> + struct fanotify_error_event *fee2) >> >> +{ >> >> + /* Error events against the same file system are always merged. */ >> >> + if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid)) >> >> + return false; >> >> + >> >> + return true; >> >> +} >> >> + >> >> static bool fanotify_should_merge(struct fanotify_event *old, >> >> struct fanotify_event *new) >> >> { >> >> @@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old, >> >> case FANOTIFY_EVENT_TYPE_FID_NAME: >> >> return fanotify_name_event_equal(FANOTIFY_NE(old), >> >> FANOTIFY_NE(new)); >> >> + case FANOTIFY_EVENT_TYPE_FS_ERROR: >> >> + return fanotify_error_event_equal(FANOTIFY_EE(old), >> >> + FANOTIFY_EE(new)); >> >> default: >> >> WARN_ON_ONCE(1); >> >> } >> >> @@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old, >> >> return false; >> >> } >> >> >> >> +static void fanotify_merge_error_event(struct fanotify_error_event *dest, >> >> + struct fanotify_error_event *origin) >> >> +{ >> >> + dest->err_count++; >> >> +} >> >> + >> >> +static void fanotify_merge_event(struct fanotify_event *dest, >> >> + struct fanotify_event *origin) >> >> +{ >> >> + dest->mask |= origin->mask; >> >> + >> >> + if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR) >> >> + fanotify_merge_error_event(FANOTIFY_EE(dest), >> >> + FANOTIFY_EE(origin)); >> >> +} >> >> + >> >> /* Limit event merges to limit CPU overhead per event */ >> >> #define FANOTIFY_MAX_MERGE_EVENTS 128 >> >> >> >> @@ -175,7 +204,7 @@ static int fanotify_merge(struct fsnotify_group *group, >> >> if (++i > FANOTIFY_MAX_MERGE_EVENTS) >> >> break; >> >> if (fanotify_should_merge(old, new)) { >> >> - old->mask |= new->mask; >> >> + fanotify_merge_event(old, new); >> >> return 1; >> >> } >> >> } >> >> @@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, >> >> static struct fanotify_event *fanotify_alloc_error_event( >> >> struct fsnotify_group *group, >> >> __kernel_fsid_t *fsid, >> >> - const void *data, int data_type) >> >> + const void *data, int data_type, >> >> + unsigned int *hash) >> >> { >> >> struct fs_error_report *report = >> >> fsnotify_data_error_report(data, data_type); >> >> @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event( >> >> return NULL; >> >> >> >> fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; >> >> + fee->err_count = 1; >> >> + >> >> + *hash ^= fanotify_hash_fsid(fsid); >> >> >> >> return &fee->fae; >> >> } >> > >> > Forgot to store fee->fsid? >> >> Not really. this is part of the FID info record support, which is done >> in patch 23. >> > > Sure, it does not really matter for bisection when FS_ERROR is not yet > wired, but it is weird to compare event fsid's when they have not been > initialized. > > Logically, storing the fsid in this patch would be better. Makes sense. Will do! Thanks,
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 9b970359570a..7032083df62a 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1, return fanotify_info_equal(info1, info2); } +static bool fanotify_error_event_equal(struct fanotify_error_event *fee1, + struct fanotify_error_event *fee2) +{ + /* Error events against the same file system are always merged. */ + if (!fanotify_fsid_equal(&fee1->fsid, &fee2->fsid)) + return false; + + return true; +} + static bool fanotify_should_merge(struct fanotify_event *old, struct fanotify_event *new) { @@ -141,6 +151,9 @@ static bool fanotify_should_merge(struct fanotify_event *old, case FANOTIFY_EVENT_TYPE_FID_NAME: return fanotify_name_event_equal(FANOTIFY_NE(old), FANOTIFY_NE(new)); + case FANOTIFY_EVENT_TYPE_FS_ERROR: + return fanotify_error_event_equal(FANOTIFY_EE(old), + FANOTIFY_EE(new)); default: WARN_ON_ONCE(1); } @@ -148,6 +161,22 @@ static bool fanotify_should_merge(struct fanotify_event *old, return false; } +static void fanotify_merge_error_event(struct fanotify_error_event *dest, + struct fanotify_error_event *origin) +{ + dest->err_count++; +} + +static void fanotify_merge_event(struct fanotify_event *dest, + struct fanotify_event *origin) +{ + dest->mask |= origin->mask; + + if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR) + fanotify_merge_error_event(FANOTIFY_EE(dest), + FANOTIFY_EE(origin)); +} + /* Limit event merges to limit CPU overhead per event */ #define FANOTIFY_MAX_MERGE_EVENTS 128 @@ -175,7 +204,7 @@ static int fanotify_merge(struct fsnotify_group *group, if (++i > FANOTIFY_MAX_MERGE_EVENTS) break; if (fanotify_should_merge(old, new)) { - old->mask |= new->mask; + fanotify_merge_event(old, new); return 1; } } @@ -577,7 +606,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, static struct fanotify_event *fanotify_alloc_error_event( struct fsnotify_group *group, __kernel_fsid_t *fsid, - const void *data, int data_type) + const void *data, int data_type, + unsigned int *hash) { struct fs_error_report *report = fsnotify_data_error_report(data, data_type); @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event( return NULL; fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; + fee->err_count = 1; + + *hash ^= fanotify_hash_fsid(fsid); return &fee->fae; } @@ -664,7 +697,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group, event = fanotify_alloc_perm_event(path, gfp); } else if (fanotify_is_error_event(mask)) { event = fanotify_alloc_error_event(group, fsid, data, - data_type); + data_type, &hash); } else if (name_event && (file_name || child)) { event = fanotify_alloc_name_event(id, fsid, file_name, child, &hash, gfp); diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index ebef952481fa..2b032b79d5b0 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -199,6 +199,9 @@ FANOTIFY_NE(struct fanotify_event *event) struct fanotify_error_event { struct fanotify_event fae; + u32 err_count; /* Suppressed errors count */ + + __kernel_fsid_t fsid; /* FSID this error refers to. */ }; static inline struct fanotify_error_event * @@ -332,7 +335,6 @@ static inline struct path *fanotify_event_path(struct fanotify_event *event) static inline bool fanotify_is_hashed_event(u32 mask) { return !(fanotify_is_perm_event(mask) || - fanotify_is_error_event(mask) || fsnotify_is_overflow_event(mask)); }
Error events (FAN_FS_ERROR) against the same file system can be merged by simply iterating the error count. The hash is taken from the fsid, without considering the FH. This means that only the first error object is reported. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++++++++++++++--- fs/notify/fanotify/fanotify.h | 4 +++- 2 files changed, 39 insertions(+), 4 deletions(-)