Re: [RFC][PATCH 0/2] Monitoring unmounted fs with fanotify

On Tue, Apr 18, 2023 at 5:12 PM Christian Brauner [off-list ref] wrote:

On Tue, Apr 18, 2023 at 04:56:40PM +0300, Amir Goldstein wrote:

quoted

On Tue, Apr 18, 2023 at 4:33 PM Christian Brauner [off-list ref] wrote:

quoted

On Fri, Apr 14, 2023 at 09:29:01PM +0300, Amir Goldstein wrote:

quoted

Jan,

Followup on my quest to close the gap with inotify functionality,
here is a proposal for FAN_UNMOUNT event.

I have had many design questions about this:

I'm going to humbly express what I feel makes sense to me when looking
at this from a user perspective:

quoted

1) Should we also report FAN_UNMOUNT for marked inodes and sb
   on sb shutdown (same as IN_UNMOUNT)?

My preference would be if this would be a separate event type.
FAN_SB_SHUTDOWN or something.

If we implement an event for this at all, I would suggest FAN_IGNORED
or FAN_EVICTED, which has the same meaning as IN_IGNORED.
When you get an event that the watch went away, it could be because of:
1. watch removed by user
2. watch removed because inode was evicted (with FAN_MARK_EVICTABLE)
3. inode deleted
4. sb shutdown

IN_IGNORED is generated in all of the above except for inode evict
that is not possible with inotify.

User can figure out on his own if the inode was deleted or if fs was unmounted,
so there is not really a need for FAN_SB_SHUTDOWN IMO.

Ok, sounds good.

quoted

Actually, I think that FAN_IGNORED would be quite useful for the
FAN_MARK_EVICTABLE case, but it is a bit less trivial to implement
than FAN_UNMOUNT was.

quoted

quoted

2) Should we also report FAN_UNMOUNT on sb mark for any unmounts
   of that sb?

I don't think so. It feels to me that if you watch an sb you don't
necessarily want to watch bind mounts of that sb.

quoted

3) Should we report also the fid of the mount root? and if we do...
4) Should we report/consider FAN_ONDIR filter?

All of the questions above I answered "not unless somebody requests"
in this first RFC.

Fwiw, I agree.

quoted

Specifically, I did get a request for an unmount event for containers
use case.

I have also had doubts regarding the info records.
I decided that reporting fsid and mntid is minimum, but couldn't
decide if they were better of in a single MNTID record or seprate
records.

I went with separate records, because:
a) FAN_FS_ERROR has set a precendent of separate fid record with
   fsid and empty fid, so I followed this precendent
b) MNTID record we may want to add later with FAN_REPORT_MNTID
   to all the path events, so better that it is independent

Just thought of another reason:
 c) FAN_UNMOUNT does not need to require FAN_REPORT_FID
     so it does not depend on filesystem having a valid f_fsid nor
     exports_ops. In case of "pseudo" fs, FAN_UNMOUNT can report
     only MNTID record (I will amend the patch with this minor change).

I see some pseudo fses generate f_fsid, e.g., tmpfs in mm/shmem.c

tmpfs is not "pseudo" in my eyes, because it implements a great deal of the
vfs interfaces, including export_ops.

and also I fixed its f_fsid recently:
59cda49ecf6c shmem: allow reporting fanotify events with file handles on tmpfs

At the risk of putting my foot in my mouth, what's stopping us from
making them all support f_fsid?

Nothing much. Jan had the same opinion [1].

We could do either:
1. use uuid_to_fsid() in vfs_statfs() if fs has set s_uuid and not set f_fsid
2. use s_dev as f_fsid in vfs_statfs() if fs did not set f_fsid nor s_uuid
3. randomize s_uuid for simple fs (like tmpfs)
4. any combination of the above and more

Note that we will also need to decide what to do with
name_to_handle_at() for those pseudo fs.

Quoting Jan from [1]:
"But otherwise the proposal to make name_to_handle_at() work even for
filesystems not exportable through NFS makes sense to me. But I guess we
need some buy-in from VFS maintainers for this." (hint hint).

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/ (local)