From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:00:28
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 <djwong@kernel.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dave Chinner <david@fromorbit.com>
Cc: jack@suse.com
To: amir73il@gmail.com
Cc: dhowells@redhat.com
Cc: khazhy@google.com
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-ext4@vger.kernel.org
Cc: linux-api@vger.kernel.org
Cc: linux-api@vger.kernel.org
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
--
2.33.0
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:00:34
From: Amir Goldstein <amir73il@gmail.com>
Align the arguments of fsnotify_name() to those of fsnotify().
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
include/linux/fsnotify.h | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:00:40
From: Amir Goldstein <amir73il@gmail.com>
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.
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
[Expand explanation in commit message]
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
@@ -262,6 +265,19 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type)}}+staticinlinestructdentry*fsnotify_data_dentry(constvoid*data,intdata_type)+{+switch(data_type){+caseFSNOTIFY_EVENT_DENTRY:+/* Non const is needed for dget() */+return(structdentry*)data;+caseFSNOTIFY_EVENT_PATH:+return((conststructpath*)data)->dentry;+default:+returnNULL;+}+}+staticinlineconststructpath*fsnotify_data_path(constvoid*data,intdata_type){
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:00:45
From: Amir Goldstein <amir73il@gmail.com>
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.
Fixes: aa93bdc5500c ("fsnotify: use helpers to access data by data_type")
Link: https://lore.kernel.org/linux-fsdevel/87mtp5yz0q.fsf@collabora.com/
Reviewed-by: Jan Kara <jack@suse.cz>
Reported-by: Gabriel Krisman Bertazi <redacted>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
include/linux/fsnotify.h | 22 ++++++++++++++++------
kernel/audit_fsnotify.c | 3 +--
kernel/audit_watch.c | 3 +--
3 files changed, 18 insertions(+), 10 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:00:52
Some 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.
Fixes: 94e00d28a680 ("fsnotify: use hash table for faster events merge")
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:01:05
Every 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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
@@ -126,17 +126,24 @@ static int fanotify_fid_info_len(int fh_len, int name_len)FANOTIFY_EVENT_ALIGN);}-staticintfanotify_event_info_len(unsignedintinfo_mode,-structfanotify_event*event)+staticsize_tfanotify_event_len(unsignedintinfo_mode,+structfanotify_event*event){-structfanotify_info*info=fanotify_event_info(event);-intdir_fh_len=fanotify_event_dir_fh_len(event);-intfh_len=fanotify_event_object_fh_len(event);-intinfo_len=0;+size_tevent_len=FAN_EVENT_METADATA_LEN;+structfanotify_info*info;+intdir_fh_len;+intfh_len;intdot_len=0;+if(!info_mode)+returnevent_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);}elseif((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);-returninfo_len;+returnevent_len;}/*
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:01:09
FAN_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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
@@ -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;+return0;+}++/* Check if filesystem can encode a unique fid */+staticintfanotify_test_fid(structdentry*dentry)+{/**Weneedtomakesurethatthefilesystemsupportsatleast*encodingafilehandlesousercanusename_to_handle_at()to
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:01:16
According 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.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/inotify/inotify_user.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:01:23
Similarly to fanotify_is_perm_event and friends, provide a helper
predicate to say whether a mask is of an overflow event.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.h | 3 ++-
include/linux/fsnotify_backend.h | 5 +++++
2 files changed, 7 insertions(+), 1 deletion(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:01:34
fsnotify_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.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
@@ -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);
@@ -498,16 +498,25 @@ extern int fsnotify_fasync(int fd, struct file *file, int on);externvoidfsnotify_destroy_event(structfsnotify_group*group,structfsnotify_event*event);/* attach the event to the group notification queue */-externintfsnotify_add_event(structfsnotify_group*group,-structfsnotify_event*event,-int(*merge)(structfsnotify_group*,-structfsnotify_event*),-void(*insert)(structfsnotify_group*,-structfsnotify_event*));+externintfsnotify_insert_event(structfsnotify_group*group,+structfsnotify_event*event,+int(*merge)(structfsnotify_group*,+structfsnotify_event*),+void(*insert)(structfsnotify_group*,+structfsnotify_event*));++staticinlineintfsnotify_add_event(structfsnotify_group*group,+structfsnotify_event*event,+int(*merge)(structfsnotify_group*,+structfsnotify_event*))+{+returnfsnotify_insert_event(group,event,merge,NULL);+}+/* Queue overflow event to a notification group */staticinlinevoidfsnotify_queue_overflow(structfsnotify_group*group){-fsnotify_add_event(group,group->overflow_event,NULL,NULL);+fsnotify_add_event(group,group->overflow_event,NULL);}staticinlineboolfsnotify_is_overflow_event(u32mask)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:01:43
Some 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.
Suggested-by: Jan Kara <jack@suse.cz>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
--
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(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:01:49
FAN_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.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fsnotify.c | 3 +++
1 file changed, 3 insertions(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:01:55
For 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.
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
@@ -155,7 +155,7 @@ struct fsnotify_ops {conststructqstr*file_name,u32cookie);void(*free_group_priv)(structfsnotify_group*group);void(*freeing_mark)(structfsnotify_mark*mark,structfsnotify_group*group);-void(*free_event)(structfsnotify_event*event);+void(*free_event)(structfsnotify_group*group,structfsnotify_event*event);/* called on final put+free to free memory */void(*free_mark)(structfsnotify_mark*mark);};
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:02:09
FAN_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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:02:13
Allow passing a NULL hash to fanotify_encode_fh and avoid calculating
the hash if not needed.
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:02:26
Instead 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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:02:37
Like 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.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
@@ -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)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:02:44
Expose 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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
@@ -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_IGNOREDoverloadsFS_ERROR.Itisonlyusedinternallybyinotify+*whichdoesnotsupportFS_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 | \
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:02:51
FAN_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.
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 1 +
include/uapi/linux/fanotify.h | 1 +
2 files changed, 2 insertions(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:03:04
Pre-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.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:03:09
Allow the FAN_FS_ERROR group mempool to grow up to an upper limit
dynamically, instead of starting already at the limit. This doesn't
bother resizing on mark removal, but next time a mark is added, the slot
will be either reused or resized. Also, if several marks are being
removed at once, most likely the group is going away anyway.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++-----
include/linux/fsnotify_backend.h | 1 +
2 files changed, 22 insertions(+), 5 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:03:18
Once 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.
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:03:22
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 <redacted>
---
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(-)
@@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,returnfanotify_info_equal(info1,info2);}+staticboolfanotify_error_event_equal(structfanotify_error_event*fee1,+structfanotify_error_event*fee2)+{+/* Error events against the same file system are always merged. */+if(!fanotify_fsid_equal(&fee1->fsid,&fee2->fsid))+returnfalse;++returntrue;+}+staticboolfanotify_should_merge(structfanotify_event*old,structfanotify_event*new){
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:03:29
fanotify_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.
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
@@ -171,12 +171,19 @@ static inline void fanotify_init_event(struct fanotify_event *event,event->pid=NULL;}+#define FANOTIFY_INLINE_FH(size) \+struct{\+structfanotify_fhobject_fh;\+/* Space for object_fh.buf[] - access with fanotify_fh_buf() */\+unsignedchar_inline_fh_buf[(size)];\+}+structfanotify_fid_event{structfanotify_eventfae;__kernel_fsid_tfsid;-structfanotify_fhobject_fh;-/* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */-unsignedchar_inline_fh_buf[FANOTIFY_INLINE_FH_LEN];++/* This must be the last element of the structure. */+FANOTIFY_INLINE_FH(FANOTIFY_INLINE_FH_LEN);};staticinlinestructfanotify_fid_event*
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:03:40
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 <redacted>
---
fs/notify/fanotify/fanotify.h | 13 +++++++++++++
fs/notify/fanotify/fanotify_user.c | 13 +++++++------
2 files changed, 20 insertions(+), 6 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:03:49
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 any file_handle encoded
Even though FILEID_ROOT is used internally, make zero-length file
handles be reported as FILEID_INVALID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify_user.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:04:01
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 than MAX_HANDLE_SZ is ever found.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:04:07
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 <redacted>
---
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(+)
@@ -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;
@@ -209,6 +209,9 @@ struct fanotify_error_event {u32err_count;/* Suppressed errors count */__kernel_fsid_tfsid;/* FSID this error refers to. */++/* This must be the last element of the structure. */+FANOTIFY_INLINE_FH(MAX_HANDLE_SZ);};staticinlinestructfanotify_error_event*
@@ -267,6 +274,10 @@ static inline int fanotify_event_dir_fh_len(struct fanotify_event *event)staticinlineboolfanotify_event_has_object_fh(structfanotify_event*event){++/* For error events, even zeroed fh are reported. */+if(event->type==FANOTIFY_EVENT_TYPE_FS_ERROR)+returntrue;if(fanotify_event_object_fh_len(event)>0)returntrue;
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:04:14
The 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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:04:24
Wire 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.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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(-)
@@ -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 | \
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:04:39
Send 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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
Changes since v6:
- Report ext4_std_errors agains superblock (jan)
---
fs/ext4/super.c | 8 ++++++++
1 file changed, 8 insertions(+)
From: Gabriel Krisman Bertazi <hidden> Date: 2021-10-19 00:04:51
Document the FAN_FS_ERROR event for user administrators and user space
developers.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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
@@ -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.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 05:34:56
On Tue, Oct 19, 2021 at 3:01 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
FAN_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.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fsnotify.c | 3 +++
1 file changed, 3 insertions(+)
@@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,if(WARN_ON_ONCE(!ops->handle_inode_event))return0;+if(!inode)+return0;+
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.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 05:38:48
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
Pre-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.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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 <amir73il@gmail.com>
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 05:50:36
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
Allow the FAN_FS_ERROR group mempool to grow up to an upper limit
dynamically, instead of starting already at the limit. This doesn't
bother resizing on mark removal, but next time a mark is added, the slot
will be either reused or resized. Also, if several marks are being
removed at once, most likely the group is going away anyway.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++-----
include/linux/fsnotify_backend.h | 1 +
2 files changed, 22 insertions(+), 5 deletions(-)
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.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 05:52:52
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
Once 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.
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 05:56:46
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
Error events (FAN_FS_ERROR) against the same file system can be merged
by simply iterating the error count. The hash is taken from the fsid,
without considering the FH. This means that only the first error object
is reported.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
quoted hunk
---
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(-)
@@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,returnfanotify_info_equal(info1,info2);}+staticboolfanotify_error_event_equal(structfanotify_error_event*fee1,+structfanotify_error_event*fee2)+{+/* Error events against the same file system are always merged. */+if(!fanotify_fsid_equal(&fee1->fsid,&fee2->fsid))+returnfalse;++returntrue;+}+staticboolfanotify_should_merge(structfanotify_event*old,structfanotify_event*new){
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 05:57:48
On Tue, Oct 19, 2021 at 3:04 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
Wire 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.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
quoted hunk
---
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(-)
@@ -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 | \--
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 06:03:08
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
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 than MAX_HANDLE_SZ is ever found.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 06:07:46
On Tue, Oct 19, 2021 at 3:04 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
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 <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
with minor nit below..
quoted hunk
---
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(+)
@@ -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;
@@ -209,6 +209,9 @@ struct fanotify_error_event {u32err_count;/* Suppressed errors count */__kernel_fsid_tfsid;/* 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.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 06:09:49
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
fanotify_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.
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
with minor nit
---
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.
@@ -171,12 +171,19 @@ static inline void fanotify_init_event(struct fanotify_event *event,event->pid=NULL;}+#define FANOTIFY_INLINE_FH(size) \+struct{\+structfanotify_fhobject_fh;\+/* Space for object_fh.buf[] - access with fanotify_fh_buf() */\+unsignedchar_inline_fh_buf[(size)];\+}+structfanotify_fid_event{structfanotify_eventfae;__kernel_fsid_tfsid;-structfanotify_fhobject_fh;-/* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */-unsignedchar_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.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 06:12:47
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted hunk
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 <redacted>
---
fs/notify/fanotify/fanotify.h | 13 +++++++++++++
fs/notify/fanotify/fanotify_user.c | 13 +++++++------
2 files changed, 20 insertions(+), 6 deletions(-)
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-19 06:14:10
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
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 any file_handle encoded
Even though FILEID_ROOT is used internally, make zero-length file
handles be reported as FILEID_INVALID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 10:01:09
On Tue 19-10-21 08:34:41, Amir Goldstein wrote:
On Tue, Oct 19, 2021 at 3:01 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted
FAN_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.
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fsnotify.c | 3 +++
1 file changed, 3 insertions(+)
@@ -252,6 +252,9 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,if(WARN_ON_ONCE(!ops->handle_inode_event))return0;+if(!inode)+return0;+
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.
And furthermore you cannot really export kernfs :)
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
Yeah, like:
if (WARN_ON_ONCE(!inode && !dir))
return 0;
3. Add WARN_ON_ONCE(!inode) before trace_nfsd_file_fsnotify_handle_event()
in nfsd_file_fsnotify_handle_event()
And:
if (WARN_ON_ONCE(!inode))
return 0;
Sounds like a good plan to me.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 11:52:45
On Tue 19-10-21 08:38:34, Amir Goldstein wrote:
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted
Pre-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.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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 <amir73il@gmail.com>
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.
Perhaps FANOTIFY_DEFAULT_FEE_POOL_SIZE would better describe what this
constant is about?
Otherwise feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 12:03:23
On Tue 19-10-21 08:50:23, Amir Goldstein wrote:
On Tue, Oct 19, 2021 at 3:03 AM Gabriel Krisman Bertazi
[off-list ref] wrote:
quoted
Allow the FAN_FS_ERROR group mempool to grow up to an upper limit
dynamically, instead of starting already at the limit. This doesn't
bother resizing on mark removal, but next time a mark is added, the slot
will be either reused or resized. Also, if several marks are being
removed at once, most likely the group is going away anyway.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
fs/notify/fanotify/fanotify_user.c | 26 +++++++++++++++++++++-----
include/linux/fsnotify_backend.h | 1 +
2 files changed, 22 insertions(+), 5 deletions(-)
This is not what I had in mind.
I was thinking start with ~32 and double each time limit is reached.
Do you mean when number of FS_ERROR marks reaches the number of preallocated
events? We could do that but note that due to mempool implementation limits
there cannot be more than 255 preallocated events, also mempool_resize()
will only update number of slots for preallocated events but these slots
will be empty. You have to manually allocate and free events to fill these
slots with preallocated events.
And also, this code grows the pool to infinity with add/remove mark loop.
I see a cap at FANOTIFY_DEFAULT_MAX_FEE_POOL in the code there. But I don't
think there's a good enough reason to hard-limit number of FS_ERROR marks
at 128. As I explained in the previous version of the series, in vast
majority of cases we will not use even a single preallocated event...
Anyway, since I clearly did not understand how mempool works and
Jan had some different ideas I would leave it to Jan to explain
how he wants the mempool init limit and resize to be implemented.
Honestly, I'm for keeping it simple for now. Just 32 preallocated events
and try to come up with something more clever only if someone actually
complains.
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 13:52:40
On Mon 18-10-21 21:00:05, Gabriel Krisman Bertazi wrote:
Error events (FAN_FS_ERROR) against the same file system can be merged
by simply iterating the error count. The hash is taken from the fsid,
without considering the FH. This means that only the first error object
is reported.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
quoted hunk
---
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(-)
@@ -111,6 +111,16 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,returnfanotify_info_equal(info1,info2);}+staticboolfanotify_error_event_equal(structfanotify_error_event*fee1,+structfanotify_error_event*fee2)+{+/* Error events against the same file system are always merged. */+if(!fanotify_fsid_equal(&fee1->fsid,&fee2->fsid))+returnfalse;++returntrue;+}+staticboolfanotify_should_merge(structfanotify_event*old,structfanotify_event*new){
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 13:58:39
On Mon 18-10-21 21:00:06, Gabriel Krisman Bertazi wrote:
fanotify_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.
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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.
One nit from me as well :)
+#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:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 14:03:17
On Mon 18-10-21 21:00:07, Gabriel Krisman Bertazi wrote:
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
into helper functions. This shouldn't have any impact on the code, but
simplifies further patches.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
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:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 14:06:51
On Mon 18-10-21 21:00:09, Gabriel Krisman Bertazi wrote:
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 than MAX_HANDLE_SZ is ever found.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 14:08:48
On Mon 18-10-21 21:00:08, Gabriel Krisman Bertazi wrote:
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 any file_handle encoded
Even though FILEID_ROOT is used internally, make zero-length file
handles be reported as FILEID_INVALID.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
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:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 14:41:18
On Mon 18-10-21 21:00:10, Gabriel Krisman Bertazi wrote:
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 <redacted>
This WARN_ON_ONCE is now pointless since you dereference report->inode
above... So I guess move the dereference after WARN?
quoted hunk
@@ -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:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 15:24:16
On Mon 18-10-21 21:00:12, Gabriel Krisman Bertazi wrote:
Wire 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.
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
quoted hunk
---
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(-)
@@ -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 | \
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 15:44:34
On Mon 18-10-21 21:00:13, Gabriel Krisman Bertazi wrote:
Send 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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 15:49:27
On Mon 18-10-21 21:00:14, Gabriel Krisman Bertazi wrote:
Introduce an example of a FAN_FS_ERROR fanotify user to track filesystem
errors.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 16:01:55
On Tue 19-10-21 17:44:26, Jan Kara wrote:
On Mon 18-10-21 21:00:13, Gabriel Krisman Bertazi wrote:
quoted
Send 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.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
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:
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
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Jan Kara <jack@suse.cz> Date: 2021-10-19 16:47:38
On Mon 18-10-21 21:00:15, Gabriel Krisman Bertazi wrote:
quoted hunk
Document the FAN_FS_ERROR event for user administrators and user space
developers.
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
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
@@ -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
^^ a
+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?
+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) ]
+
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.
+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.
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
--
Jan Kara [off-list ref]
SUSE Labs, CR
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 hunk
+ 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