Re: [PATCH 00/11] File system wide monitoring

On Fri, May 21, 2021 at 5:42 AM Gabriel Krisman Bertazi
[off-list ref] wrote:

Hi,

This series follow up on my previous proposal [1] to support file system
wide monitoring.  As suggested by Amir, this proposal drops the ring
buffer in favor of a single slot associated with each mark.  This
simplifies a bit the implementation, as you can see in the code.

As a reminder, This proposal is limited to an interface for
administrators to monitor the health of a file system, instead of a
generic inteface for file errors.  Therefore, this doesn't solve the
problem of writeback errors or the need to watch a specific subtree.

In comparison to the previous RFC, this implementation also drops the
per-fs data and location, and leave those as future extensions.

* Implementation

The feature is implemented on top of fanotify, as a new type of fanotify
mark, FAN_ERROR, which a file system monitoring tool can register to
receive error notifications.  When an error occurs a new notification is
generated, in addition followed by this info field:

 - FS generic data: A file system agnostic structure that has a generic
 error code and identifies the filesystem.  Basically, it let's
 userspace know something happened on a monitored filesystem.  Since
 only the first error is recorded since the last read, this also
 includes a counter of errors that happened since the last read.

* Testing

This was tested by watching notifications flowing from an intentionally
corrupted filesystem in different places.  In addition, other events
were watched in an attempt to detect regressions.

Is there a specific testsuite for fanotify I should be running?

LTP is where we maintain the fsnotify regression test.
The inotify* and fanotify* tests specifically.

* Patches

This patchset is divided as follows: Patch 1 through 5 are refactoring
to fsnotify/fanotify in preparation for FS_ERROR/FAN_ERROR; patch 6 and
7 implement the FS_ERROR API for filesystems to report error; patch 8
add support for FAN_ERROR in fanotify; Patch 9 is an example
implementation for ext4; patch 10 and 11 provide a sample userspace code
and documentation.

I also pushed the full series to:

  https://gitlab.collabora.com/krisman/linux -b fanotify-notifications-single-slot

All in all the series looks good, give or take some implementation
details.

One general comment about UAPI (CC linux-api) -
I think Darrick has proposed to report ino/gen instead of only ino.
I personally think it would be a shame not to reuse the already existing
FAN_EVENT_INFO_TYPE_FID record format, but I can understand why
you did not want to go there:
1. Not all error reports carry inode information
2. Not all filesystems support file handles
3. Any other reason that I missed?

My proposal is that in cases where group was initialized with
FAN_REPORT_FID (implies fs supports file handles) AND error report
does carry inode information, record fanotify_info in fanotify_error_event
and report FAN_EVENT_INFO_TYPE_FID record in addition to
FAN_EVENT_INFO_TYPE_ERROR record to user.

I am not insisting on this change, but I think it won't add much complexity
to your implementation and it will allow more flexibility to the API going
forward.

However, for the time being, if you want to avoid the UAPI discussion,
I don't mind if you disallow FAN_ERROR mark for group with
FAN_REPORT_FID.

In most likelihood, the tool monitoring filesystem for errors will not care
about other events, so it shouldn't care about FAN_REPORT_FID anyway.
I'd like to hear what other think about this point as well.

Thanks,
Amir.