file system-wide error monitoring
Subsystems (8)
v8 cover file system-wide error monitoring
Hi,
This is the 8th version of this patch series. We are getting close! Thank you Amir and Jan for your repeated assistance in getting this in shape. This version applies all your feedback from previous version, in particular, it has a resizeable mempool, such that we don't waste to much space if not needed.
This was tested with LTP for regressions and also using the sample code on the last patch, with a corrupted image. I wrote a new ltp test for this feature which is being reviewed and is available at:
https://gitlab.collabora.com/krisman/ltp -b fan-fs-error
In addition, I wrote a man-page that can be pulled from:
https://gitlab.collabora.com/krisman/man-pages.git -b fan-fs-error
And is being reviewed at the list.
I also pushed this full series to:
https://gitlab.collabora.com/krisman/linux -b fanotify-notifications-v8
Thank you
Cc: Darrick J. Wong <[email protected]>
Cc: Theodore Ts'o <[email protected]>
Cc: Dave Chinner <[email protected]>
Cc: [email protected]
To: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Amir Goldstein (3):
fsnotify: pass data_type to fsnotify_name()
fsnotify: pass dentry instead of inode data
fsnotify: clarify contract for create event hooks
Gabriel Krisman Bertazi (29):
fsnotify: Don't insert unmergeable events in hashtable
fanotify: Fold event size calculation to its own function
fanotify: Split fsid check from other fid mode checks
inotify: Don't force FS_IN_IGNORED
fsnotify: Add helper to detect overflow_event
fsnotify: Add wrapper around fsnotify_add_event
fsnotify: Retrieve super block from the data field
fsnotify: Protect fsnotify_handle_inode_event from no-inode events
fsnotify: Pass group argument to free_event
fanotify: Support null inode event in fanotify_dfid_inode
fanotify: Allow file handle encoding for unhashed events
fanotify: Encode empty file handle when no inode is provided
fanotify: Require fid_mode for any non-fd event
fsnotify: Support FS_ERROR event type
fanotify: Reserve UAPI bits for FAN_FS_ERROR
fanotify: Pre-allocate pool of error events
fanotify: Dynamically resize the FAN_FS_ERROR pool
fanotify: Support enqueueing of error events
fanotify: Support merging of error events
fanotify: Wrap object_fh inline space in a creator macro
fanotify: Add helpers to decide whether to report FID/DFID
fanotify: Report fid entry even for zero-length file_handle
fanotify: WARN_ON against too large file handles
fanotify: Report fid info for file related file system errors
fanotify: Emit generic error info for error event
fanotify: Allow users to request FAN_FS_ERROR events
ext4: Send notifications on error
samples: Add fs error monitoring example
docs: Document the FAN_FS_ERROR event
.../admin-guide/filesystem-monitoring.rst | 76 ++++++++ Documentation/admin-guide/index.rst | 1 + fs/ext4/super.c | 8 + fs/notify/fanotify/fanotify.c | 116 +++++++++++- fs/notify/fanotify/fanotify.h | 60 +++++- fs/notify/fanotify/fanotify_user.c | 172 ++++++++++++++---- fs/notify/fsnotify.c | 10 +- fs/notify/group.c | 2 +- fs/notify/inotify/inotify_fsnotify.c | 5 +- fs/notify/inotify/inotify_user.c | 6 +- fs/notify/notification.c | 14 +- include/linux/fanotify.h | 9 +- include/linux/fsnotify.h | 58 ++++-- include/linux/fsnotify_backend.h | 96 +++++++++- include/uapi/linux/fanotify.h | 8 + kernel/audit_fsnotify.c | 3 +- kernel/audit_watch.c | 3 +- samples/Kconfig | 9 + samples/Makefile | 1 + samples/fanotify/Makefile | 5 + samples/fanotify/fs-monitor.c | 142 +++++++++++++++ 21 files changed, 705 insertions(+), 99 deletions(-) create mode 100644 Documentation/admin-guide/filesystem-monitoring.rst create mode 100644 samples/fanotify/Makefile create mode 100644 samples/fanotify/fs-monitor.c
Signature · 1 line
2.33.0
PATCH v8 1/32 fsnotify: pass data_type to fsnotify_name()
From: Amir Goldstein <[email protected]>
Align the arguments of fsnotify_name() to those of fsnotify().
--- include/linux/fsnotify.h | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
include/linux/fsnotify.h +13 -9
@@ -26,20 +26,21 @@* FS_EVENT_ON_CHILD mask on the parent inode and will not be reported if only * the child is interested and not the parent. */ -static inline void fsnotify_name (struct inode *dir,__u32 mask, -struct inode *child, -const struct qstr *name,u32 cookie) +static inline int fsnotify_name (__u32 mask,const void *data,int data_type, +struct inode *dir,const struct qstr *name, +u32 cookie) {if (atomic_long_read (&dir->i_sb->s_fsnotify_connectors) ==0 ) -return ; +return 0 ; -fsnotify (mask, child,FSNOTIFY_EVENT_INODE , dir, name,NULL , cookie); +return fsnotify (mask, data, data_type, dir, name,NULL , cookie); }static inline void fsnotify_dirent (struct inode *dir,struct dentry *dentry,__u32 mask) { -fsnotify_name (dir, mask,d_inode (dentry), &dentry->d_name,0 ); +fsnotify_name (mask,d_inode (dentry),FSNOTIFY_EVENT_INODE , + dir, &dentry->d_name,0 ); }static inline void fsnotify_inode (struct inode *inode,__u32 mask)
@@ -154,8 +155,10 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir, new_dir_mask |=FS_ISDIR ; } -fsnotify_name (old_dir, old_dir_mask, source, old_name, fs_cookie); -fsnotify_name (new_dir, new_dir_mask, source, new_name, fs_cookie); +fsnotify_name (old_dir_mask, source,FSNOTIFY_EVENT_INODE , + old_dir, old_name, fs_cookie); +fsnotify_name (new_dir_mask, source,FSNOTIFY_EVENT_INODE , + new_dir, new_name, fs_cookie);if (target)fsnotify_link_count (target);
@@ -209,7 +212,8 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode,fsnotify_link_count (inode);audit_inode_child (dir, new_dentry,AUDIT_TYPE_CHILD_CREATE ); -fsnotify_name (dir,FS_CREATE , inode, &new_dentry->d_name,0 ); +fsnotify_name (FS_CREATE , inode,FSNOTIFY_EVENT_INODE , + dir, &new_dentry->d_name,0 ); }/*
Signature · 1 line
2.33.0
PATCH v8 2/32 fsnotify: pass dentry instead of inode data
In reply to Gabriel Krisman BertaziFrom: Amir Goldstein <[email protected]>
Define a new data type to pass for event - FSNOTIFY_EVENT_DENTRY. Use it to pass the dentry instead of it's ->d_inode where available.
This is needed in preparation to the refactor to retrieve the super block from the data field. In some cases (i.e. mkdir in kernfs), the data inode comes from a negative dentry, such that no super block information would be available. By receiving the dentry itself, instead of the inode, fsnotify can derive the super block even on these cases.
---
Changes since v7:
- Improve commit message (Jan)
--- include/linux/fsnotify.h | 5 ++--- include/linux/fsnotify_backend.h | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-)
include/linux/fsnotify.h +2 -3
@@ -39,8 +39,7 @@ static inline int fsnotify_name(__u32 mask, const void *data, int data_type,static inline void fsnotify_dirent (struct inode *dir,struct dentry *dentry,__u32 mask) { -fsnotify_name (mask,d_inode (dentry),FSNOTIFY_EVENT_INODE , - dir, &dentry->d_name,0 ); +fsnotify_name (mask, dentry,FSNOTIFY_EVENT_DENTRY , dir, &dentry->d_name,0 ); }static inline void fsnotify_inode (struct inode *inode,__u32 mask)
@@ -87,7 +86,7 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,*/ static inline void fsnotify_dentry (struct dentry *dentry,__u32 mask) { -fsnotify_parent (dentry, mask,d_inode (dentry),FSNOTIFY_EVENT_INODE ); +fsnotify_parent (dentry, mask, dentry,FSNOTIFY_EVENT_DENTRY ); }static inline int fsnotify_file (struct file *file,__u32 mask)
include/linux/fsnotify_backend.h +16 -0
@@ -248,6 +248,7 @@ enum fsnotify_data_type {FSNOTIFY_EVENT_NONE ,FSNOTIFY_EVENT_PATH ,FSNOTIFY_EVENT_INODE , + FSNOTIFY_EVENT_DENTRY, };static inline struct inode *fsnotify_data_inode (const void *data,int data_type)
@@ -255,6 +256,8 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)switch (data_type) {case FSNOTIFY_EVENT_INODE :return (struct inode *)data; +case FSNOTIFY_EVENT_DENTRY : +return d_inode (data);case FSNOTIFY_EVENT_PATH :return d_inode (((const struct path *)data)->dentry);default :
@@ -262,6 +265,19 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type) } } +static inline struct dentry *fsnotify_data_dentry (const void *data,int data_type) +{ +switch (data_type) { +case FSNOTIFY_EVENT_DENTRY : +/* Non const is needed for dget() */ +return (struct dentry *)data; +case FSNOTIFY_EVENT_PATH : +return ((const struct path *)data)->dentry; +default : +return NULL ; + } +} +static inline const struct path *fsnotify_data_path (const void *data,int data_type) {
Signature · 1 line
2.33.0
PATCH v8 3/32 fsnotify: clarify contract for create event hooks
In reply to Gabriel Krisman BertaziFrom: Amir Goldstein <[email protected]>
Clarify argument names and contract for fsnotify_create() and fsnotify_mkdir() to reflect the anomaly of kernfs, which leaves dentries negavite after mkdir/create.
Remove the WARN_ON(!inode) in audit code that were added by the Fixes commit under the wrong assumption that dentries cannot be negative after mkdir/create.
--- include/linux/fsnotify.h | 22 ++++++++++++++++------ kernel/audit_fsnotify.c | 3 +-- kernel/audit_watch.c | 3 +-- 3 files changed, 18 insertions(+), 10 deletions(-)
include/linux/fsnotify.h +16 -6
@@ -192,16 +192,22 @@ static inline void fsnotify_inoderemove(struct inode *inode)/* * fsnotify_create - 'name' was linked in +* +* Caller must make sure that dentry->d_name is stable. +* Note: some filesystems (e.g. kernfs) leave @dentry negative and instantiate +* ->d_inode later */ -static inline void fsnotify_create (struct inode *inode,struct dentry *dentry) +static inline void fsnotify_create (struct inode *dir,struct dentry *dentry) { -audit_inode_child (inode, dentry,AUDIT_TYPE_CHILD_CREATE ); +audit_inode_child (dir, dentry,AUDIT_TYPE_CHILD_CREATE ); -fsnotify_dirent (inode, dentry,FS_CREATE ); +fsnotify_dirent (dir, dentry,FS_CREATE ); }/* * fsnotify_link - new hardlink in 'inode' directory +* +* Caller must make sure that new_dentry->d_name is stable. * Note: We have to pass also the linked inode ptr as some filesystems leave * new_dentry->d_inode NULL and instantiate inode pointer later */
@@ -230,12 +236,16 @@ static inline void fsnotify_unlink(struct inode *dir, struct dentry *dentry)/* * fsnotify_mkdir - directory 'name' was created +* +* Caller must make sure that dentry->d_name is stable. +* Note: some filesystems (e.g. kernfs) leave @dentry negative and instantiate +* ->d_inode later */ -static inline void fsnotify_mkdir (struct inode *inode,struct dentry *dentry) +static inline void fsnotify_mkdir (struct inode *dir,struct dentry *dentry) { -audit_inode_child (inode, dentry,AUDIT_TYPE_CHILD_CREATE ); +audit_inode_child (dir, dentry,AUDIT_TYPE_CHILD_CREATE ); -fsnotify_dirent (inode, dentry,FS_CREATE |FS_ISDIR ); +fsnotify_dirent (dir, dentry,FS_CREATE |FS_ISDIR ); }/*
kernel/audit_fsnotify.c +1 -2
@@ -160,8 +160,7 @@ static int audit_mark_handle_event(struct fsnotify_mark *inode_mark, u32 mask, audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark); -if (WARN_ON_ONCE (inode_mark->group != audit_fsnotify_group) || -WARN_ON_ONCE (!inode)) +if (WARN_ON_ONCE (inode_mark->group != audit_fsnotify_group))return 0 ;if (mask & (FS_CREATE |FS_MOVED_TO |FS_DELETE |FS_MOVED_FROM )) {
kernel/audit_watch.c +1 -2
@@ -472,8 +472,7 @@ static int audit_watch_handle_event(struct fsnotify_mark *inode_mark, u32 mask, parent = container_of(inode_mark, struct audit_parent, mark); -if (WARN_ON_ONCE (inode_mark->group != audit_watch_group) || -WARN_ON_ONCE (!inode)) +if (WARN_ON_ONCE (inode_mark->group != audit_watch_group))return 0 ;if (mask & (FS_CREATE |FS_MOVED_TO ) && inode)
Signature · 1 line
2.33.0
PATCH v8 4/32 fsnotify: Don't insert unmergeable events in hashtable
In reply to Gabriel Krisman BertaziSome events, like the overflow event, are not mergeable, so they are not hashed. But, when failing inside fsnotify_add_event for lack of space, fsnotify_add_event() still calls the insert hook, which adds the overflow event to the merge list. Add a check to prevent any kind of unmergeable event to be inserted in the hashtable.
---
Changes since v2:
- Do check for hashed events inside the insert hook (Amir)
--- fs/notify/fanotify/fanotify.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
fs/notify/fanotify/fanotify.c +4 -2
@@ -702,6 +702,9 @@ static void fanotify_insert_event(struct fsnotify_group *group,assert_spin_locked (&group->notification_lock); +if (!fanotify_is_hashed_event (event->mask)) +return ; +pr_debug ("%s: group=%p event=%p bucket=%u\n" , __func__, group, event, bucket);
@@ -779,8 +782,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask, fsn_event = &event->fse; ret =fsnotify_add_event (group, fsn_event, fanotify_merge, -fanotify_is_hashed_event (mask) ? - fanotify_insert_event :NULL ); + fanotify_insert_event);if (ret) {/* Permission events shouldn't be merged */ BUG_ON (ret ==1 && mask &FANOTIFY_PERM_EVENTS );
Signature · 1 line
2.33.0
PATCH v8 5/32 fanotify: Fold event size calculation to its own function
In reply to Gabriel Krisman BertaziEvery time this function is invoked, it is immediately added to FAN_EVENT_METADATA_LEN, since there is no need to just calculate the length of info records. This minor clean up folds the rest of the calculation into the function, which now operates in terms of events, returning the size of the entire event, including metadata.
---
Changes since v6:
- Rebase on top of pidfd patches
Changes since v1:
- rebased on top of hashing patches
--- fs/notify/fanotify/fanotify_user.c | 35 +++++++++++++++++------------- 1 file changed, 20 insertions(+), 15 deletions(-)
fs/notify/fanotify/fanotify_user.c +20 -15
@@ -126,17 +126,24 @@ static int fanotify_fid_info_len(int fh_len, int name_len) FANOTIFY_EVENT_ALIGN); } -static int fanotify_event_info_len (unsigned int info_mode, -struct fanotify_event *event) +static size_t fanotify_event_len (unsigned int info_mode, +struct fanotify_event *event) { -struct fanotify_info *info =fanotify_event_info (event); -int dir_fh_len =fanotify_event_dir_fh_len (event); -int fh_len =fanotify_event_object_fh_len (event); -int info_len =0 ; +size_t event_len =FAN_EVENT_METADATA_LEN ; +struct fanotify_info *info; +int dir_fh_len; +int fh_len;int dot_len =0 ; +if (!info_mode) +return event_len; + + info =fanotify_event_info (event); + dir_fh_len =fanotify_event_dir_fh_len (event); + fh_len =fanotify_event_object_fh_len (event); +if (dir_fh_len) { - info_len +=fanotify_fid_info_len (dir_fh_len, info->name_len); + event_len +=fanotify_fid_info_len (dir_fh_len, info->name_len); }else if ((info_mode &FAN_REPORT_NAME ) && (event->mask &FAN_ONDIR )) {/*
@@ -147,12 +154,12 @@ static int fanotify_event_info_len(unsigned int info_mode, }if (info_mode &FAN_REPORT_PIDFD ) - info_len +=FANOTIFY_PIDFD_INFO_HDR_LEN ; + event_len +=FANOTIFY_PIDFD_INFO_HDR_LEN ;if (fh_len) - info_len +=fanotify_fid_info_len (fh_len, dot_len); + event_len +=fanotify_fid_info_len (fh_len, dot_len); -return info_len; +return event_len; }/*
@@ -181,7 +188,7 @@ static void fanotify_unhash_event(struct fsnotify_group *group,static struct fanotify_event *get_one_event (struct fsnotify_group *group,size_t count) { -size_t event_size =FAN_EVENT_METADATA_LEN ; +size_t event_size;struct fanotify_event *event =NULL ;struct fsnotify_event *fsn_event;unsigned int info_mode =FAN_GROUP_FLAG (group,FANOTIFY_INFO_MODES );
@@ -194,8 +201,7 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group, goto out; event =FANOTIFY_E (fsn_event); -if (info_mode) - event_size +=fanotify_event_info_len (info_mode, event); + event_size =fanotify_event_len (info_mode, event);if (event_size > count) { event =ERR_PTR (-EINVAL );
@@ -537,8 +543,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,pr_debug ("%s: group=%p event=%p\n" , __func__, group, event); - metadata.event_len =FAN_EVENT_METADATA_LEN + -fanotify_event_info_len (info_mode, event); + metadata.event_len =fanotify_event_len (info_mode, event); metadata.metadata_len =FAN_EVENT_METADATA_LEN ; metadata.vers =FANOTIFY_METADATA_VERSION ; metadata.reserved =0 ;
Signature · 1 line
2.33.0
PATCH v8 6/32 fanotify: Split fsid check from other fid mode checks
In reply to Gabriel Krisman BertaziFAN_FS_ERROR will require fsid, but not necessarily require the filesystem to expose a file handle. Split those checks into different functions, so they can be used separately when setting up an event.
While there, update a comment about tmpfs having 0 fsid, which is no longer true.
---
Changes since v2:
- FAN_ERROR -> FAN_FS_ERROR (Amir)
- Update comment (Amir)
Changes since v1:
(Amir)
- Sort hunks to simplify diff.
Changes since RFC:
(Amir)
- Rename fanotify_check_path_fsid -> fanotify_test_fsid.
- Use dentry directly instead of path.
--- fs/notify/fanotify/fanotify_user.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)
fs/notify/fanotify/fanotify_user.c +18 -9
@@ -1300,16 +1300,15 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)return fd; } -/* Check if filesystem can encode a unique fid */ -static int fanotify_test_fid (struct path *path,__kernel_fsid_t *fsid) +static int fanotify_test_fsid (struct dentry *dentry,__kernel_fsid_t *fsid) {__kernel_fsid_t root_fsid;int err;/* -* Make sure path is not in filesystem with zero fsid (e.g. tmpfs). +* Make sure dentry is not of a filesystem with zero fsid (e.g. fuse). */ - err =vfs_get_fsid (path->dentry, fsid); + err =vfs_get_fsid (dentry, fsid);if (err)return err;
@@ -1317,10 +1316,10 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)return -ENODEV ;/* -* Make sure path is not inside a filesystem subvolume (e.g. btrfs) +* Make sure dentry is not of a filesystem subvolume (e.g. btrfs) * which uses a different fsid than sb root. */ - err =vfs_get_fsid (path->dentry->d_sb->s_root, &root_fsid); + err =vfs_get_fsid (dentry->d_sb->s_root, &root_fsid);if (err)return err;
@@ -1328,6 +1327,12 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid) root_fsid.val[1] != fsid->val[1])return -EXDEV ; +return 0 ; +} + +/* Check if filesystem can encode a unique fid */ +static int fanotify_test_fid (struct dentry *dentry) +{/* * We need to make sure that the file system supports at least * encoding a file handle so user can use name_to_handle_at() to
@@ -1335,8 +1340,8 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)* objects. However, name_to_handle_at() requires that the * filesystem also supports decoding file handles. */ -if (!path->dentry->d_sb->s_export_op || - !path->dentry->d_sb->s_export_op->fh_to_dentry) +if (!dentry->d_sb->s_export_op || + !dentry->d_sb->s_export_op->fh_to_dentry)return -EOPNOTSUPP ;return 0 ;
@@ -1487,7 +1492,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, }if (fid_mode) { - ret =fanotify_test_fid (&path, &__fsid); + ret =fanotify_test_fsid (path.dentry, &__fsid); +if (ret) + goto path_put_and_out; + + ret =fanotify_test_fid (path.dentry);if (ret) goto path_put_and_out;
Signature · 1 line
2.33.0
PATCH v8 7/32 inotify: Don't force FS_IN_IGNORED
In reply to Gabriel Krisman BertaziAccording to Amir:
"FS_IN_IGNORED is completely internal to inotify and there is no need to set it in i_fsnotify_mask at all, so if we remove the bit from the output of inotify_arg_to_mask() no functionality will change and we will be able to overload the event bit for FS_ERROR."
This is done in preparation to overload FS_ERROR with the notification mechanism in fanotify.
--- fs/notify/inotify/inotify_user.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
fs/notify/inotify/inotify_user.c +3 -3
@@ -94,10 +94,10 @@ static inline __u32 inotify_arg_to_mask(struct inode *inode, u32 arg)__u32 mask;/* -* Everything should accept their own ignored and should receive events -* when the inode is unmounted. All directories care about children. +* Everything should receive events when the inode is unmounted. +* All directories care about children. */ - mask = (FS_IN_IGNORED |FS_UNMOUNT ); + mask = (FS_UNMOUNT );if (S_ISDIR (inode->i_mode)) mask |=FS_EVENT_ON_CHILD ;
Signature · 1 line
2.33.0
PATCH v8 8/32 fsnotify: Add helper to detect overflow_event
In reply to Gabriel Krisman BertaziSimilarly to fanotify_is_perm_event and friends, provide a helper predicate to say whether a mask is of an overflow event.
--- fs/notify/fanotify/fanotify.h | 3 ++- include/linux/fsnotify_backend.h | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-)
fs/notify/fanotify/fanotify.h +2 -1
@@ -315,7 +315,8 @@ 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) && !(mask &FS_Q_OVERFLOW ); +return !(fanotify_is_perm_event (mask) || +fsnotify_is_overflow_event (mask)); } static inline unsigned int fanotify_event_hash_bucket(
include/linux/fsnotify_backend.h +5 -0
@@ -510,6 +510,11 @@ static inline void fsnotify_queue_overflow(struct fsnotify_group *group)fsnotify_add_event (group, group->overflow_event,NULL ,NULL ); } +static inline bool fsnotify_is_overflow_event (u32 mask) +{ +return mask &FS_Q_OVERFLOW ; +} +static inline bool fsnotify_notify_queue_is_empty (struct fsnotify_group *group) {assert_spin_locked (&group->notification_lock);
Signature · 1 line
2.33.0
PATCH v8 9/32 fsnotify: Add wrapper around fsnotify_add_event
In reply to Gabriel Krisman Bertazifsnotify_add_event is growing in number of parameters, which in most case are just passed a NULL pointer. So, split out a new fsnotify_insert_event function to clean things up for users who don't need an insert hook.
--- fs/notify/fanotify/fanotify.c | 4 ++-- fs/notify/inotify/inotify_fsnotify.c | 2 +- fs/notify/notification.c | 12 ++++++------ include/linux/fsnotify_backend.h | 23 ++++++++++++++++------- 4 files changed, 25 insertions(+), 16 deletions(-)
fs/notify/fanotify/fanotify.c +2 -2
@@ -781,8 +781,8 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask, } fsn_event = &event->fse; - ret =fsnotify_add_event (group, fsn_event, fanotify_merge, - fanotify_insert_event); + ret =fsnotify_insert_event (group, fsn_event, fanotify_merge, + fanotify_insert_event);if (ret) {/* Permission events shouldn't be merged */ BUG_ON (ret ==1 && mask &FANOTIFY_PERM_EVENTS );
fs/notify/inotify/inotify_fsnotify.c +1 -1
@@ -116,7 +116,7 @@ int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,if (len)strcpy (event->name, name->name); - ret =fsnotify_add_event (group, fsn_event, inotify_merge,NULL ); + ret =fsnotify_add_event (group, fsn_event, inotify_merge);if (ret) {/* Our event wasn't used in the end. Free it. */ fsnotify_destroy_event (group, fsn_event);
fs/notify/notification.c +6 -6
@@ -78,12 +78,12 @@ void fsnotify_destroy_event(struct fsnotify_group *group,* 2 if the event was not queued - either the queue of events has overflown * or the group is shutting down. */ -int fsnotify_add_event (struct fsnotify_group *group, -struct fsnotify_event *event, -int (*merge)(struct fsnotify_group *, -struct fsnotify_event *), -void (*insert)(struct fsnotify_group *, -struct fsnotify_event *)) +int fsnotify_insert_event (struct fsnotify_group *group, +struct fsnotify_event *event, +int (*merge)(struct fsnotify_group *, +struct fsnotify_event *), +void (*insert)(struct fsnotify_group *, +struct fsnotify_event *)) {int ret =0 ;struct list_head *list = &group->notification_list;
include/linux/fsnotify_backend.h +16 -7
@@ -498,16 +498,25 @@ extern int fsnotify_fasync(int fd, struct file *file, int on);extern void fsnotify_destroy_event (struct fsnotify_group *group,struct fsnotify_event *event);/* attach the event to the group notification queue */ -extern int fsnotify_add_event (struct fsnotify_group *group, -struct fsnotify_event *event, -int (*merge)(struct fsnotify_group *, -struct fsnotify_event *), -void (*insert)(struct fsnotify_group *, -struct fsnotify_event *)); +extern int fsnotify_insert_event (struct fsnotify_group *group, +struct fsnotify_event *event, +int (*merge)(struct fsnotify_group *, +struct fsnotify_event *), +void (*insert)(struct fsnotify_group *, +struct fsnotify_event *)); + +static inline int fsnotify_add_event (struct fsnotify_group *group, +struct fsnotify_event *event, +int (*merge)(struct fsnotify_group *, +struct fsnotify_event *)) +{ +return fsnotify_insert_event (group, event, merge,NULL ); +} +/* Queue overflow event to a notification group */ static inline void fsnotify_queue_overflow (struct fsnotify_group *group) { -fsnotify_add_event (group, group->overflow_event,NULL ,NULL ); +fsnotify_add_event (group, group->overflow_event,NULL ); }static inline bool fsnotify_is_overflow_event (u32 mask)
Signature · 1 line
2.33.0
PATCH v8 10/32 fsnotify: Retrieve super block from the data field
In reply to Gabriel Krisman BertaziSome file system events (i.e. FS_ERROR) might not be associated with an inode or directory. For these, we can retrieve the super block from the data field. But, since the super_block is available in the data field on every event type, simplify the code to always retrieve it from there, through a new helper.
--
Changes since v6:
- Always use data field for superblock retrieval
Changes since v5:
- add fsnotify_data_sb handle to retrieve sb from the data field. (jan)
--- fs/notify/fsnotify.c | 7 +++---- include/linux/fsnotify_backend.h | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-)
fs/notify/fsnotify.c +3 -4
@@ -455,16 +455,16 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)* @file_name is relative to * @file_name: optional file name associated with event * @inode: optional inode associated with event - -* either @dir or @inode must be non-NULL. -* if both are non-NULL event may be reported to both. +* If @dir and @inode are both non-NULL, event may be +* reported to both. * @cookie: inotify rename cookie */ int fsnotify (__u32 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); +struct super_block *sb =fsnotify_data_sb (data, data_type);struct fsnotify_iter_info iter_info = {}; -struct super_block *sb;struct mount *mnt =NULL ;struct inode *parent =NULL ;int ret =0 ;
@@ -483,7 +483,6 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,*/ parent = dir; } - sb = inode->i_sb;/* * Optimization: srcu_read_lock() has a memory barrier which can
include/linux/fsnotify_backend.h +15 -0
@@ -289,6 +289,21 @@ static inline const struct path *fsnotify_data_path(const void *data, } } +static inline struct super_block *fsnotify_data_sb (const void *data, +int data_type) +{ +switch (data_type) { +case FSNOTIFY_EVENT_INODE : +return ((struct inode *)data)->i_sb; +case FSNOTIFY_EVENT_DENTRY : +return ((struct dentry *)data)->d_sb; +case FSNOTIFY_EVENT_PATH : +return ((const struct path *)data)->dentry->d_sb; +default : +return NULL ; + } +} +enum fsnotify_obj_type {FSNOTIFY_OBJ_TYPE_INODE ,FSNOTIFY_OBJ_TYPE_PARENT ,
Signature · 1 line
2.33.0
PATCH v8 11/32 fsnotify: Protect fsnotify_handle_inode_event from no-inode events
In reply to Gabriel Krisman BertaziFAN_FS_ERROR allows events without inodes - i.e. for file system-wide errors. Even though fsnotify_handle_inode_event is not currently used by fanotify, this patch protects this path to handle this new case.
--- fs/notify/fsnotify.c | 3 +++ 1 file changed, 3 insertions(+)
fs/notify/fsnotify.c +3 -0
@@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,if (WARN_ON_ONCE (!ops->handle_inode_event))return 0 ; +if (!inode) +return 0 ; +if ((inode_mark->mask &FS_EXCL_UNLINK ) && path &&d_unlinked (path->dentry))return 0 ;
Signature · 1 line
2.33.0
Amir Goldstein
reviewerRe: [PATCH v8 11/32] fsnotify: Protect fsnotify_handle_inode_event from no-inode events
Quoted 16 lines · Gabriel Krisman Bertazi
FAN_FS_ERRORallows events without inodes - i.e. for file system-wide errors. Even thoughfsnotify_handle_inode_eventis not currently used by fanotify, this patch protects this path to handle this new case.Suggested-by Amir Goldstein <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>--- fs/notify/fsnotify.c | 3 +++ 1 file changed, 3 insertions(+)fs/notify/fsnotify.c +0
-0
fs/notify/fsnotify.c static int fsnotify_handle_inode_event(struct fsnotify_group *group, · line 257
Quoted 6 lines
fs/notify/fsnotify.c +3
-0@@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,if (WARN_ON_ONCE (!ops->handle_inode_event))return 0 ; +if (!inode) +return 0 ; +
Sigh.. the plot thickens.
There are three in-tree backends that implement the ->handle_inode_event() interface.
inotify and dnotify can take NULL inode and the above will make the CREATE events on kernfs vanish, so we cannot do that.
Sorry for not noticing this earlier when I asked for this change.
nfsd_file_fsnotify_handle_event() can most certainly not take NULL inode, but nfsd does not watch for CREATE events.
I think what we need to do is (Jan please correct me if you think otherwise):
1. Document the handle_inode_event() interface that either inode or dir must be non-NULL
2. WARN_ON_ONCE(!inode && !dir) instead of just (!inode) above
3. Add WARN_ON_ONCE(!inode) before trace_nfsd_file_fsnotify_handle_event() in nfsd_file_fsnotify_handle_event()
Apologies, Gabriel, for having to cleanup our mess ;-)
Thanks,
Amir.
Jan Kara
maintainerRe: [PATCH v8 11/32] fsnotify: Protect fsnotify_handle_inode_event from no-inode events
Quoted 29 lines · Amir Goldstein
Quoted 16 lines · Gabriel Krisman Bertazi
FAN_FS_ERRORallows events without inodes - i.e. for file system-wide errors. Even thoughfsnotify_handle_inode_eventis not currently used by fanotify, this patch protects this path to handle this new case.Suggested-by Amir Goldstein <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>--- fs/notify/fsnotify.c | 3 +++ 1 file changed, 3 insertions(+)fs/notify/fsnotify.c +0
-0fs/notify/fsnotify.c static int fsnotify_handle_inode_event(struct fsnotify_group *group, · line 257
Quoted 6 lines
fs/notify/fsnotify.c +3
-0@@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,if (WARN_ON_ONCE (!ops->handle_inode_event))return 0 ; +if (!inode) +return 0 ; +Sigh.. the plot thickens.
There are three in-tree backends that implement the ->handle_inode_event() interface.
Quoted 6 lines
inotify and dnotify can take NULL inode and the above will make the CREATE events on kernfs vanish, so we cannot do that.
Sorry for not noticing this earlier when I asked for this change.
nfsd_file_fsnotify_handle_event()can most certainly not take NULL inode, but nfsd does not watch for CREATE events.
And furthermore you cannot really export kernfs :)
Quoted 4 lines
I think what we need to do is (Jan please correct me if you think otherwise):
1. Document thehandle_inode_event()interface that either inode or dir must be non-NULL
2.WARN_ON_ONCE(!inode && !dir)instead of just (!inode) above
Yeah, like:
if (WARN_ON_ONCE(!inode && !dir)) return 0;
Quoted 2 lines
3. Add
WARN_ON_ONCE(!inode)beforetrace_nfsd_file_fsnotify_handle_event()innfsd_file_fsnotify_handle_event()
And:
if (WARN_ON_ONCE(!inode)) return 0;
Sounds like a good plan to me.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 12/32 fsnotify: Pass group argument to free_event
In reply to Gabriel Krisman BertaziFor group-wide mempool backed events, like FS_ERROR, the free_event callback will need to reference the group's mempool to free the memory. Wire that argument into the current callers.
--- fs/notify/fanotify/fanotify.c | 3 ++- fs/notify/group.c | 2 +- fs/notify/inotify/inotify_fsnotify.c | 3 ++- fs/notify/notification.c | 2 +- include/linux/fsnotify_backend.h | 2 +- 5 files changed, 7 insertions(+), 5 deletions(-)
fs/notify/fanotify/fanotify.c +2 -1
@@ -835,7 +835,8 @@ static void fanotify_free_name_event(struct fanotify_event *event)kfree (FANOTIFY_NE (event)); } -static void fanotify_free_event (struct fsnotify_event *fsn_event) +static void fanotify_free_event (struct fsnotify_group *group, +struct fsnotify_event *fsn_event) {struct fanotify_event *event;
fs/notify/group.c +1 -1
@@ -88,7 +88,7 @@ void fsnotify_destroy_group(struct fsnotify_group *group)* that deliberately ignores overflow events. */ if (group->overflow_event) - group->ops->free_event (group->overflow_event); + group->ops->free_event (group, group->overflow_event);fsnotify_put_group (group); }
fs/notify/inotify/inotify_fsnotify.c +2 -1
@@ -177,7 +177,8 @@ static void inotify_free_group_priv(struct fsnotify_group *group)dec_inotify_instances (group->inotify_data.ucounts); } -static void inotify_free_event (struct fsnotify_event *fsn_event) +static void inotify_free_event (struct fsnotify_group *group, +struct fsnotify_event *fsn_event) {kfree (INOTIFY_E (fsn_event)); }
fs/notify/notification.c +1 -1
@@ -64,7 +64,7 @@ void fsnotify_destroy_event(struct fsnotify_group *group,WARN_ON (!list_empty (&event->list));spin_unlock (&group->notification_lock); } - group->ops->free_event (event); + group->ops->free_event (group, event); }/*
include/linux/fsnotify_backend.h +1 -1
@@ -155,7 +155,7 @@ struct fsnotify_ops { const struct qstr *file_name, u32 cookie);void (*free_group_priv)(struct fsnotify_group *group);void (*freeing_mark)(struct fsnotify_mark *mark,struct fsnotify_group *group); -void (*free_event)(struct fsnotify_event *event); +void (*free_event)(struct fsnotify_group *group,struct fsnotify_event *event);/* called on final put+free to free memory */ void (*free_mark)(struct fsnotify_mark *mark); };
Signature · 1 line
2.33.0
PATCH v8 13/32 fanotify: Support null inode event in fanotify_dfid_inode
In reply to Gabriel Krisman BertaziFAN_FS_ERROR doesn't support DFID, but this function is still called for every event. The problem is that it is not capable of handling null inodes, which now can happen in case of superblock error events. For this case, just returning dir will be enough.
--- fs/notify/fanotify/fanotify.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/notify/fanotify/fanotify.c +1 -1
@@ -452,7 +452,7 @@ static struct inode *fanotify_dfid_inode(u32 event_mask, const void *data,if (event_mask &ALL_FSNOTIFY_DIRENT_EVENTS )return dir; -if (S_ISDIR (inode->i_mode)) +if (inode &&S_ISDIR (inode->i_mode))return inode;return dir;
Signature · 1 line
2.33.0
PATCH v8 14/32 fanotify: Allow file handle encoding for unhashed events
In reply to Gabriel Krisman BertaziAllow passing a NULL hash to fanotify_encode_fh and avoid calculating the hash if not needed.
--- fs/notify/fanotify/fanotify.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
fs/notify/fanotify/fanotify.c +6 -2
@@ -403,8 +403,12 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, fh->type = type; fh->len = fh_len; -/* Mix fh into event merge key */ - *hash ^=fanotify_hash_fh (fh); +/* +* Mix fh into event merge key. Hash might be NULL in case of +* unhashed FID events (i.e. FAN_FS_ERROR). +*/ +if (hash) + *hash ^=fanotify_hash_fh (fh);return FANOTIFY_FH_HDR_LEN + fh_len;
Signature · 1 line
2.33.0
PATCH v8 15/32 fanotify: Encode empty file handle when no inode is provided
In reply to Gabriel Krisman BertaziInstead of failing, encode an invalid file handle in fanotify_encode_fh if no inode is provided. This bogus file handle will be reported by FAN_FS_ERROR for non-inode errors.
---
Changes since v6:
- Use FILEID_ROOT as the internal value (jan)
- Create an empty FH (jan)
Changes since v5:
- Preserve flags initialization (jan)
- Add BUILD_BUG_ON (amir)
- Require minimum of FANOTIFY_NULL_FH_LEN for fh_len(amir)
- Improve comment to explain the null FH length (jan)
- Simplify logic
--- fs/notify/fanotify/fanotify.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
fs/notify/fanotify/fanotify.c +8 -1
@@ -370,8 +370,14 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, fh->type =FILEID_ROOT ; fh->len =0 ; fh->flags =0 ; + +/* +* Invalid FHs are used by FAN_FS_ERROR for errors not +* linked to any inode. The f_handle won't be reported +* back to userspace. +*/ if (!inode) -return 0 ; + goto out;/* * !gpf means preallocated variable size fh, but fh_len could
@@ -403,6 +409,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, fh->type = type; fh->len = fh_len; +out:/* * Mix fh into event merge key. Hash might be NULL in case of * unhashed FID events (i.e. FAN_FS_ERROR).
Signature · 1 line
2.33.0
PATCH v8 16/32 fanotify: Require fid_mode for any non-fd event
In reply to Gabriel Krisman BertaziLike inode events, FAN_FS_ERROR will require fid mode. Therefore, convert the verification during fanotify_mark(2) to require fid for any non-fd event. This means fid_mode will not only be required for inode events, but for any event that doesn't provide a descriptor.
---
changes since v5:
- Fix condition to include FANOTIFY_EVENT_FLAGS. (me)
- Fix comment identation (jan)
--- fs/notify/fanotify/fanotify_user.c | 12 ++++++------ include/linux/fanotify.h | 3 +++ 2 files changed, 9 insertions(+), 6 deletions(-)
fs/notify/fanotify/fanotify_user.c +6 -6
@@ -1458,14 +1458,14 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, goto fput_and_out;/* -* Events with data type inode do not carry enough information to report -* event->fd, so we do not allow setting a mask for inode events unless -* group supports reporting fid. -* inode events are not supported on a mount mark, because they do not -* carry enough information (i.e. path) to be filtered by mount point. +* Events that do not carry enough information to report +* event->fd require a group that supports reporting fid. Those +* events are not supported on a mount mark, because they do not +* carry enough information (i.e. path) to be filtered by mount +* point. */ fid_mode =FAN_GROUP_FLAG (group,FANOTIFY_FID_BITS ); -if (mask &FANOTIFY_INODE_EVENTS && +if (mask & ~(FANOTIFY_FD_EVENTS |FANOTIFY_EVENT_FLAGS ) && (!fid_mode || mark_type ==FAN_MARK_MOUNT )) goto fput_and_out;
include/linux/fanotify.h +3 -0
@@ -84,6 +84,9 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */*/ #define FANOTIFY_DIRENT_EVENTS (FAN_MOVE | FAN_CREATE | FAN_DELETE) +/* Events that can be reported with event->fd */ +#define FANOTIFY_FD_EVENTS (FANOTIFY_PATH_EVENTS | FANOTIFY_PERM_EVENTS) +/* Events that can only be reported with data type FSNOTIFY_EVENT_INODE */ #define FANOTIFY_INODE_EVENTS (FANOTIFY_DIRENT_EVENTS | \ FAN_ATTRIB | FAN_MOVE_SELF | FAN_DELETE_SELF)
Signature · 1 line
2.33.0
PATCH v8 17/32 fsnotify: Support FS_ERROR event type
In reply to Gabriel Krisman BertaziExpose a new type of fsnotify event for filesystems to report errors for userspace monitoring tools. fanotify will send this type of notification for FAN_FS_ERROR events. This also introduce a helper for generating the new event.
---
Changes since v6:
- Add fsnotify_data_error_report
Changes since v5:
- pass sb inside data field (jan)
Changes since v3:
- Squash patch ("fsnotify: Introduce helpers to send error_events")
- Drop reviewed-bys!
Changes since v2:
- FAN_ERROR->FAN_FS_ERROR (Amir)
Changes since v1:
- Overload FS_ERROR with FS_IN_IGNORED
- Implement support for this type on fsnotify_data_inode (Amir)
--- include/linux/fsnotify.h | 13 +++++++++++++ include/linux/fsnotify_backend.h | 32 +++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-)
include/linux/fsnotify.h +13 -0
@@ -339,4 +339,17 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid) fsnotify_dentry(dentry, mask); } +static inline int fsnotify_sb_error (struct super_block *sb,struct inode *inode, +int error) +{ +struct fs_error_report report = { + .error = error, + .inode = inode, + .sb = sb, + }; + +return fsnotify (FS_ERROR , &report,FSNOTIFY_EVENT_ERROR , +NULL ,NULL ,NULL ,0 ); +} +#endif /* _LINUX_FS_NOTIFY_H */
include/linux/fsnotify_backend.h +31 -1
@@ -42,6 +42,12 @@#define FS_UNMOUNT 0x00002000/* inode on umount fs */ #define FS_Q_OVERFLOW 0x00004000/* Event queued overflowed */ +#define FS_ERROR 0x00008000/* Filesystem Error (fanotify) */ + +/* +* FS_IN_IGNORED overloads FS_ERROR. It is only used internally by inotify +* which does not support FS_ERROR. +*/ #define FS_IN_IGNORED 0x00008000/* last inotify event here */ #define FS_OPEN_PERM 0x00010000/* open event in an permission hook */
@@ -95,7 +101,8 @@#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \ FS_EVENTS_POSS_ON_CHILD | \ FS_DELETE_SELF | FS_MOVE_SELF | FS_DN_RENAME | \ - FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED) + FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \ + FS_ERROR)/* Extra flags that may be reported with event or control handling of events */ #define ALL_FSNOTIFY_FLAGS (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
@@ -249,6 +256,13 @@ enum fsnotify_data_type {FSNOTIFY_EVENT_PATH ,FSNOTIFY_EVENT_INODE ,FSNOTIFY_EVENT_DENTRY , + FSNOTIFY_EVENT_ERROR, +}; + +struct fs_error_report { +int error; +struct inode *inode; +struct super_block *sb; };static inline struct inode *fsnotify_data_inode (const void *data,int data_type)
@@ -260,6 +274,8 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)return d_inode (data);case FSNOTIFY_EVENT_PATH :return d_inode (((const struct path *)data)->dentry); +case FSNOTIFY_EVENT_ERROR : +return ((struct fs_error_report *)data)->inode;default :return NULL ; }
@@ -299,6 +315,20 @@ static inline struct super_block *fsnotify_data_sb(const void *data,return ((struct dentry *)data)->d_sb;case FSNOTIFY_EVENT_PATH :return ((const struct path *)data)->dentry->d_sb; +case FSNOTIFY_EVENT_ERROR : +return ((struct fs_error_report *) data)->sb; +default : +return NULL ; + } +} + +static inline struct fs_error_report *fsnotify_data_error_report ( +const void *data, +int data_type) +{ +switch (data_type) { +case FSNOTIFY_EVENT_ERROR : +return (struct fs_error_report *) data;default :return NULL ; }
Signature · 1 line
2.33.0
PATCH v8 18/32 fanotify: Reserve UAPI bits for FAN_FS_ERROR
In reply to Gabriel Krisman BertaziFAN_FS_ERROR allows reporting of event type FS_ERROR to userspace, which is a mechanism to report file system wide problems via fanotify. This commit preallocate userspace visible bits to match the FS_ERROR event.
--- fs/notify/fanotify/fanotify.c | 1 + include/uapi/linux/fanotify.h | 1 + 2 files changed, 2 insertions(+)
fs/notify/fanotify/fanotify.c +1 -0
@@ -752,6 +752,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,BUILD_BUG_ON (FAN_ONDIR !=FS_ISDIR );BUILD_BUG_ON (FAN_OPEN_EXEC !=FS_OPEN_EXEC );BUILD_BUG_ON (FAN_OPEN_EXEC_PERM !=FS_OPEN_EXEC_PERM ); +BUILD_BUG_ON (FAN_FS_ERROR !=FS_ERROR );BUILD_BUG_ON (HWEIGHT32 (ALL_FANOTIFY_EVENT_BITS ) !=19 );
include/uapi/linux/fanotify.h +1 -0
@@ -20,6 +20,7 @@#define FAN_OPEN_EXEC 0x00001000/* File was opened for exec */ #define FAN_Q_OVERFLOW 0x00004000/* Event queued overflowed */ +#define FAN_FS_ERROR 0x00008000/* Filesystem error */ #define FAN_OPEN_PERM 0x00010000/* File open in perm check */ #define FAN_ACCESS_PERM 0x00020000/* File accessed in perm check */
Signature · 1 line
2.33.0
PATCH v8 19/32 fanotify: Pre-allocate pool of error events
In reply to Gabriel Krisman BertaziPre-allocate slots for file system errors to have greater chances of succeeding, since error events can happen in GFP_NOFS context. This patch introduces a group-wide mempool of error events, shared by all FAN_FS_ERROR marks in this group.
For now, just allocate 128 positions. A future patch allows this array to be dynamically resized when a new mark is added.
---
Changes since v7:
- Expand limit to 128. (Amir)
--- 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(-)
fs/notify/fanotify/fanotify.c +3 -0
@@ -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)
fs/notify/fanotify/fanotify.h +11 -0
@@ -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 )
fs/notify/fanotify/fanotify_user.c +25 -1
@@ -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_MAX_FEE_POOL 128/* * 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_MAX_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,
include/linux/fsnotify_backend.h +2 -0
@@ -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 */ };
Signature · 1 line
2.33.0
Re: [PATCH v8 19/32] fanotify: Pre-allocate pool of error events
Quoted 6 lines · Gabriel Krisman Bertazi
Pre-allocate slots for file system errors to have greater chances of succeeding, since error events can happen in
GFP_NOFScontext. This patch introduces a group-wide mempool of error events, shared by allFAN_FS_ERRORmarks in this group.
Quoted 8 lines
For now, just allocate 128 positions. A future patch allows this array to be dynamically resized when a new mark is added.
Signed-off-by Gabriel Krisman Bertazi <[email protected]>---
Changes since v7:
- Expand limit to 128. (Amir)
I am not sure if Jan was also on board with this request but otherwise
Quoted 137 lines
--- 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(-)fs/notify/fanotify/fanotify.c +3
-0@@ -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)fs/notify/fanotify/fanotify.h +11
-0@@ -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 )fs/notify/fanotify/fanotify_user.c +25
-1@@ -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_MAX_FEE_POOL 128/* * 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_MAX_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,include/linux/fsnotify_backend.h +2
-1@@ -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
Re: [PATCH v8 19/32] fanotify: Pre-allocate pool of error events
Quoted 13 lines · Amir Goldstein
Quoted 11 lines · Gabriel Krisman Bertazi
Pre-allocate slots for file system errors to have greater chances of succeeding, since error events can happen in
GFP_NOFScontext. This patch introduces a group-wide mempool of error events, shared by allFAN_FS_ERRORmarks in this group.For now, just allocate 128 positions. A future patch allows this array to be dynamically resized when a new mark is added.
Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Quoted 7 lines
Quoted 3 lines
---
Changes since v7:
- Expand limit to 128. (Amir)I am not sure if Jan was also on board with this request but otherwise
Reviewed-by Amir Goldstein <[email protected]>
I don't really care. I don't see a strong reason to go above original 32 (so I'd slightly prefer that) but OTOH I also don't think those few KB per notification group using FS_ERROR matter since I don't expect such groups to be that common.
Quoted 9 lines
fs/notify/fanotify/fanotify_user.c line 33
Quoted 9 lines
fs/notify/fanotify/fanotify_user.c +1
-0@@ -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_MAX_FEE_POOL 128
Perhaps FANOTIFY_DEFAULT_FEE_POOL_SIZE would better describe what this constant is about?
Otherwise feel free to add:
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 20/32 fanotify: Dynamically resize the FAN_FS_ERROR pool
In reply to Gabriel Krisman BertaziAllow 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.
--- fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- include/linux/fsnotify_backend.h | 1 + 2 files changed, 22 insertions(+), 5 deletions(-)
fs/notify/fanotify/fanotify_user.c +21 -5
@@ -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,
include/linux/fsnotify_backend.h +1 -0
@@ -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 */ };
Signature · 1 line
2.33.0
Amir Goldstein
reviewerRe: [PATCH v8 20/32] fanotify: Dynamically resize the FAN_FS_ERROR pool
Quoted 46 lines · Gabriel Krisman Bertazi
Allow the
FAN_FS_ERRORgroup 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 <[email protected]>--- fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- include/linux/fsnotify_backend.h | 1 + 2 files changed, 22 insertions(+), 5 deletions(-)fs/notify/fanotify/fanotify_user.c +12
-5@@ -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,
fs/notify/fanotify/fanotify_user.c static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, · line 1082
Quoted 10 lines
fs/notify/fanotify/fanotify_user.c +9
-0+ 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.
Jan Kara
maintainerRe: [PATCH v8 20/32] fanotify: Dynamically resize the FAN_FS_ERROR pool
Quoted 53 lines · Amir Goldstein
Quoted 51 lines · Gabriel Krisman Bertazi
Allow the
FAN_FS_ERRORgroup 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 <[email protected]>--- fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- include/linux/fsnotify_backend.h | 1 + 2 files changed, 22 insertions(+), 5 deletions(-)fs/notify/fanotify/fanotify_user.c +17
-5@@ -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 ); +
Quoted 8 lines
fs/notify/fanotify/fanotify_user.c static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, · line 1082
Quoted 5 lines
fs/notify/fanotify/fanotify_user.c +4
-0+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.
Quoted 1 line
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...
Quoted 3 lines
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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH v8 20/32] fanotify: Dynamically resize the FAN_FS_ERROR pool
Quoted 77 lines · Jan Kara
Quoted 53 lines · Amir Goldstein
Quoted 51 lines · Gabriel Krisman Bertazi
Allow the
FAN_FS_ERRORgroup 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 <[email protected]>--- fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- include/linux/fsnotify_backend.h | 1 + 2 files changed, 22 insertions(+), 5 deletions(-)fs/notify/fanotify/fanotify_user.c +17
-5@@ -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 ); +Quoted 8 lines
fs/notify/fanotify/fanotify_user.c static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, · line 1082
Quoted 5 lines
fs/notify/fanotify/fanotify_user.c +4
-0+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_ERRORmarks 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, alsomempool_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.Quoted 1 line
And also, this code grows the pool to infinity with add/remove mark loop.
I see a cap at
FANOTIFY_DEFAULT_MAX_FEE_POOLin the code there. But I don't think there's a good enough reason to hard-limit number ofFS_ERRORmarks 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...
Quoted 7 lines
Quoted 3 lines
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.
Signature · 1 line
Gabriel Krisman Bertazi
Jan Kara
maintainerRe: [PATCH v8 20/32] fanotify: Dynamically resize the FAN_FS_ERROR pool
Quoted 83 lines · Gabriel Krisman Bertazi
Quoted 81 lines · Jan Kara
Quoted 53 lines · Amir Goldstein
Quoted 51 lines · Gabriel Krisman Bertazi
Allow the
FAN_FS_ERRORgroup 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 <[email protected]>--- fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++----- include/linux/fsnotify_backend.h | 1 + 2 files changed, 22 insertions(+), 5 deletions(-)fs/notify/fanotify/fanotify_user.c +17
-5@@ -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 ); +Quoted 8 lines
fs/notify/fanotify/fanotify_user.c static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, · line 1082
Quoted 5 lines
fs/notify/fanotify/fanotify_user.c +4
-0+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_ERRORmarks 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, alsomempool_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.Quoted 1 line
And also, this code grows the pool to infinity with add/remove mark loop.
I see a cap at
FANOTIFY_DEFAULT_MAX_FEE_POOLin the code there. But I don't think there's a good enough reason to hard-limit number ofFS_ERRORmarks 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...Quoted 3 lines
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.
Quoted 7 lines
Quoted 3 lines
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_ERRORmarks 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.
Quoted 2 lines
In the future, if this ever becomes a problem, we look into dynamic resizing/increasing the limit?
Yes.
Quoted 3 lines
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_resizeactually 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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 21/32 fanotify: Support enqueueing of error events
In reply to Gabriel Krisman BertaziOnce an error event is triggered, enqueue it in the notification group, similarly to what is done for other events. FAN_FS_ERROR is not handled specially, since the memory is now handled by a preallocated mempool.
For now, make the event unhashed. A future patch implements merging of this kind of event.
---
Changes since v7:
- WARN_ON -> WARN_ON_ONCE (Amir)
--- fs/notify/fanotify/fanotify.c | 35 +++++++++++++++++++++++++++++++++++ fs/notify/fanotify/fanotify.h | 6 ++++++ 2 files changed, 41 insertions(+)
fs/notify/fanotify/fanotify.c +35 -0
@@ -574,6 +574,27 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,return &fne->fae; } +static struct fanotify_event *fanotify_alloc_error_event ( +struct fsnotify_group *group, +__kernel_fsid_t *fsid, +const void *data,int data_type) +{ +struct fs_error_report *report = +fsnotify_data_error_report (data, data_type); +struct fanotify_error_event *fee; + +if (WARN_ON_ONCE (!report)) +return NULL ; + + fee =mempool_alloc (&group->fanotify_data.error_events_pool,GFP_NOFS ); +if (!fee) +return NULL ; + + fee->fae.type =FANOTIFY_EVENT_TYPE_FS_ERROR ; + +return &fee->fae; +} + static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group, u32 mask, const void *data, int data_type, struct inode *dir,
@@ -641,6 +662,9 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,if (fanotify_is_perm_event (mask)) { event =fanotify_alloc_perm_event (path, gfp); + }else if (fanotify_is_error_event (mask)) { + event =fanotify_alloc_error_event (group, fsid, data, + data_type); }else if (name_event && (file_name || child)) { event =fanotify_alloc_name_event (id, fsid, file_name, child, &hash, gfp);
@@ -850,6 +874,14 @@ static void fanotify_free_name_event(struct fanotify_event *event)kfree (FANOTIFY_NE (event)); } +static void fanotify_free_error_event (struct fsnotify_group *group, +struct fanotify_event *event) +{ +struct fanotify_error_event *fee =FANOTIFY_EE (event); + +mempool_free (fee, &group->fanotify_data.error_events_pool); +} +static void fanotify_free_event (struct fsnotify_group *group,struct fsnotify_event *fsn_event) {
@@ -873,6 +905,9 @@ static void fanotify_free_event(struct fsnotify_group *group,case FANOTIFY_EVENT_TYPE_OVERFLOW :kfree (event);break ; +case FANOTIFY_EVENT_TYPE_FS_ERROR : +fanotify_free_error_event (group, event); +break ;default :WARN_ON_ONCE (1 ); }
fs/notify/fanotify/fanotify.h +6 -0
@@ -298,6 +298,11 @@ static inline struct fanotify_event *FANOTIFY_E(struct fsnotify_event *fse) return container_of(fse, struct fanotify_event, fse); } +static inline bool fanotify_is_error_event (u32 mask) +{ +return mask &FAN_FS_ERROR ; +} +static inline bool fanotify_event_has_path (struct fanotify_event *event) { return event->type == FANOTIFY_EVENT_TYPE_PATH ||
@@ -327,6 +332,7 @@ 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)); }
Signature · 1 line
2.33.0
Re: [PATCH v8 21/32] fanotify: Support enqueueing of error events
Quoted 6 lines · Gabriel Krisman Bertazi
Once an error event is triggered, enqueue it in the notification group, similarly to what is done for other events.
FAN_FS_ERRORis not handled specially, since the memory is now handled by a preallocated mempool.
Quoted 6 lines
For now, make the event unhashed. A future patch implements merging of this kind of event.
Reviewed-by Jan Kara <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Quoted 102 lines
---
Changes since v7:
-WARN_ON->WARN_ON_ONCE(Amir)--- fs/notify/fanotify/fanotify.c | 35 +++++++++++++++++++++++++++++++++++ fs/notify/fanotify/fanotify.h | 6 ++++++ 2 files changed, 41 insertions(+)fs/notify/fanotify/fanotify.c +35
-0@@ -574,6 +574,27 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,return &fne->fae; } +static struct fanotify_event *fanotify_alloc_error_event ( +struct fsnotify_group *group, +__kernel_fsid_t *fsid, +const void *data,int data_type) +{ +struct fs_error_report *report = +fsnotify_data_error_report (data, data_type); +struct fanotify_error_event *fee; + +if (WARN_ON_ONCE (!report)) +return NULL ; + + fee =mempool_alloc (&group->fanotify_data.error_events_pool,GFP_NOFS ); +if (!fee) +return NULL ; + + fee->fae.type =FANOTIFY_EVENT_TYPE_FS_ERROR ; + +return &fee->fae; +} + static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group, u32 mask, const void *data, int data_type, struct inode *dir,@@ -641,6 +662,9 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,if (fanotify_is_perm_event (mask)) { event =fanotify_alloc_perm_event (path, gfp); + }else if (fanotify_is_error_event (mask)) { + event =fanotify_alloc_error_event (group, fsid, data, + data_type); }else if (name_event && (file_name || child)) { event =fanotify_alloc_name_event (id, fsid, file_name, child, &hash, gfp);@@ -850,6 +874,14 @@ static void fanotify_free_name_event(struct fanotify_event *event)kfree (FANOTIFY_NE (event)); } +static void fanotify_free_error_event (struct fsnotify_group *group, +struct fanotify_event *event) +{ +struct fanotify_error_event *fee =FANOTIFY_EE (event); + +mempool_free (fee, &group->fanotify_data.error_events_pool); +} +static void fanotify_free_event (struct fsnotify_group *group,struct fsnotify_event *fsn_event) {@@ -873,6 +905,9 @@ static void fanotify_free_event(struct fsnotify_group *group,case FANOTIFY_EVENT_TYPE_OVERFLOW :kfree (event);break ; +case FANOTIFY_EVENT_TYPE_FS_ERROR : +fanotify_free_error_event (group, event); +break ;default :WARN_ON_ONCE (1 ); }fs/notify/fanotify/fanotify.h +6
-1@@ -298,6 +298,11 @@ static inline struct fanotify_event *FANOTIFY_E(struct fsnotify_event *fse) return container_of(fse, struct fanotify_event, fse); } +static inline bool fanotify_is_error_event (u32 mask) +{ +return mask &FAN_FS_ERROR ; +} +static inline bool fanotify_event_has_path (struct fanotify_event *event) { return event->type == FANOTIFY_EVENT_TYPE_PATH ||@@ -327,6 +332,7 @@ 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)); } --2.33.0
PATCH v8 22/32 fanotify: Support merging of error events
In reply to Gabriel Krisman BertaziError 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.
---
Changes since v7:
- Move fee->fsid assignment here (Amir)
- Open code error event merge logic in fanotify_merge (Jan)
--- fs/notify/fanotify/fanotify.c | 26 ++++++++++++++++++++++++-- fs/notify/fanotify/fanotify.h | 4 +++- 2 files changed, 27 insertions(+), 3 deletions(-)
fs/notify/fanotify/fanotify.c +24 -2
@@ -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 ); }
@@ -176,6 +189,10 @@ static int fanotify_merge(struct fsnotify_group *group,break ;if (fanotify_should_merge (old, new)) { old->mask |= new->mask; + +if (fanotify_is_error_event (old->mask)) +FANOTIFY_EE (old)->err_count++; +return 1 ; } }
@@ -577,7 +594,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 +609,10 @@ static struct fanotify_event *fanotify_alloc_error_event(return NULL ; fee->fae.type =FANOTIFY_EVENT_TYPE_FS_ERROR ; + fee->err_count =1 ; + fee->fsid = *fsid; + + *hash ^=fanotify_hash_fsid (fsid);return &fee->fae; }
@@ -664,7 +686,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);
fs/notify/fanotify/fanotify.h +3 -1
@@ -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)); }
Signature · 1 line
2.33.0
Re: [PATCH v8 22/32] fanotify: Support merging of error events
Quoted 8 lines · Gabriel Krisman Bertazi
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 <[email protected]>
Quoted 106 lines
---
Changes since v7:
- Movefee->fsidassignment here (Amir)
- Open code error event merge logic infanotify_merge(Jan)--- fs/notify/fanotify/fanotify.c | 26 ++++++++++++++++++++++++-- fs/notify/fanotify/fanotify.h | 4 +++- 2 files changed, 27 insertions(+), 3 deletions(-)fs/notify/fanotify/fanotify.c +24
-2@@ -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 ); }@@ -176,6 +189,10 @@ static int fanotify_merge(struct fsnotify_group *group,break ;if (fanotify_should_merge (old, new)) { old->mask |= new->mask; + +if (fanotify_is_error_event (old->mask)) +FANOTIFY_EE (old)->err_count++; +return 1 ; } }@@ -577,7 +594,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 +609,10 @@ static struct fanotify_event *fanotify_alloc_error_event(return NULL ; fee->fae.type =FANOTIFY_EVENT_TYPE_FS_ERROR ; + fee->err_count =1 ; + fee->fsid = *fsid; + + *hash ^=fanotify_hash_fsid (fsid);return &fee->fae; }@@ -664,7 +686,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);fs/notify/fanotify/fanotify.h +3
-2@@ -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)); } --2.33.0
Re: [PATCH v8 22/32] fanotify: Support merging of error events
In reply to Gabriel Krisman BertaziQuoted 6 lines · Gabriel Krisman Bertazi
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 <[email protected]>
Looks good to me. Feel free to add:
Honza
Quoted 106 lines
---
Changes since v7:
- Movefee->fsidassignment here (Amir)
- Open code error event merge logic infanotify_merge(Jan)--- fs/notify/fanotify/fanotify.c | 26 ++++++++++++++++++++++++-- fs/notify/fanotify/fanotify.h | 4 +++- 2 files changed, 27 insertions(+), 3 deletions(-)fs/notify/fanotify/fanotify.c +24
-2@@ -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 ); }@@ -176,6 +189,10 @@ static int fanotify_merge(struct fsnotify_group *group,break ;if (fanotify_should_merge (old, new)) { old->mask |= new->mask; + +if (fanotify_is_error_event (old->mask)) +FANOTIFY_EE (old)->err_count++; +return 1 ; } }@@ -577,7 +594,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 +609,10 @@ static struct fanotify_event *fanotify_alloc_error_event(return NULL ; fee->fae.type =FANOTIFY_EVENT_TYPE_FS_ERROR ; + fee->err_count =1 ; + fee->fsid = *fsid; + + *hash ^=fanotify_hash_fsid (fsid);return &fee->fae; }@@ -664,7 +686,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);fs/notify/fanotify/fanotify.h +3
-1@@ -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)); }Signature · 1 line
2.33.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 23/32 fanotify: Wrap object_fh inline space in a creator macro
In reply to Gabriel Krisman Bertazifanotify_error_event would duplicate this sequence of declarations that already exist elsewhere with a slight different size. Create a helper macro to avoid code duplication.
---
Among the suggestions, I think this is simpler because it avoids deep nesting the variable-sized attribute, which would have been hidden inside fee->ffe->object_fh.buf.
It also avoids touching the allocators, which are nicely hidden inside helper KMEM_CACHE() macros that hides several parameters.
--- fs/notify/fanotify/fanotify.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
fs/notify/fanotify/fanotify.h +10 -3
@@ -171,12 +171,19 @@ static inline void fanotify_init_event(struct fanotify_event *event, event->pid =NULL ; } +#define FANOTIFY_INLINE_FH (size) \ +struct { \ + struct fanotify_fh object_fh; \ +/* Space for object_fh.buf[] - access with fanotify_fh_buf() */ \ +unsigned char _inline_fh_buf[(size)]; \ +} +struct fanotify_fid_event {struct fanotify_event fae;__kernel_fsid_t fsid; -struct fanotify_fh object_fh; -/* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */ -unsigned char _inline_fh_buf[FANOTIFY_INLINE_FH_LEN ]; + +/* This must be the last element of the structure. */ +FANOTIFY_INLINE_FH (FANOTIFY_INLINE_FH_LEN); }; static inline struct fanotify_fid_event *
Signature · 1 line
2.33.0
Re: [PATCH v8 23/32] fanotify: Wrap object_fh inline space in a creator macro
Quoted 8 lines · Gabriel Krisman Bertazi
fanotify_error_eventwould duplicate this sequence of declarations that already exist elsewhere with a slight different size. Create a helper macro to avoid code duplication.Suggested-by Jan Kara <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>
with minor nit
Quoted 7 lines
---
Among the suggestions, I think this is simpler because it avoids deep nesting the variable-sized attribute, which would have been hidden insidefee->ffe->object_fh.buf.It also avoids touching the allocators, which are nicely hidden inside helper
KMEM_CACHE()macros that hides several parameters.
I like this option best as well.
Quoted 19 lines
--- fs/notify/fanotify/fanotify.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)fs/notify/fanotify/fanotify.h +7
-0@@ -171,12 +171,19 @@ static inline void fanotify_init_event(struct fanotify_event *event, event->pid =NULL ; } +#define FANOTIFY_INLINE_FH (size) \ +struct { \ + struct fanotify_fh object_fh; \ +/* Space for object_fh.buf[] - access with fanotify_fh_buf() */ \ +unsigned char _inline_fh_buf[(size)]; \ +} +
fs/notify/fanotify/fanotify.h static inline void fanotify_init_event(struct fanotify_event *event, · line 187
Quoted 10 lines
fs/notify/fanotify/fanotify.h +3
-3struct fanotify_fid_event {struct fanotify_event fae;__kernel_fsid_t fsid; -struct fanotify_fh object_fh; -/* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */ -unsigned char _inline_fh_buf[FANOTIFY_INLINE_FH_LEN ]; + +/* This must be the last element of the structure. */ +FANOTIFY_INLINE_FH (FANOTIFY_INLINE_FH_LEN); };
It's not true that is must be the last element.
this is only true for struct fanotify_fh with zero size buf[].
Thanks,
Amir.
Re: [PATCH v8 23/32] fanotify: Wrap object_fh inline space in a creator macro
In reply to Gabriel Krisman BertaziQuoted 11 lines · Gabriel Krisman Bertazi
fanotify_error_eventwould duplicate this sequence of declarations that already exist elsewhere with a slight different size. Create a helper macro to avoid code duplication.Suggested-by Jan Kara <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>---
Among the suggestions, I think this is simpler because it avoids deep nesting the variable-sized attribute, which would have been hidden insidefee->ffe->object_fh.buf.
One nit from me as well :)
fs/notify/fanotify/fanotify.h static inline void fanotify_init_event(struct fanotify_event *event, · line 180
Quoted 7 lines
fs/notify/fanotify/fanotify.h +7
-0+#define FANOTIFY_INLINE_FH (size) \ +struct { \ + struct fanotify_fh object_fh; \ +/* Space for object_fh.buf[] - access with fanotify_fh_buf() */ \ +unsigned char _inline_fh_buf[(size)]; \ +} +
Can the macro perhaps take the name of the fanotify_fh member it creates? Like:
#define FANOTIFY_INLINE_FH(name, size)
Harcoding _inline_fh_buf is fine since it isn't ever used directly but hardcoding object_fh looks ugly to me. With that improved feel free to add:
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 24/32 fanotify: Add helpers to decide whether to report FID/DFID
In reply to Gabriel Krisman BertaziNow that there is an event that reports FID records even for a zeroed file handle, wrap the logic that deides whether to issue the records into helper functions. This shouldn't have any impact on the code, but simplifies further patches.
--- fs/notify/fanotify/fanotify.h | 13 +++++++++++++ fs/notify/fanotify/fanotify_user.c | 13 +++++++------ 2 files changed, 20 insertions(+), 6 deletions(-)
fs/notify/fanotify/fanotify.h +13 -0
@@ -265,6 +265,19 @@ static inline int fanotify_event_dir_fh_len(struct fanotify_event *event)return info ?fanotify_info_dir_fh_len (info) :0 ; } +static inline bool fanotify_event_has_object_fh (struct fanotify_event *event) +{ +if (fanotify_event_object_fh_len (event) >0 ) +return true; + +return false; +} + +static inline bool fanotify_event_has_dir_fh (struct fanotify_event *event) +{ +return (fanotify_event_dir_fh_len (event) >0 ) ? true : false; +} +struct fanotify_path_event {struct fanotify_event fae;struct path path;
fs/notify/fanotify/fanotify_user.c +7 -6
@@ -140,10 +140,9 @@ static size_t fanotify_event_len(unsigned int info_mode,return event_len; info =fanotify_event_info (event); - dir_fh_len =fanotify_event_dir_fh_len (event); - fh_len =fanotify_event_object_fh_len (event); -if (dir_fh_len) { +if (fanotify_event_has_dir_fh (event)) { + dir_fh_len =fanotify_event_dir_fh_len (event); event_len +=fanotify_fid_info_len (dir_fh_len, info->name_len); }else if ((info_mode &FAN_REPORT_NAME ) && (event->mask &FAN_ONDIR )) {
@@ -157,8 +156,10 @@ static size_t fanotify_event_len(unsigned int info_mode,if (info_mode &FAN_REPORT_PIDFD ) event_len +=FANOTIFY_PIDFD_INFO_HDR_LEN ; -if (fh_len) +if (fanotify_event_has_object_fh (event)) { + fh_len =fanotify_event_object_fh_len (event); event_len +=fanotify_fid_info_len (fh_len, dot_len); + }return event_len; }
@@ -451,7 +452,7 @@ static int copy_info_records_to_user(struct fanotify_event *event,/* * Event info records order is as follows: dir fid + name, child fid. */ -if (fanotify_event_dir_fh_len (event)) { +if (fanotify_event_has_dir_fh (event)) { info_type = info->name_len ?FAN_EVENT_INFO_TYPE_DFID_NAME :FAN_EVENT_INFO_TYPE_DFID ; ret = copy_fid_info_to_user(fanotify_event_fsid(event),
@@ -467,7 +468,7 @@ static int copy_info_records_to_user(struct fanotify_event *event, total_bytes += ret; } -if (fanotify_event_object_fh_len (event)) { +if (fanotify_event_has_object_fh (event)) {const char *dot =NULL ;int dot_len =0 ;
Signature · 1 line
2.33.0
Amir Goldstein
reviewerRe: [PATCH v8 24/32] fanotify: Add helpers to decide whether to report FID/DFID
Quoted 17 lines · Gabriel Krisman Bertazi
Now that there is an event that reports FID records even for a zeroed file handle, wrap the logic that deides whether to issue the records into helper functions. This shouldn't have any impact on the code, but simplifies further patches.
Signed-off-by Gabriel Krisman Bertazi <[email protected]>--- fs/notify/fanotify/fanotify.h | 13 +++++++++++++ fs/notify/fanotify/fanotify_user.c | 13 +++++++------ 2 files changed, 20 insertions(+), 6 deletions(-)fs/notify/fanotify/fanotify.h +0
-0
fs/notify/fanotify/fanotify.h static inline int fanotify_event_dir_fh_len(struct fanotify_event *event) · line 273
Quoted 9 lines
fs/notify/fanotify/fanotify.h +6
-0@@ -265,6 +265,19 @@ static inline int fanotify_event_dir_fh_len(struct fanotify_event *event)return info ?fanotify_info_dir_fh_len (info) :0 ; } +static inline bool fanotify_event_has_object_fh (struct fanotify_event *event) +{ +if (fanotify_event_object_fh_len (event) >0 ) +return true; + +return false;
Sorry, this construct gives me a rush ;)
What's wrong with
return fanotify_event_object_fh_len(event) > 0;
fs/notify/fanotify/fanotify.h static inline int fanotify_event_dir_fh_len(struct fanotify_event *event) · line 279
Quoted 6 lines
fs/notify/fanotify/fanotify.h +6
-0+} + +static inline bool fanotify_event_has_dir_fh (struct fanotify_event *event) +{ +return (fanotify_event_dir_fh_len (event) >0 ) ? true : false; +}
Likewise, except '(cond) ? true : false' gives me an even more irritating rush...
Thanks,
Amir.
Re: [PATCH v8 24/32] fanotify: Add helpers to decide whether to report FID/DFID
In reply to Gabriel Krisman BertaziQuoted 2 lines · Gabriel Krisman Bertazi
Now that there is an event that reports FID records even for a zeroed file handle, wrap the logic that deides whether to issue the records
^^^^ decides
Quoted 4 lines
into helper functions. This shouldn't have any impact on the code, but simplifies further patches.
Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Looks good to me but I agree with Amir there's no need to explicit true / false returns when checking just a simple condition.
Feel free to add:
Honza
Quoted 79 lines
--- fs/notify/fanotify/fanotify.h | 13 +++++++++++++ fs/notify/fanotify/fanotify_user.c | 13 +++++++------ 2 files changed, 20 insertions(+), 6 deletions(-)fs/notify/fanotify/fanotify.h +13
-0@@ -265,6 +265,19 @@ static inline int fanotify_event_dir_fh_len(struct fanotify_event *event)return info ?fanotify_info_dir_fh_len (info) :0 ; } +static inline bool fanotify_event_has_object_fh (struct fanotify_event *event) +{ +if (fanotify_event_object_fh_len (event) >0 ) +return true; + +return false; +} + +static inline bool fanotify_event_has_dir_fh (struct fanotify_event *event) +{ +return (fanotify_event_dir_fh_len (event) >0 ) ? true : false; +} +struct fanotify_path_event {struct fanotify_event fae;struct path path;fs/notify/fanotify/fanotify_user.c +7
-6@@ -140,10 +140,9 @@ static size_t fanotify_event_len(unsigned int info_mode,return event_len; info =fanotify_event_info (event); - dir_fh_len =fanotify_event_dir_fh_len (event); - fh_len =fanotify_event_object_fh_len (event); -if (dir_fh_len) { +if (fanotify_event_has_dir_fh (event)) { + dir_fh_len =fanotify_event_dir_fh_len (event); event_len +=fanotify_fid_info_len (dir_fh_len, info->name_len); }else if ((info_mode &FAN_REPORT_NAME ) && (event->mask &FAN_ONDIR )) {@@ -157,8 +156,10 @@ static size_t fanotify_event_len(unsigned int info_mode,if (info_mode &FAN_REPORT_PIDFD ) event_len +=FANOTIFY_PIDFD_INFO_HDR_LEN ; -if (fh_len) +if (fanotify_event_has_object_fh (event)) { + fh_len =fanotify_event_object_fh_len (event); event_len +=fanotify_fid_info_len (fh_len, dot_len); + }return event_len; }@@ -451,7 +452,7 @@ static int copy_info_records_to_user(struct fanotify_event *event,/* * Event info records order is as follows: dir fid + name, child fid. */ -if (fanotify_event_dir_fh_len (event)) { +if (fanotify_event_has_dir_fh (event)) { info_type = info->name_len ?FAN_EVENT_INFO_TYPE_DFID_NAME :FAN_EVENT_INFO_TYPE_DFID ; ret = copy_fid_info_to_user(fanotify_event_fsid(event),@@ -467,7 +468,7 @@ static int copy_info_records_to_user(struct fanotify_event *event, total_bytes += ret; } -if (fanotify_event_object_fh_len (event)) { +if (fanotify_event_has_object_fh (event)) {const char *dot =NULL ;int dot_len =0 ;Signature · 1 line
2.33.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 25/32 fanotify: Report fid entry even for zero-length file_handle
In reply to Gabriel Krisman BertaziNon-inode errors will reported with an empty file_handle. In preparation for that, allow some events to print the FID record even if there isn't any file_handle encoded
Even though FILEID_ROOT is used internally, make zero-length file handles be reported as FILEID_INVALID.
--- fs/notify/fanotify/fanotify_user.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
fs/notify/fanotify/fanotify_user.c +5 -3
@@ -339,9 +339,6 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,pr_debug ("%s: fh_len=%zu name_len=%zu, info_len=%zu, count=%zu\n" , __func__, fh_len, name_len, info_len, count); -if (!fh_len) -return 0 ; -if (WARN_ON_ONCE (len <sizeof (info) || len > count))return -EFAULT ;
@@ -376,6 +373,11 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, handle.handle_type = fh->type; handle.handle_bytes = fh_len; + +/* Mangle handle_type for bad file_handle */ +if (!fh_len) + handle.handle_type =FILEID_INVALID ; +if (copy_to_user (buf, &handle,sizeof (handle)))return -EFAULT ;
Signature · 1 line
2.33.0
Re: [PATCH v8 25/32] fanotify: Report fid entry even for zero-length file_handle
Quoted 9 lines · Gabriel Krisman Bertazi
Non-inode errors will reported with an empty
file_handle. In preparation for that, allow some events to print the FID record even if there isn't anyfile_handleencodedEven though
FILEID_ROOTis used internally, make zero-length file handles be reported asFILEID_INVALID.Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Quoted 33 lines
--- fs/notify/fanotify/fanotify_user.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)fs/notify/fanotify/fanotify_user.c +5
-4@@ -339,9 +339,6 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,pr_debug ("%s: fh_len=%zu name_len=%zu, info_len=%zu, count=%zu\n" , __func__, fh_len, name_len, info_len, count); -if (!fh_len) -return 0 ; -if (WARN_ON_ONCE (len <sizeof (info) || len > count))return -EFAULT ;@@ -376,6 +373,11 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, handle.handle_type = fh->type; handle.handle_bytes = fh_len; + +/* Mangle handle_type for bad file_handle */ +if (!fh_len) + handle.handle_type =FILEID_INVALID ; +if (copy_to_user (buf, &handle,sizeof (handle)))return -EFAULT ; --2.33.0
Re: [PATCH v8 25/32] fanotify: Report fid entry even for zero-length file_handle
In reply to Gabriel Krisman BertaziQuoted 8 lines · Gabriel Krisman Bertazi
Non-inode errors will reported with an empty
file_handle. In preparation for that, allow some events to print the FID record even if there isn't anyfile_handleencodedEven though
FILEID_ROOTis used internally, make zero-length file handles be reported asFILEID_INVALID.Signed-off-by Gabriel Krisman Bertazi <[email protected]>
I suppose you need to move fanotify_has_object_fh() change from patch 27 here. Otherwise the change looks good so feel free to add:
Honza
Quoted 33 lines
--- fs/notify/fanotify/fanotify_user.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)fs/notify/fanotify/fanotify_user.c +5
-3@@ -339,9 +339,6 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,pr_debug ("%s: fh_len=%zu name_len=%zu, info_len=%zu, count=%zu\n" , __func__, fh_len, name_len, info_len, count); -if (!fh_len) -return 0 ; -if (WARN_ON_ONCE (len <sizeof (info) || len > count))return -EFAULT ;@@ -376,6 +373,11 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, handle.handle_type = fh->type; handle.handle_bytes = fh_len; + +/* Mangle handle_type for bad file_handle */ +if (!fh_len) + handle.handle_type =FILEID_INVALID ; +if (copy_to_user (buf, &handle,sizeof (handle)))return -EFAULT ;Signature · 1 line
2.33.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 26/32 fanotify: WARN_ON against too large file handles
In reply to Gabriel Krisman Bertazistruct fanotify_error_event, at least, is preallocated and isn't able to to handle arbitrarily large file handles. Future-proof the code by complaining loudly if a handle larger than MAX_HANDLE_SZ is ever found.
--- fs/notify/fanotify/fanotify.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
fs/notify/fanotify/fanotify.c +11 -1
@@ -360,13 +360,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,static int fanotify_encode_fh_len (struct inode *inode) {int dwords =0 ; +int fh_len;if (!inode)return 0 ;exportfs_encode_inode_fh (inode,NULL , &dwords,NULL ); + fh_len = dwords <<2 ; -return dwords <<2 ; +/* +* struct fanotify_error_event might be preallocated and is +* limited to MAX_HANDLE_SZ. This should never happen, but +* safeguard by forcing an invalid file handle. +*/ +if (WARN_ON_ONCE (fh_len >MAX_HANDLE_SZ )) +return 0 ; + +return fh_len; }/*
Signature · 1 line
2.33.0
Re: [PATCH v8 26/32] fanotify: WARN_ON against too large file handles
Quoted 6 lines · Gabriel Krisman Bertazi
struct
fanotify_error_event, at least, is preallocated and isn't able to to handle arbitrarily large file handles. Future-proof the code by complaining loudly if a handle larger thanMAX_HANDLE_SZis ever found.Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Quoted 36 lines
--- fs/notify/fanotify/fanotify.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)fs/notify/fanotify/fanotify.c +11
-2@@ -360,13 +360,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,static int fanotify_encode_fh_len (struct inode *inode) {int dwords =0 ; +int fh_len;if (!inode)return 0 ;exportfs_encode_inode_fh (inode,NULL , &dwords,NULL ); + fh_len = dwords <<2 ; -return dwords <<2 ; +/* +* struct fanotify_error_event might be preallocated and is +* limited to MAX_HANDLE_SZ. This should never happen, but +* safeguard by forcing an invalid file handle. +*/ +if (WARN_ON_ONCE (fh_len >MAX_HANDLE_SZ )) +return 0 ; + +return fh_len; }/* -- 2.33.0
Re: [PATCH v8 26/32] fanotify: WARN_ON against too large file handles
In reply to Gabriel Krisman BertaziQuoted 5 lines · Gabriel Krisman Bertazi
struct
fanotify_error_event, at least, is preallocated and isn't able to to handle arbitrarily large file handles. Future-proof the code by complaining loudly if a handle larger thanMAX_HANDLE_SZis ever found.Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Looks good. Feel free to add:
Honza
Quoted 36 lines
--- fs/notify/fanotify/fanotify.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)fs/notify/fanotify/fanotify.c +11
-1@@ -360,13 +360,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,static int fanotify_encode_fh_len (struct inode *inode) {int dwords =0 ; +int fh_len;if (!inode)return 0 ;exportfs_encode_inode_fh (inode,NULL , &dwords,NULL ); + fh_len = dwords <<2 ; -return dwords <<2 ; +/* +* struct fanotify_error_event might be preallocated and is +* limited to MAX_HANDLE_SZ. This should never happen, but +* safeguard by forcing an invalid file handle. +*/ +if (WARN_ON_ONCE (fh_len >MAX_HANDLE_SZ )) +return 0 ; + +return fh_len; }/* Signature · 1 line
2.33.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 27/32 fanotify: Report fid info for file related file system errors
In reply to Gabriel Krisman BertaziPlumb the pieces to add a FID report to error records. Since all error event memory must be pre-allocated, we pre-allocate the maximum file handle size possible, such that it should always fit.
For errors that don't expose a file handle report it with an invalid FID.
---
Changes since v7:
- Move WARN_ON to separate patch (Amir)
- Avoid duplication in the structure definition (Amir)
Changes since v6:
- pass fsid from handle_events
Changes since v5:
- Use preallocated MAX_HANDLE_SZ FH buffer
- Report superblock errors with a zerolength INVALID FID (jan, amir)
--- fs/notify/fanotify/fanotify.c | 10 ++++++++++ fs/notify/fanotify/fanotify.h | 11 +++++++++++ 2 files changed, 21 insertions(+)
fs/notify/fanotify/fanotify.c +10 -0
@@ -609,7 +609,9 @@ static struct fanotify_event *fanotify_alloc_error_event( {struct fs_error_report *report =fsnotify_data_error_report (data, data_type); +struct inode *inode = report->inode;struct fanotify_error_event *fee; +int fh_len;if (WARN_ON_ONCE (!report))return NULL ;
@@ -622,6 +624,14 @@ static struct fanotify_event *fanotify_alloc_error_event( fee->err_count =1 ; fee->fsid = *fsid; + fh_len =fanotify_encode_fh_len (inode); + +/* Bad fh_len. Fallback to using an invalid fh. Should never happen. */ +if (!fh_len && inode) + inode =NULL ; + +fanotify_encode_fh (&fee->object_fh, inode, fh_len,NULL ,0 ); + *hash ^=fanotify_hash_fsid (fsid);return &fee->fae;
fs/notify/fanotify/fanotify.h +11 -0
@@ -209,6 +209,9 @@ struct fanotify_error_event {u32 err_count;/* Suppressed errors count */ __kernel_fsid_t fsid;/* FSID this error refers to. */ + +/* This must be the last element of the structure. */ +FANOTIFY_INLINE_FH (MAX_HANDLE_SZ ); }; static inline struct fanotify_error_event *
@@ -223,6 +226,8 @@ static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event)return &FANOTIFY_FE (event)->fsid; else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME) return &FANOTIFY_NE(event)->fsid; + else if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR) + return &FANOTIFY_EE(event)->fsid; else return NULL; }
@@ -234,6 +239,8 @@ static inline struct fanotify_fh *fanotify_event_object_fh(return &FANOTIFY_FE (event)->object_fh; else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME) return fanotify_info_file_fh(&FANOTIFY_NE(event)->info); + else if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR) + return &FANOTIFY_EE(event)->object_fh; else return NULL; }
@@ -267,6 +274,10 @@ static inline int fanotify_event_dir_fh_len(struct fanotify_event *event)static inline bool fanotify_event_has_object_fh (struct fanotify_event *event) { + +/* For error events, even zeroed fh are reported. */ +if (event->type ==FANOTIFY_EVENT_TYPE_FS_ERROR ) +return true;if (fanotify_event_object_fh_len (event) >0 )return true;
Signature · 1 line
2.33.0
Re: [PATCH v8 27/32] fanotify: Report fid info for file related file system errors
Quoted 10 lines · Gabriel Krisman Bertazi
Plumb the pieces to add a FID report to error records. Since all error event memory must be pre-allocated, we pre-allocate the maximum file handle size possible, such that it should always fit.
For errors that don't expose a file handle report it with an invalid FID.
Signed-off-by Gabriel Krisman Bertazi <[email protected]>
with minor nit below..
Quoted 48 lines
---
Changes since v7:
- MoveWARN_ONto separate patch (Amir)
- Avoid duplication in the structure definition (Amir)
Changes since v6:
- pass fsid fromhandle_events
Changes since v5:
- Use preallocatedMAX_HANDLE_SZFH buffer
- Report superblock errors with a zerolength INVALID FID (jan, amir)--- fs/notify/fanotify/fanotify.c | 10 ++++++++++ fs/notify/fanotify/fanotify.h | 11 +++++++++++ 2 files changed, 21 insertions(+)fs/notify/fanotify/fanotify.c +10
-0@@ -609,7 +609,9 @@ static struct fanotify_event *fanotify_alloc_error_event( {struct fs_error_report *report =fsnotify_data_error_report (data, data_type); +struct inode *inode = report->inode;struct fanotify_error_event *fee; +int fh_len;if (WARN_ON_ONCE (!report))return NULL ;@@ -622,6 +624,14 @@ static struct fanotify_event *fanotify_alloc_error_event( fee->err_count =1 ; fee->fsid = *fsid; + fh_len =fanotify_encode_fh_len (inode); + +/* Bad fh_len. Fallback to using an invalid fh. Should never happen. */ +if (!fh_len && inode) + inode =NULL ; + +fanotify_encode_fh (&fee->object_fh, inode, fh_len,NULL ,0 ); + *hash ^=fanotify_hash_fsid (fsid);return &fee->fae;fs/notify/fanotify/fanotify.h +0
-0
fs/notify/fanotify/fanotify.h struct fanotify_error_event { · line 214
Quoted 6 lines
fs/notify/fanotify/fanotify.h +3
-0@@ -209,6 +209,9 @@ struct fanotify_error_event {u32 err_count;/* Suppressed errors count */ __kernel_fsid_t fsid;/* FSID this error refers to. */ + +/* This must be the last element of the structure. */ +FANOTIFY_INLINE_FH (MAX_HANDLE_SZ );
Does not really have to be last but certainly doesn't hurt
Thanks,
Amir.
Re: [PATCH v8 27/32] fanotify: Report fid info for file related file system errors
In reply to Gabriel Krisman BertaziQuoted 8 lines · Gabriel Krisman Bertazi
Plumb the pieces to add a FID report to error records. Since all error event memory must be pre-allocated, we pre-allocate the maximum file handle size possible, such that it should always fit.
For errors that don't expose a file handle report it with an invalid FID.
Signed-off-by Gabriel Krisman Bertazi <[email protected]>
...
Quoted 5 lines
fs/notify/fanotify/fanotify.c +0
-0
fs/notify/fanotify/fanotify.c static struct fanotify_event *fanotify_alloc_error_event( · line 617
Quoted 9 lines
fs/notify/fanotify/fanotify.c +2
-0@@ -609,7 +609,9 @@ static struct fanotify_event *fanotify_alloc_error_event( {struct fs_error_report *report =fsnotify_data_error_report (data, data_type); +struct inode *inode = report->inode;struct fanotify_error_event *fee; +int fh_len;if (WARN_ON_ONCE (!report))return NULL ;
This WARN_ON_ONCE is now pointless since you dereference report->inode above... So I guess move the dereference after WARN?
fs/notify/fanotify/fanotify.c static inline int fanotify_event_dir_fh_len(struct fanotify_event *event) · line 282
Quoted 10 lines
fs/notify/fanotify/fanotify.c +4
-0@@ -267,6 +274,10 @@ static inline int fanotify_event_dir_fh_len(struct fanotify_event *event)static inline bool fanotify_event_has_object_fh (struct fanotify_event *event) { + +/* For error events, even zeroed fh are reported. */ +if (event->type ==FANOTIFY_EVENT_TYPE_FS_ERROR ) +return true;if (fanotify_event_object_fh_len (event) >0 )return true;
This hunk belongs into patch 25. With these fixed feel free to add:
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 28/32 fanotify: Emit generic error info for error event
In reply to Gabriel Krisman BertaziThe error info is a record sent to users on FAN_FS_ERROR events documenting the type of error. It also carries an error count, documenting how many errors were observed since the last reporting.
---
Changes since v6:
- Rebase on top of pidfd patches
Changes since v5:
- Move error code here
--- fs/notify/fanotify/fanotify.c | 1 + fs/notify/fanotify/fanotify.h | 1 + fs/notify/fanotify/fanotify_user.c | 35 ++++++++++++++++++++++++++++++ include/uapi/linux/fanotify.h | 7 ++++++ 4 files changed, 44 insertions(+)
fs/notify/fanotify/fanotify.c +1 -0
@@ -621,6 +621,7 @@ static struct fanotify_event *fanotify_alloc_error_event(return NULL ; fee->fae.type =FANOTIFY_EVENT_TYPE_FS_ERROR ; + fee->error = report->error; fee->err_count =1 ; fee->fsid = *fsid;
fs/notify/fanotify/fanotify.h +1 -0
@@ -206,6 +206,7 @@ FANOTIFY_NE(struct fanotify_event *event)struct fanotify_error_event {struct fanotify_event fae; +s32 error;/* Error reported by the Filesystem. */ u32 err_count;/* Suppressed errors count */ __kernel_fsid_t fsid;/* FSID this error refers to. */
fs/notify/fanotify/fanotify_user.c +35 -0
@@ -115,6 +115,8 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly; (sizeof(struct fanotify_event_info_fid) + sizeof(struct file_handle))#define FANOTIFY_PIDFD_INFO_HDR_LEN \ sizeof(struct fanotify_event_info_pidfd) +#define FANOTIFY_ERROR_INFO_LEN \ + (sizeof(struct fanotify_event_info_error))static int fanotify_fid_info_len (int fh_len,int name_len) {
@@ -139,6 +141,9 @@ static size_t fanotify_event_len(unsigned int info_mode,if (!info_mode)return event_len; +if (fanotify_is_error_event (event->mask)) + event_len +=FANOTIFY_ERROR_INFO_LEN ; + info =fanotify_event_info (event);if (fanotify_event_has_dir_fh (event)) {
@@ -324,6 +329,28 @@ static int process_access_response(struct fsnotify_group *group,return -ENOENT ; } +static size_t copy_error_info_to_user (struct fanotify_event *event, +char __user *buf,int count) +{ +struct fanotify_event_info_error info; +struct fanotify_error_event *fee =FANOTIFY_EE (event); + + info.hdr.info_type =FAN_EVENT_INFO_TYPE_ERROR ; + info.hdr.pad =0 ; + info.hdr.len =FANOTIFY_ERROR_INFO_LEN ; + +if (WARN_ON (count < info.hdr.len)) +return -EFAULT ; + + info.error = fee->error; + info.error_count = fee->err_count; + +if (copy_to_user (buf, &info,sizeof (info))) +return -EFAULT ; + +return info.hdr.len; +} + static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, int info_type, const char *name, size_t name_len,
@@ -530,6 +557,14 @@ static int copy_info_records_to_user(struct fanotify_event *event, total_bytes += ret; } +if (fanotify_is_error_event (event->mask)) { + ret =copy_error_info_to_user (event, buf, count); +if (ret <0 ) +return ret; + buf += ret; + count -= ret; + } +return total_bytes; }
include/uapi/linux/fanotify.h +7 -0
@@ -126,6 +126,7 @@ struct fanotify_event_metadata {#define FAN_EVENT_INFO_TYPE_DFID_NAME 2#define FAN_EVENT_INFO_TYPE_DFID 3#define FAN_EVENT_INFO_TYPE_PIDFD 4 +#define FAN_EVENT_INFO_TYPE_ERROR 5/* Variable length info record following event metadata */ struct fanotify_event_info_header {
@@ -160,6 +161,12 @@ struct fanotify_event_info_pidfd {__s32 pidfd; }; +struct fanotify_event_info_error { +struct fanotify_event_info_header hdr; +__s32 error; +__u32 error_count; +}; +struct fanotify_response {__s32 fd;__u32 response;
Signature · 1 line
2.33.0
PATCH v8 29/32 fanotify: Allow users to request FAN_FS_ERROR events
In reply to Gabriel Krisman BertaziWire up the FAN_FS_ERROR event in the fanotify_mark syscall, allowing user space to request the monitoring of FAN_FS_ERROR events.
These events are limited to filesystem marks, so check it is the case in the syscall handler.
---
Changes since v7:
- Move the verification closer to similar code (Amir)
--- fs/notify/fanotify/fanotify.c | 2 +- fs/notify/fanotify/fanotify_user.c | 4 ++++ include/linux/fanotify.h | 6 +++++- 3 files changed, 10 insertions(+), 2 deletions(-)
fs/notify/fanotify/fanotify.c +1 -1
@@ -821,7 +821,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,BUILD_BUG_ON (FAN_OPEN_EXEC_PERM !=FS_OPEN_EXEC_PERM );BUILD_BUG_ON (FAN_FS_ERROR !=FS_ERROR ); -BUILD_BUG_ON (HWEIGHT32 (ALL_FANOTIFY_EVENT_BITS ) !=19 ); +BUILD_BUG_ON (HWEIGHT32 (ALL_FANOTIFY_EVENT_BITS ) !=20 ); mask =fanotify_group_event_mask (group, iter_info, mask, data, data_type, dir);
fs/notify/fanotify/fanotify_user.c +4 -0
@@ -1535,6 +1535,10 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, group->priority == FS_PRIO_0) goto fput_and_out; +if (mask &FAN_FS_ERROR && + mark_type !=FAN_MARK_FILESYSTEM ) + goto fput_and_out; +/* * Events that do not carry enough information to report * event->fd require a group that supports reporting fid. Those
include/linux/fanotify.h +5 -1
@@ -91,9 +91,13 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */#define FANOTIFY_INODE_EVENTS (FANOTIFY_DIRENT_EVENTS | \ FAN_ATTRIB | FAN_MOVE_SELF | FAN_DELETE_SELF) +/* Events that can only be reported with data type FSNOTIFY_EVENT_ERROR */ +#define FANOTIFY_ERROR_EVENTS (FAN_FS_ERROR) +/* Events that user can request to be notified on */ #define FANOTIFY_EVENTS (FANOTIFY_PATH_EVENTS | \ - FANOTIFY_INODE_EVENTS) + FANOTIFY_INODE_EVENTS | \ + FANOTIFY_ERROR_EVENTS)/* Events that require a permission response from user */ #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
Signature · 1 line
2.33.0
Re: [PATCH v8 29/32] fanotify: Allow users to request FAN_FS_ERROR events
Quoted 9 lines · Gabriel Krisman Bertazi
Wire up the
FAN_FS_ERRORevent in thefanotify_marksyscall, allowing user space to request the monitoring ofFAN_FS_ERRORevents.These events are limited to filesystem marks, so check it is the case in the syscall handler.
Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Quoted 59 lines
---
Changes since v7:
- Move the verification closer to similar code (Amir)--- fs/notify/fanotify/fanotify.c | 2 +- fs/notify/fanotify/fanotify_user.c | 4 ++++ include/linux/fanotify.h | 6 +++++- 3 files changed, 10 insertions(+), 2 deletions(-)fs/notify/fanotify/fanotify.c +1
-1@@ -821,7 +821,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,BUILD_BUG_ON (FAN_OPEN_EXEC_PERM !=FS_OPEN_EXEC_PERM );BUILD_BUG_ON (FAN_FS_ERROR !=FS_ERROR ); -BUILD_BUG_ON (HWEIGHT32 (ALL_FANOTIFY_EVENT_BITS ) !=19 ); +BUILD_BUG_ON (HWEIGHT32 (ALL_FANOTIFY_EVENT_BITS ) !=20 ); mask =fanotify_group_event_mask (group, iter_info, mask, data, data_type, dir);fs/notify/fanotify/fanotify_user.c +4
-0@@ -1535,6 +1535,10 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, group->priority == FS_PRIO_0) goto fput_and_out; +if (mask &FAN_FS_ERROR && + mark_type !=FAN_MARK_FILESYSTEM ) + goto fput_and_out; +/* * Events that do not carry enough information to report * event->fd require a group that supports reporting fid. Those include/linux/fanotify.h +5
-2@@ -91,9 +91,13 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */#define FANOTIFY_INODE_EVENTS (FANOTIFY_DIRENT_EVENTS | \ FAN_ATTRIB | FAN_MOVE_SELF | FAN_DELETE_SELF) +/* Events that can only be reported with data type FSNOTIFY_EVENT_ERROR */ +#define FANOTIFY_ERROR_EVENTS (FAN_FS_ERROR) +/* Events that user can request to be notified on */ #define FANOTIFY_EVENTS (FANOTIFY_PATH_EVENTS | \ - FANOTIFY_INODE_EVENTS) + FANOTIFY_INODE_EVENTS | \ + FANOTIFY_ERROR_EVENTS)/* Events that require a permission response from user */ #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \ --2.33.0
Re: [PATCH v8 29/32] fanotify: Allow users to request FAN_FS_ERROR events
In reply to Gabriel Krisman BertaziQuoted 7 lines · Gabriel Krisman Bertazi
Wire up the
FAN_FS_ERRORevent in thefanotify_marksyscall, allowing user space to request the monitoring ofFAN_FS_ERRORevents.These events are limited to filesystem marks, so check it is the case in the syscall handler.
Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Looks good. Feel free to add:
Honza
Quoted 60 lines
---
Changes since v7:
- Move the verification closer to similar code (Amir)--- fs/notify/fanotify/fanotify.c | 2 +- fs/notify/fanotify/fanotify_user.c | 4 ++++ include/linux/fanotify.h | 6 +++++- 3 files changed, 10 insertions(+), 2 deletions(-)fs/notify/fanotify/fanotify.c +1
-1@@ -821,7 +821,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,BUILD_BUG_ON (FAN_OPEN_EXEC_PERM !=FS_OPEN_EXEC_PERM );BUILD_BUG_ON (FAN_FS_ERROR !=FS_ERROR ); -BUILD_BUG_ON (HWEIGHT32 (ALL_FANOTIFY_EVENT_BITS ) !=19 ); +BUILD_BUG_ON (HWEIGHT32 (ALL_FANOTIFY_EVENT_BITS ) !=20 ); mask =fanotify_group_event_mask (group, iter_info, mask, data, data_type, dir);fs/notify/fanotify/fanotify_user.c +4
-0@@ -1535,6 +1535,10 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, group->priority == FS_PRIO_0) goto fput_and_out; +if (mask &FAN_FS_ERROR && + mark_type !=FAN_MARK_FILESYSTEM ) + goto fput_and_out; +/* * Events that do not carry enough information to report * event->fd require a group that supports reporting fid. Those include/linux/fanotify.h +5
-1@@ -91,9 +91,13 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */#define FANOTIFY_INODE_EVENTS (FANOTIFY_DIRENT_EVENTS | \ FAN_ATTRIB | FAN_MOVE_SELF | FAN_DELETE_SELF) +/* Events that can only be reported with data type FSNOTIFY_EVENT_ERROR */ +#define FANOTIFY_ERROR_EVENTS (FAN_FS_ERROR) +/* Events that user can request to be notified on */ #define FANOTIFY_EVENTS (FANOTIFY_PATH_EVENTS | \ - FANOTIFY_INODE_EVENTS) + FANOTIFY_INODE_EVENTS | \ + FANOTIFY_ERROR_EVENTS)/* Events that require a permission response from user */ #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \Signature · 1 line
2.33.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 30/32 ext4: Send notifications on error
In reply to Gabriel Krisman BertaziSend a FS_ERROR message via fsnotify to a userspace monitoring tool whenever a ext4 error condition is triggered. This follows the existing error conditions in ext4, so it is hooked to the ext4_error* functions.
It also follows the current dmesg reporting in the format. The filesystem message is composed mostly by the string that would be otherwise printed in dmesg.
A new ext4 specific record format is exposed in the uapi, such that a monitoring tool knows what to expect when listening errors of an ext4 filesystem.
---
Changes since v6:
- Report ext4_std_errors agains superblock (jan)
--- fs/ext4/super.c | 8 ++++++++ 1 file changed, 8 insertions(+)
fs/ext4/super.c +8 -0
@@ -46,6 +46,7 @@#include <linux/part_stat.h> #include <linux/kthread.h> #include <linux/freezer.h> +#include <linux/fsnotify.h> #include "ext4.h" #include "ext4_extents.h" /* Needed for trace points definition */
@@ -759,6 +760,8 @@ void __ext4_error(struct super_block *sb, const char *function, sb->s_id, function, line, current->comm, &vaf);va_end (args); } +fsnotify_sb_error (sb,NULL , error); +ext4_handle_error (sb, force_ro, error,0 , block, function, line); }
@@ -789,6 +792,8 @@ void __ext4_error_inode(struct inode *inode, const char *function, current->comm, &vaf);va_end (args); } +fsnotify_sb_error (inode->i_sb, inode, error); +ext4_handle_error (inode->i_sb, false, error, inode->i_ino, block, function, line); }
@@ -827,6 +832,8 @@ void __ext4_error_file(struct file *file, const char *function, current->comm, path, &vaf);va_end (args); } +fsnotify_sb_error (inode->i_sb, inode,EFSCORRUPTED ); +ext4_handle_error (inode->i_sb, false,EFSCORRUPTED , inode->i_ino, block, function, line); }
@@ -894,6 +901,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,printk (KERN_CRIT "EXT4-fs error (device %s) in %s:%d: %s\n" , sb->s_id, function, line, errstr); } +fsnotify_sb_error (sb,NULL , errno);ext4_handle_error (sb, false, -errno,0 ,0 , function, line); }
Signature · 1 line
2.33.0
Re: [PATCH v8 30/32] ext4: Send notifications on error
Quoted 8 lines · Gabriel Krisman Bertazi
Send a
FS_ERRORmessage via fsnotify to a userspace monitoring tool whenever a ext4 error condition is triggered. This follows the existing error conditions in ext4, so it is hooked to the ext4_error* functions.It also follows the current dmesg reporting in the format. The filesystem message is composed mostly by the string that would be otherwise printed in dmesg.
Quoted 7 lines
A new ext4 specific record format is exposed in the uapi, such that a monitoring tool knows what to expect when listening errors of an ext4 filesystem.
Reviewed-by Amir Goldstein <[email protected]>Reviewed-by Theodore Ts'o <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Looks good to me. Feel free to add:
Honza
Quoted 58 lines
---
Changes since v6:
- Reportext4_std_errorsagains superblock (jan)--- fs/ext4/super.c | 8 ++++++++ 1 file changed, 8 insertions(+)fs/ext4/super.c +8
-0@@ -46,6 +46,7 @@#include <linux/part_stat.h> #include <linux/kthread.h> #include <linux/freezer.h> +#include <linux/fsnotify.h> #include "ext4.h" #include "ext4_extents.h" /* Needed for trace points definition */ @@ -759,6 +760,8 @@ void __ext4_error(struct super_block *sb, const char *function, sb->s_id, function, line, current->comm, &vaf);va_end (args); } +fsnotify_sb_error (sb,NULL , error); +ext4_handle_error (sb, force_ro, error,0 , block, function, line); }@@ -789,6 +792,8 @@ void __ext4_error_inode(struct inode *inode, const char *function, current->comm, &vaf);va_end (args); } +fsnotify_sb_error (inode->i_sb, inode, error); +ext4_handle_error (inode->i_sb, false, error, inode->i_ino, block, function, line); }@@ -827,6 +832,8 @@ void __ext4_error_file(struct file *file, const char *function, current->comm, path, &vaf);va_end (args); } +fsnotify_sb_error (inode->i_sb, inode,EFSCORRUPTED ); +ext4_handle_error (inode->i_sb, false,EFSCORRUPTED , inode->i_ino, block, function, line); }@@ -894,6 +901,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,printk (KERN_CRIT "EXT4-fs error (device %s) in %s:%d: %s\n" , sb->s_id, function, line, errstr); } +fsnotify_sb_error (sb,NULL , errno);ext4_handle_error (sb, false, -errno,0 ,0 , function, line); }Signature · 1 line
2.33.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Jan Kara
maintainerRe: [PATCH v8 30/32] ext4: Send notifications on error
Quoted 13 lines · Jan Kara
Quoted 12 lines · Gabriel Krisman Bertazi
Send a
FS_ERRORmessage via fsnotify to a userspace monitoring tool whenever a ext4 error condition is triggered. This follows the existing error conditions in ext4, so it is hooked to the ext4_error* functions.It also follows the current dmesg reporting in the format. The filesystem message is composed mostly by the string that would be otherwise printed in dmesg.
A new ext4 specific record format is exposed in the uapi, such that a monitoring tool knows what to expect when listening errors of an ext4 filesystem.
Quoted 7 lines
Quoted 3 lines
Reviewed-by Amir Goldstein <[email protected]>Reviewed-by Theodore Ts'o <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>Looks good to me. Feel free to add:
Reviewed-by Jan Kara <[email protected]>
Hum, I actually retract this because the code doesn't match what is written in the documentation and I'm not 100% sure what is correct. In particular:
Quoted 6 lines
fs/ext4/super.c void __ext4_error(struct super_block *sb, const char *function, · line 764
Quoted 6 lines
fs/ext4/super.c +2
-0@@ -759,6 +760,8 @@ void __ext4_error(struct super_block *sb, const char *function, sb->s_id, function, line, current->comm, &vaf);va_end (args); } +fsnotify_sb_error (sb,NULL , error); +
E.g. here you pass the 'error' to fsnotify. This will be just standard 'errno' number, not ext4 error code as described in the documentation. Also note that frequently 'error' will be 0 which gets magically transformed to EFSCORRUPTED in save_error_info() in the ext4 error handling below. So there's clearly some more work to do...
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH v8 30/32] ext4: Send notifications on error
Quoted 25 lines · Jan Kara
Quoted 13 lines · Jan Kara
Quoted 12 lines · Gabriel Krisman Bertazi
Send a
FS_ERRORmessage via fsnotify to a userspace monitoring tool whenever a ext4 error condition is triggered. This follows the existing error conditions in ext4, so it is hooked to the ext4_error* functions.It also follows the current dmesg reporting in the format. The filesystem message is composed mostly by the string that would be otherwise printed in dmesg.
A new ext4 specific record format is exposed in the uapi, such that a monitoring tool knows what to expect when listening errors of an ext4 filesystem.
Quoted 7 lines
Quoted 3 lines
Reviewed-by Amir Goldstein <[email protected]>Reviewed-by Theodore Ts'o <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>Looks good to me. Feel free to add:
Reviewed-by Jan Kara <[email protected]>Hum, I actually retract this because the code doesn't match what is written in the documentation and I'm not 100% sure what is correct. In particular:
Quoted 12 lines
Quoted 6 lines
fs/ext4/super.c void __ext4_error(struct super_block *sb, const char *function, · line 764
Quoted 6 lines
fs/ext4/super.c +2
-0@@ -759,6 +760,8 @@ void __ext4_error(struct super_block *sb, const char *function, sb->s_id, function, line, current->comm, &vaf);va_end (args); } +fsnotify_sb_error (sb,NULL , error); +E.g. here you pass the 'error' to fsnotify. This will be just standard 'errno' number, not ext4 error code as described in the documentation. Also note that frequently 'error' will be 0 which gets magically transformed to EFSCORRUPTED in
save_error_info()in the ext4 error handling below. So there's clearly some more work to do...
Nice catch.
The many 0 returns were discussed before, around v3. You can notice one of my LTP tests is designed to catch that. We agreed ext4 shouldn't be returning 0, and that we would write a patch to fix it, but I didn't think it belonged as part of this series.
You are also right about the EXT4_ vs. errno. the documentation is buggy, since it was brought from the fs-specific descriptor days, which no longer exists. Nevertheless, I think there is a case for always returning file system specific errors here, since they are more descriptive.
Should we agree to follow the documentation and return FS specific errors instead of errno, then?
Either way, I'm dropping all r-by flags here.
Signature · 1 line
Gabriel Krisman Bertazi
Theodore Ts'o
maintainerRe: [PATCH v8 30/32] ext4: Send notifications on error
Quoted 11 lines · Gabriel Krisman Bertazi
Quoted 6 lines
E.g. here you pass the 'error' to fsnotify. This will be just standard 'errno' number, not ext4 error code as described in the documentation. Also note that frequently 'error' will be 0 which gets magically transformed to EFSCORRUPTED in
save_error_info()in the ext4 error handling below. So there's clearly some more work to do...The many 0 returns were discussed before, around v3. You can notice one of my LTP tests is designed to catch that. We agreed ext4 shouldn't be returning 0, and that we would write a patch to fix it, but I didn't think it belonged as part of this series.
The fact that ext4 passes 0 into __ext4_error() to mean EFSCORRUPTED is an internal implementation detail, and as currently implemented it is *not* a bug. It was just a convenience to minimize the number of call sites that needed to be modified when we added the feature of storing the error code to be stored in the superblock.
So I think this is something that should be addressed in this patchset, and it's pretty simple to do so. It's just a matter of doing something like this:
fsnotify_sb_error(sb, NULL, error ? error : EFSCORRUPTED);
Quoted 5 lines
You are also right about the
EXT4_vs. errno. the documentation is buggy, since it was brought from the fs-specific descriptor days, which no longer exists. Nevertheless, I think there is a case for always returning file system specific errors here, since they are more descriptive.
So the history is that ext4 specific errors were used because we were storing them in the superblock --- and so we need an architecture independent way of storing the error codes. (Errno codes are not stable across architectures; and consider what might happen if we had error codes written on an say, an ARM platform, and then that disk is attached to an Alpha, S390, or Power system?)
Quoted 2 lines
Should we agree to follow the documentation and return FS specific errors instead of errno, then?
I disagree. We should use errno's, for a couple of reasons. First of all, users of fsnotify shouldn't need to know which file system to interpret the error codes.
Secondly, the reason why ext4 has file system specific error cdoes is because those codes are written into the superblock, and errno's are not stable across different architectures. So for ext4, we needed to worry what might happen if the error code was written while the file system was mounted on say, an ARM-64 system, and then storage device might get attached to a S390, Alpha, or PA-RISC system. This is not a problem that the fsnotify API needs to worry about.
Finally, the error codes that we used for the ext4 superblock are *not* more descriptive than errno's --- we only have 16 ext4-specific error codes, and there are far more errno values.
Cheers,
- Ted
PATCH v8 31/32 samples: Add fs error monitoring example
In reply to Gabriel Krisman BertaziIntroduce an example of a FAN_FS_ERROR fanotify user to track filesystem errors.
---
Changes since v4:
- Protect file_handle defines with ifdef guards
Changes since v1:
- minor fixes
--- samples/Kconfig | 9 +++ samples/Makefile | 1 + samples/fanotify/Makefile | 5 ++ samples/fanotify/fs-monitor.c | 142 ++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+) create mode 100644 samples/fanotify/Makefile create mode 100644 samples/fanotify/fs-monitor.c
samples/Kconfig +9 -0
@@ -120,6 +120,15 @@ config SAMPLE_CONNECTOR with it. See also Documentation/driver-api/connector.rst +config SAMPLE_FANOTIFY_ERROR + bool "Build fanotify error monitoring sample" + depends on FANOTIFY + help + When enabled, this builds an example code that uses the + FAN_FS_ERROR fanotify mechanism to monitor filesystem + errors. + See also Documentation/admin-guide/filesystem-monitoring.rst. + config SAMPLE_HIDRAW bool "hidraw sample" depends on CC_CAN_LINK && HEADERS_INSTALL
samples/Makefile +1 -0
@@ -5,6 +5,7 @@ subdir-$(CONFIG_SAMPLE_AUXDISPLAY) += auxdisplay subdir-$(CONFIG_SAMPLE_ANDROID_BINDERFS) += binderfs obj-$(CONFIG_SAMPLE_CONFIGFS) += configfs/ obj-$(CONFIG_SAMPLE_CONNECTOR) += connector/ +obj-$(CONFIG_SAMPLE_FANOTIFY_ERROR) += fanotify/ subdir-$(CONFIG_SAMPLE_HIDRAW) += hidraw obj-$(CONFIG_SAMPLE_HW_BREAKPOINT) += hw_breakpoint/ obj-$(CONFIG_SAMPLE_KDB) += kdb/
samples/fanotify/Makefile new file +5 -0
@@ -0,0 +1,5 @@ +# SPDX-License-Identifier: GPL-2.0-only +userprogs-always-y += fs-monitor + +userccflags += -I usr/include -Wall +
samples/fanotify/fs-monitor.c new file +142 -0
142 lines not on this page · raw message
Signature · 1 line
2.33.0
Re: [PATCH v8 31/32] samples: Add fs error monitoring example
Quoted 5 lines · Gabriel Krisman Bertazi
Introduce an example of a
FAN_FS_ERRORfanotify user to track filesystem errors.Reviewed-by Amir Goldstein <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Looks good. Feel free to add:
Honza
Quoted 209 lines
---
Changes since v4:
- Protectfile_handledefines with ifdef guardsChanges since v1:
- minor fixes--- samples/Kconfig | 9 +++ samples/Makefile | 1 + samples/fanotify/Makefile | 5 ++ samples/fanotify/fs-monitor.c | 142 ++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+) create mode 100644 samples/fanotify/Makefile create mode 100644 samples/fanotify/fs-monitor.csamples/Kconfig +9
-0@@ -120,6 +120,15 @@ config SAMPLE_CONNECTOR with it. See also Documentation/driver-api/connector.rst +config SAMPLE_FANOTIFY_ERROR + bool "Build fanotify error monitoring sample" + depends on FANOTIFY + help + When enabled, this builds an example code that uses the + FAN_FS_ERROR fanotify mechanism to monitor filesystem + errors. + See also Documentation/admin-guide/filesystem-monitoring.rst. + config SAMPLE_HIDRAW bool "hidraw sample" depends on CC_CAN_LINK && HEADERS_INSTALLsamples/Makefile +1
-0@@ -5,6 +5,7 @@ subdir-$(CONFIG_SAMPLE_AUXDISPLAY) += auxdisplay subdir-$(CONFIG_SAMPLE_ANDROID_BINDERFS) += binderfs obj-$(CONFIG_SAMPLE_CONFIGFS) += configfs/ obj-$(CONFIG_SAMPLE_CONNECTOR) += connector/ +obj-$(CONFIG_SAMPLE_FANOTIFY_ERROR) += fanotify/ subdir-$(CONFIG_SAMPLE_HIDRAW) += hidraw obj-$(CONFIG_SAMPLE_HW_BREAKPOINT) += hw_breakpoint/ obj-$(CONFIG_SAMPLE_KDB) += kdb/samples/fanotify/Makefile new file +5
-0@@ -0,0 +1,5 @@ +# SPDX-License-Identifier: GPL-2.0-only +userprogs-always-y += fs-monitor + +userccflags += -I usr/include -Wall +samples/fanotify/fs-monitor.c new file +142
-0@@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright 2021, Collabora Ltd. +*/ + +#define _GNU_SOURCE +#include <errno.h> +#include <err.h> +#include <stdlib.h> +#include <stdio.h> +#include <fcntl.h> +#include <sys/fanotify.h> +#include <sys/types.h> +#include <unistd.h> +#include <sys/types.h> + +#ifndef FAN_FS_ERROR +#define FAN_FS_ERROR 0x00008000 +#define FAN_EVENT_INFO_TYPE_ERROR 5 + +struct fanotify_event_info_error { +struct fanotify_event_info_header hdr; +__s32 error; +__u32 error_count; +}; +#endif + +#ifndef FILEID_INO32_GEN +#define FILEID_INO32_GEN 1 +#endif + +#ifndef FILEID_INVALID +#define FILEID_INVALID 0xff +#endif + +static void print_fh (struct file_handle *fh) +{ +int i; +uint32_t *h = (uint32_t *) fh->f_handle; + +printf ("\tfh: " ); +for (i =0 ; i < fh->handle_bytes; i++) +printf ("%hhx" , fh->f_handle[i]); +printf ("\n" ); + +printf ("\tdecoded fh: " ); +if (fh->handle_type ==FILEID_INO32_GEN ) +printf ("inode=%u gen=%u\n" , h[0 ], h[1 ]); +else if (fh->handle_type ==FILEID_INVALID && !fh->handle_bytes) +printf ("Type %d (Superblock error)\n" , fh->handle_type); +else +printf ("Type %d (Unknown)\n" , fh->handle_type); + +} + +static void handle_notifications (char *buffer,int len) +{ +struct fanotify_event_metadata *event = + (struct fanotify_event_metadata *) buffer; +struct fanotify_event_info_header *info; +struct fanotify_event_info_error *err; +struct fanotify_event_info_fid *fid; +int off; + +for (;FAN_EVENT_OK (event, len); event =FAN_EVENT_NEXT (event, len)) { + +if (event->mask !=FAN_FS_ERROR ) { +printf ("unexpected FAN MARK: %llx\n" , event->mask); + goto next_event; + } + +if (event->fd !=FAN_NOFD ) { +printf ("Unexpected fd (!= FAN_NOFD)\n" ); + goto next_event; + } + +printf ("FAN_FS_ERROR (len=%d)\n" , event->event_len); + +for (off =sizeof (*event) ; off < event->event_len; + off += info->len) { + info = (struct fanotify_event_info_header *) + ((char *) event + off); + +switch (info->info_type) { +case FAN_EVENT_INFO_TYPE_ERROR : + err = (struct fanotify_event_info_error *) info; + +printf ("\tGeneric Error Record: len=%d\n" , + err->hdr.len); +printf ("\terror: %d\n" , err->error); +printf ("\terror_count: %d\n" , err->error_count); +break ; + +case FAN_EVENT_INFO_TYPE_FID : + fid = (struct fanotify_event_info_fid *) info; + +printf ("\tfsid: %x%x\n" , + fid->fsid.val[0 ], fid->fsid.val[1 ]); +print_fh ((struct file_handle *) &fid->handle); +break ; + +default : +printf ("\tUnknown info type=%d len=%d:\n" , + info->info_type, info->len); + } + } +next_event: +printf ("---\n\n" ); + } +} + +int main (int argc,char **argv) +{ +int fd; + +char buffer[BUFSIZ ]; + +if (argc <2 ) { +printf ("Missing path argument\n" ); +return 1 ; + } + + fd =fanotify_init (FAN_CLASS_NOTIF |FAN_REPORT_FID ,O_RDONLY ); +if (fd <0 ) +errx (1 ,"fanotify_init" ); + +if (fanotify_mark (fd,FAN_MARK_ADD |FAN_MARK_FILESYSTEM , +FAN_FS_ERROR ,AT_FDCWD , argv[1 ])) { +errx (1 ,"fanotify_mark" ); + } + +while (1 ) { +int n =read (fd, buffer,BUFSIZ ); + +if (n <0 ) +errx (1 ,"read" ); + +handle_notifications (buffer, n); + } + +return 0 ; +}Signature · 1 line
2.33.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH v8 31/32] samples: Add fs error monitoring example
In reply to Gabriel Krisman BertaziQuoted 26 lines · Gabriel Krisman Bertazi
Introduce an example of a
FAN_FS_ERRORfanotify user to track filesystem errors.Reviewed-by Amir Goldstein <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>Reviewed-by Jan Kara <[email protected]>---
Changes since v4:
- Protectfile_handledefines with ifdef guardsChanges since v1:
- minor fixes--- samples/Kconfig | 9 +++ samples/Makefile | 1 + samples/fanotify/Makefile | 5 ++ samples/fanotify/fs-monitor.c | 142 ++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+) create mode 100644 samples/fanotify/Makefile create mode 100644 samples/fanotify/fs-monitor.csamples/Kconfig +0
-0
samples/Kconfig config SAMPLE_CONNECTOR · line 125
Quoted 6 lines
samples/Kconfig +3
-0@@ -120,6 +120,15 @@ config SAMPLE_CONNECTOR with it. See also Documentation/driver-api/connector.rst +config SAMPLE_FANOTIFY_ERROR + bool "Build fanotify error monitoring sample" + depends on FANOTIFY
This needs something like
depends on CC_CAN_LINK
or possibly even
depends on CC_CAN_LINK && HEADERS_INSTALL
to avoid compilation errors such as
samples/fanotify/fs-monitor.c:7:10: fatal error: errno.h: No such file or directory
7 | #include <errno.h>
| ^~~~~~~~~compilation terminated.
when using a toolchain without C library support, such as those provided on kernel.org.
Guenter
Quoted 180 lines
samples/Kconfig +6
-0+ help + When enabled, this builds an example code that uses the + FAN_FS_ERROR fanotify mechanism to monitor filesystem + errors. + See also Documentation/admin-guide/filesystem-monitoring.rst. + config SAMPLE_HIDRAW bool "hidraw sample" depends on CC_CAN_LINK && HEADERS_INSTALLsamples/Makefile +1
-0@@ -5,6 +5,7 @@ subdir-$(CONFIG_SAMPLE_AUXDISPLAY) += auxdisplay subdir-$(CONFIG_SAMPLE_ANDROID_BINDERFS) += binderfs obj-$(CONFIG_SAMPLE_CONFIGFS) += configfs/ obj-$(CONFIG_SAMPLE_CONNECTOR) += connector/ +obj-$(CONFIG_SAMPLE_FANOTIFY_ERROR) += fanotify/ subdir-$(CONFIG_SAMPLE_HIDRAW) += hidraw obj-$(CONFIG_SAMPLE_HW_BREAKPOINT) += hw_breakpoint/ obj-$(CONFIG_SAMPLE_KDB) += kdb/samples/fanotify/Makefile new file +5
-0@@ -0,0 +1,5 @@ +# SPDX-License-Identifier: GPL-2.0-only +userprogs-always-y += fs-monitor + +userccflags += -I usr/include -Wall +samples/fanotify/fs-monitor.c new file +142
-0@@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright 2021, Collabora Ltd. +*/ + +#define _GNU_SOURCE +#include <errno.h> +#include <err.h> +#include <stdlib.h> +#include <stdio.h> +#include <fcntl.h> +#include <sys/fanotify.h> +#include <sys/types.h> +#include <unistd.h> +#include <sys/types.h> + +#ifndef FAN_FS_ERROR +#define FAN_FS_ERROR 0x00008000 +#define FAN_EVENT_INFO_TYPE_ERROR 5 + +struct fanotify_event_info_error { +struct fanotify_event_info_header hdr; +__s32 error; +__u32 error_count; +}; +#endif + +#ifndef FILEID_INO32_GEN +#define FILEID_INO32_GEN 1 +#endif + +#ifndef FILEID_INVALID +#define FILEID_INVALID 0xff +#endif + +static void print_fh (struct file_handle *fh) +{ +int i; +uint32_t *h = (uint32_t *) fh->f_handle; + +printf ("\tfh: " ); +for (i =0 ; i < fh->handle_bytes; i++) +printf ("%hhx" , fh->f_handle[i]); +printf ("\n" ); + +printf ("\tdecoded fh: " ); +if (fh->handle_type ==FILEID_INO32_GEN ) +printf ("inode=%u gen=%u\n" , h[0 ], h[1 ]); +else if (fh->handle_type ==FILEID_INVALID && !fh->handle_bytes) +printf ("Type %d (Superblock error)\n" , fh->handle_type); +else +printf ("Type %d (Unknown)\n" , fh->handle_type); + +} + +static void handle_notifications (char *buffer,int len) +{ +struct fanotify_event_metadata *event = + (struct fanotify_event_metadata *) buffer; +struct fanotify_event_info_header *info; +struct fanotify_event_info_error *err; +struct fanotify_event_info_fid *fid; +int off; + +for (;FAN_EVENT_OK (event, len); event =FAN_EVENT_NEXT (event, len)) { + +if (event->mask !=FAN_FS_ERROR ) { +printf ("unexpected FAN MARK: %llx\n" , event->mask); + goto next_event; + } + +if (event->fd !=FAN_NOFD ) { +printf ("Unexpected fd (!= FAN_NOFD)\n" ); + goto next_event; + } + +printf ("FAN_FS_ERROR (len=%d)\n" , event->event_len); + +for (off =sizeof (*event) ; off < event->event_len; + off += info->len) { + info = (struct fanotify_event_info_header *) + ((char *) event + off); + +switch (info->info_type) { +case FAN_EVENT_INFO_TYPE_ERROR : + err = (struct fanotify_event_info_error *) info; + +printf ("\tGeneric Error Record: len=%d\n" , + err->hdr.len); +printf ("\terror: %d\n" , err->error); +printf ("\terror_count: %d\n" , err->error_count); +break ; + +case FAN_EVENT_INFO_TYPE_FID : + fid = (struct fanotify_event_info_fid *) info; + +printf ("\tfsid: %x%x\n" , + fid->fsid.val[0 ], fid->fsid.val[1 ]); +print_fh ((struct file_handle *) &fid->handle); +break ; + +default : +printf ("\tUnknown info type=%d len=%d:\n" , + info->info_type, info->len); + } + } +next_event: +printf ("---\n\n" ); + } +} + +int main (int argc,char **argv) +{ +int fd; + +char buffer[BUFSIZ ]; + +if (argc <2 ) { +printf ("Missing path argument\n" ); +return 1 ; + } + + fd =fanotify_init (FAN_CLASS_NOTIF |FAN_REPORT_FID ,O_RDONLY ); +if (fd <0 ) +errx (1 ,"fanotify_init" ); + +if (fanotify_mark (fd,FAN_MARK_ADD |FAN_MARK_FILESYSTEM , +FAN_FS_ERROR ,AT_FDCWD , argv[1 ])) { +errx (1 ,"fanotify_mark" ); + } + +while (1 ) { +int n =read (fd, buffer,BUFSIZ ); + +if (n <0 ) +errx (1 ,"read" ); + +handle_notifications (buffer, n); + } + +return 0 ; +}
Re: [PATCH v8 31/32] samples: Add fs error monitoring example
Quoted 40 lines · Guenter Roeck
Quoted 26 lines · Gabriel Krisman Bertazi
Introduce an example of a
FAN_FS_ERRORfanotify user to track filesystem errors.Reviewed-by Amir Goldstein <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>Reviewed-by Jan Kara <[email protected]>---
Changes since v4:
- Protectfile_handledefines with ifdef guardsChanges since v1:
- minor fixes--- samples/Kconfig | 9 +++ samples/Makefile | 1 + samples/fanotify/Makefile | 5 ++ samples/fanotify/fs-monitor.c | 142 ++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+) create mode 100644 samples/fanotify/Makefile create mode 100644 samples/fanotify/fs-monitor.csamples/Kconfig +0
-0samples/Kconfig config SAMPLE_CONNECTOR · line 125
Quoted 6 lines
samples/Kconfig +3
-0@@ -120,6 +120,15 @@ config SAMPLE_CONNECTOR with it. See also Documentation/driver-api/connector.rst +config SAMPLE_FANOTIFY_ERROR + bool "Build fanotify error monitoring sample" + depends on FANOTIFYThis needs something like
depends onCC_CAN_LINK
or possibly evendepends on CC_CAN_LINK && HEADERS_INSTALLto avoid compilation errors such as
Quoted 7 lines
samples/fanotify/fs-monitor.c:7:10: fatal error:errno.h: No such file or directory7 | #include <errno.h> | ^~~~~~~~~compilation terminated.
when using a toolchain without C library support, such as those provided on kernel.org.
Thank you, Guenter.
We discussed this, but I wasn't sure how to silence the error and it didn't trigger in the past versions.
The original patch is already in Jan's tree. Jan, would you pick the pack below to address it? Feel free to squash it into the original commit, if you think it is saner..
Thanks,
-- >8 --
From: Gabriel Krisman Bertazi <[email protected]>
Date: Thu, 28 Oct 2021 15:34:46 -0300
Subject: [PATCH] samples: Make fs-monitor depend on libc and headers
Prevent build errors when headers or libc are not available, such as on kernel build bots, like the below:
samples/fanotify/fs-monitor.c:7:10: fatal error: errno.h: No such file or directory
7 | #include <errno.h>
| ^~~~~~~~~
--- samples/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
samples/Kconfig +1 -1
@@ -122,7 +122,7 @@ config SAMPLE_CONNECTOR config SAMPLE_FANOTIFY_ERROR bool "Build fanotify error monitoring sample" - depends on FANOTIFY + depends on FANOTIFY && CC_CAN_LINK && HEADERS_INSTALL help When enabled, this builds an example code that uses the FAN_FS_ERROR fanotify mechanism to monitor filesystem
Signature · 1 line
2.33.0
Guenter Roeck
Tested-byRe: [PATCH v8 31/32] samples: Add fs error monitoring example
Quoted 69 lines · Gabriel Krisman Bertazi
Quoted 40 lines · Guenter Roeck
Quoted 26 lines · Gabriel Krisman Bertazi
Introduce an example of a
FAN_FS_ERRORfanotify user to track filesystem errors.Reviewed-by Amir Goldstein <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>Reviewed-by Jan Kara <[email protected]>---
Changes since v4:
- Protectfile_handledefines with ifdef guardsChanges since v1:
- minor fixes--- samples/Kconfig | 9 +++ samples/Makefile | 1 + samples/fanotify/Makefile | 5 ++ samples/fanotify/fs-monitor.c | 142 ++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+) create mode 100644 samples/fanotify/Makefile create mode 100644 samples/fanotify/fs-monitor.csamples/Kconfig +0
-0samples/Kconfig config SAMPLE_CONNECTOR · line 125
Quoted 6 lines
samples/Kconfig +3
-0@@ -120,6 +120,15 @@ config SAMPLE_CONNECTOR with it. See also Documentation/driver-api/connector.rst +config SAMPLE_FANOTIFY_ERROR + bool "Build fanotify error monitoring sample" + depends on FANOTIFYThis needs something like
depends onCC_CAN_LINK
or possibly evendepends on CC_CAN_LINK && HEADERS_INSTALLto avoid compilation errors such as
Quoted 7 lines
samples/fanotify/fs-monitor.c:7:10: fatal error:errno.h: No such file or directory7 | #include <errno.h> | ^~~~~~~~~compilation terminated.
when using a toolchain without C library support, such as those provided on kernel.org.
Thank you, Guenter.
We discussed this, but I wasn't sure how to silence the error and it didn't trigger in the past versions.
The original patch is already in Jan's tree. Jan, would you pick the pack below to address it? Feel free to squash it into the original commit, if you think it is saner..
Thanks,
-- >8 --
From: Gabriel Krisman Bertazi <[email protected]>
Date: Thu, 28 Oct 2021 15:34:46 -0300
Subject: [PATCH] samples: Make fs-monitor depend on libc and headersPrevent build errors when headers or libc are not available, such as on kernel build bots, like the below:
Quoted 7 lines
samples/fanotify/fs-monitor.c:7:10: fatal error:errno.h: No such file or directory
7 | #include <errno.h>| ^~~~~~~~~Suggested-by Guenter Roeck <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>
Quoted 19 lines
--- samples/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)samples/Kconfig +1
-1@@ -122,7 +122,7 @@ config SAMPLE_CONNECTOR config SAMPLE_FANOTIFY_ERROR bool "Build fanotify error monitoring sample" - depends on FANOTIFY + depends on FANOTIFY && CC_CAN_LINK && HEADERS_INSTALL help When enabled, this builds an example code that uses the FAN_FS_ERROR fanotify mechanism to monitor filesystemSignature · 1 line
2.33.0
Jan Kara
maintainerRe: [PATCH v8 31/32] samples: Add fs error monitoring example
In reply to Gabriel Krisman BertaziQuoted 50 lines · Gabriel Krisman Bertazi
Quoted 40 lines · Guenter Roeck
Quoted 26 lines · Gabriel Krisman Bertazi
Introduce an example of a
FAN_FS_ERRORfanotify user to track filesystem errors.Reviewed-by Amir Goldstein <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>Reviewed-by Jan Kara <[email protected]>---
Changes since v4:
- Protectfile_handledefines with ifdef guardsChanges since v1:
- minor fixes--- samples/Kconfig | 9 +++ samples/Makefile | 1 + samples/fanotify/Makefile | 5 ++ samples/fanotify/fs-monitor.c | 142 ++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+) create mode 100644 samples/fanotify/Makefile create mode 100644 samples/fanotify/fs-monitor.csamples/Kconfig +0
-0samples/Kconfig config SAMPLE_CONNECTOR · line 125
Quoted 6 lines
samples/Kconfig +3
-0@@ -120,6 +120,15 @@ config SAMPLE_CONNECTOR with it. See also Documentation/driver-api/connector.rst +config SAMPLE_FANOTIFY_ERROR + bool "Build fanotify error monitoring sample" + depends on FANOTIFYThis needs something like
depends onCC_CAN_LINK
or possibly evendepends on CC_CAN_LINK && HEADERS_INSTALLto avoid compilation errors such as
Quoted 7 lines
samples/fanotify/fs-monitor.c:7:10: fatal error:errno.h: No such file or directory7 | #include <errno.h> | ^~~~~~~~~compilation terminated.
when using a toolchain without C library support, such as those provided on kernel.org.
Quoted 8 lines
Thank you, Guenter.
We discussed this, but I wasn't sure how to silence the error and it didn't trigger in the past versions.
The original patch is already in Jan's tree. Jan, would you pick the pack below to address it? Feel free to squash it into the original commit, if you think it is saner..
Thanks guys, I've added the patch to my tree. If we had more time, I'd probably squash it but given I'd like to send Linus a pull request at the end of the week I don't want to touch commits that are already in next.
Honza
Quoted 35 lines
-- >8 --
From: Gabriel Krisman Bertazi <[email protected]>
Date: Thu, 28 Oct 2021 15:34:46 -0300
Subject: [PATCH] samples: Make fs-monitor depend on libc and headersPrevent build errors when headers or libc are not available, such as on kernel build bots, like the below:
samples/fanotify/fs-monitor.c:7:10: fatal error:errno.h: No such file or directory
7 | #include <errno.h>| ^~~~~~~~~Suggested-by Guenter Roeck <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>--- samples/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)samples/Kconfig +1
-1@@ -122,7 +122,7 @@ config SAMPLE_CONNECTOR config SAMPLE_FANOTIFY_ERROR bool "Build fanotify error monitoring sample" - depends on FANOTIFY + depends on FANOTIFY && CC_CAN_LINK && HEADERS_INSTALL help When enabled, this builds an example code that uses the FAN_FS_ERROR fanotify mechanism to monitor filesystemSignature · 1 line
2.33.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH v8 32/32 docs: Document the FAN_FS_ERROR event
In reply to Gabriel Krisman BertaziDocument the FAN_FS_ERROR event for user administrators and user space developers.
---
Changes Since v7:
- Update semantics
Changes Since v6:
- English fixes (jan)
- Proper document error field (jan)
Changes Since v4:
- Update documentation about reporting non-file error.
Changes Since v3:
- Move FAN_FS_ERROR notification into a subsection of the file.
Changes Since v2:
- NTR
Changes since v1:
- Drop references to location record
- Explain that the inode field is optional
- Explain we are reporting only the first error
--- .../admin-guide/filesystem-monitoring.rst | 76 +++++++++++++++++++ Documentation/admin-guide/index.rst | 1 + 2 files changed, 77 insertions(+) create mode 100644 Documentation/admin-guide/filesystem-monitoring.rst
Documentation/admin-guide/filesystem-monitoring.rst new file +76 -0
@@ -0,0 +1,76 @@ +.. SPDX-License-Identifier: GPL-2.0 + +==================================== +File system Monitoring with fanotify +==================================== + +File system Error Reporting +=========================== + +Fanotify supports the FAN_FS_ERROR event type for file system-wide error +reporting. It is meant to be used by file system health monitoring +daemons, which listen for these events and take actions (notify +sysadmin, start recovery) when a file system problem is detected. + +By design, A FAN_FS_ERROR notification exposes sufficient information +for a monitoring tool to know a problem in the file system has happened. +It doesn't necessarily provide a user space application with semantics +to verify an IO operation was successfully executed. That is out of +scope for this feature. Instead, it is only meant as a framework for +early file system problem detection and reporting recovery tools. + +When a file system operation fails, it is common for dozens of kernel +errors to cascade after the initial failure, hiding the original failure +log, which is usually the most useful debug data to troubleshoot the +problem. For this reason, FAN_FS_ERROR tries to report only the first +error that occurred for a process since the last notification, and it +simply counts additional errors. This ensures that the most important +pieces of information are never lost. + +FAN_FS_ERROR requires the fanotify group to be setup with the +FAN_REPORT_FID flag. + +At the time of this writing, the only file system that emits FAN_FS_ERROR +notifications is Ext4. + +A user space example code is provided at ``samples/fanotify/fs-monitor.c``. + +A FAN_FS_ERROR Notification has the following format:: + + [ Notification Metadata (Mandatory) ] + [ Generic Error Record (Mandatory) ] + [ FID record (Mandatory) ] + +Generic error record +-------------------- + +The generic error record provides enough information for a file system +agnostic tool to learn about a problem in the file system, without +providing any additional details about the problem. This record is +identified by ``struct fanotify_event_info_header.info_type`` being set +to FAN_EVENT_INFO_TYPE_ERROR. + + struct fanotify_event_info_error { + struct fanotify_event_info_header hdr; + __s32 error; + __u32 error_count; + }; + +The `error` field identifies the error in a file-system specific way. +Ext4, for instance, which is the only file system implementing this +interface at the time of this writing, exposes EXT4_ERR_ values in this +field. Please refer to the file system documentation for the meaning of +specific error codes. + +`error_count` tracks the number of errors that occurred and were +suppressed to preserve the original error information, since the last +notification. + +FID record +---------- + +The FID record can be used to uniquely identify the inode that triggered +the error through the combination of fsid and file handle. A file system +specific application can use that information to attempt a recovery +procedure. Errors that are not related to an inode are reported with an +empty file handle of type FILEID_INVALID.
Documentation/admin-guide/index.rst +1 -0
@@ -82,6 +82,7 @@ configure specific aspects of kernel behavior to your liking. edid efi-stub ext4 + filesystem-monitoring nfs/index gpio/index highuid
Signature · 1 line
2.33.0
Jan Kara
maintainerRe: [PATCH v8 32/32] docs: Document the FAN_FS_ERROR event
Quoted 39 lines · Gabriel Krisman Bertazi
Document the
FAN_FS_ERRORevent for user administrators and user space developers.Reviewed-by Amir Goldstein <[email protected]>Signed-off-by Gabriel Krisman Bertazi <[email protected]>---
Changes Since v7:
- Update semantics
Changes Since v6:
- English fixes (jan)
- Proper document error field (jan)
Changes Since v4:
- Update documentation about reporting non-file error.
Changes Since v3:
- MoveFAN_FS_ERRORnotification into a subsection of the file.
Changes Since v2:
- NTR
Changes since v1:
- Drop references to location record
- Explain that the inode field is optional
- Explain we are reporting only the first error--- .../admin-guide/filesystem-monitoring.rst | 76 +++++++++++++++++++ Documentation/admin-guide/index.rst | 1 + 2 files changed, 77 insertions(+) create mode 100644 Documentation/admin-guide/filesystem-monitoring.rstDocumentation/admin-guide/filesystem-monitoring.rst new file +5
-0@@ -0,0 +1,76 @@ +.. SPDX-License-Identifier: GPL-2.0 + +==================================== +File system Monitoring with fanotify +====================================
Documentation/admin-guide/filesystem-monitoring.rst line 15
Quoted 10 lines
Documentation/admin-guide/filesystem-monitoring.rst new file +10
-0+ +File system Error Reporting +=========================== + +Fanotify supports the FAN_FS_ERROR event type for file system-wide error +reporting. It is meant to be used by file system health monitoring +daemons, which listen for these events and take actions (notify +sysadmin, start recovery) when a file system problem is detected. + +By design, A FAN_FS_ERROR notification exposes sufficient information
^^ a
Documentation/admin-guide/filesystem-monitoring.rst line 26
Quoted 11 lines
Documentation/admin-guide/filesystem-monitoring.rst new file +11
-0+for a monitoring tool to know a problem in the file system has happened. +It doesn't necessarily provide a user space application with semantics +to verify an IO operation was successfully executed. That is out of +scope for this feature. Instead, it is only meant as a framework for +early file system problem detection and reporting recovery tools. + +When a file system operation fails, it is common for dozens of kernel +errors to cascade after the initial failure, hiding the original failure +log, which is usually the most useful debug data to troubleshoot the +problem. For this reason, FAN_FS_ERROR tries to report only the first +error that occurred for a process since the last notification, and it
^^^^^^^^ rather for "a filesystem", no?
Quoted 7 lines
Documentation/admin-guide/filesystem-monitoring.rst new file +7
-0+simply counts additional errors. This ensures that the most important +pieces of information are never lost. + +FAN_FS_ERROR requires the fanotify group to be setup with the +FAN_REPORT_FID flag. + +At the time of this writing, the only file system that emits FAN_FS_ERROR
Documentation/admin-guide/filesystem-monitoring.rst line 43
Quoted 10 lines
Documentation/admin-guide/filesystem-monitoring.rst new file +10
-0+notifications is Ext4. + +A user space example code is provided at ``samples/fanotify/fs-monitor.c``. + +A FAN_FS_ERROR Notification has the following format:: + + [ Notification Metadata (Mandatory) ] + [ Generic Error Record (Mandatory) ] + [ FID record (Mandatory) ] +
I'd add a note here that the ordering of "Generic Error Record" and "FID record" is not really guaranteed and refer to sample code for sample parser.
Quoted 10 lines
Documentation/admin-guide/filesystem-monitoring.rst new file +10
-0+Generic error record +-------------------- + +The generic error record provides enough information for a file system +agnostic tool to learn about a problem in the file system, without +providing any additional details about the problem. This record is +identified by ``struct fanotify_event_info_header.info_type`` being set +to FAN_EVENT_INFO_TYPE_ERROR. + + struct fanotify_event_info_error {
Documentation/admin-guide/filesystem-monitoring.rst line 63
Quoted 10 lines
Documentation/admin-guide/filesystem-monitoring.rst new file +10
-0+ struct fanotify_event_info_header hdr; + __s32 error; + __u32 error_count; + }; + +The `error` field identifies the error in a file-system specific way. +Ext4, for instance, which is the only file system implementing this +interface at the time of this writing, exposes EXT4_ERR_ values in this +field. Please refer to the file system documentation for the meaning of +specific error codes.
If 'error' is filesystem-specific number, then how does this work with "filesystem agnostic" tool? All it can tell is "something happened"... If the error was generic errno, I can see some value in the tool being able to tell this is fs corruption (EFSCORRUPTED), hardware problem (EIO), thin provisioning running out of space (ENOSPC) or something else. But yes, I do realize it is going to be more painful to make all filesystem generate these sensible error codes. Even within a filesystem it may be sometimes difficult to propagate proper error code to fsnotify so maybe error codes will not be usable for decisions like above... What do others think?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR