Re: [PATCH v3 00/15] File system wide monitoring

+CC linux-api

On Tue, Jun 29, 2021 at 10:10 PM Gabriel Krisman Bertazi
[off-list ref] wrote:

Hi,

This is the third version of the FAN_FS_ERROR patches.  The main change
in this version is the inode information being reported through an FID
record, which means it requires the group to be created with
FAN_REPORT_FID.  It indeed simplifies a lot the FAN_FS_ERROR patch
itself.

I am glad that you took this path.
Uniformity across the UAPI is important.

This change raises the question of how we report non-inode errors.  On
one hand, we could omit the FID report, but then fsid would also be
ommited.  I chose to report these kind of errors against the root
inode.

There are other option to consider.

To avoid special casing error events in fanotify event read code,
it would is convenient to use a non-zero length FID, but you can
use a 8 bytes zero buffer as NULL-FID

If I am not mistaken, that amounts to 64 bytes of event_len
including the event_metadata and both records which is pretty
nicely aligned.

All 3 handle_type options below are valid options:
1. handle_type FILEID_ROOT
2. handle_type FILEID_INVALID
3. handle_type FILEID_INO32_GEN (i.e. ino=0;gen=0)

The advantage of option #3 is that the monitoring program
does not need to special case the NULL_FID case when
parsing the FID to informative user message.

The other changes in this iteration were made to attend to Amir
feedback.  Thank you again for your very detailed input.  It is really
appreciated.

This was tested with LTP for regressions, and also using the sample on
the last patch, with a corrupted image.  I can publish the bad image
upon request.

Just to set expectations, we now have an official standard for fanotify [1]
where we require an LTP test and man page update patch before merge
of UAPI changes.

That should not stop us from continuing the review process - it's just
a heads up, but I think that we are down to implementation details in
the review anyway and that the UAPI (give or take root inode) is
pretty much clear at this point, so spreading the review of UAPI to
wider audience is not a bad idea.

w.r.t man page update, I know you have created the admin-guide book,
but it's not the same. For linux-api reviewers, reviewing the changed to
fanotify man pages is good way to make sure we did not miss any corners.

w.r.t LTP test, I don't think that using a corrupt image will be a good way
for an LTP test. LTP tests can prepare and mount an ext4 loop image.
Does ext4 have some debugging method to inject an error?
Because that would be the best way IMO.
If it doesn't, you can implement this in ext4 and use it in the test if that
debug file exists - skip the test otherwise - it's common practice.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/YMKv1U7tNPK955ho@google.com/ (local)