Re: [RFC][PATCH] fanotify: Enable FAN_REPORT_FID on more filesystem types

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Amir!

On Tue 11-04-23 15:40:37, Amir Goldstein wrote:
> If kernel supports FAN_REPORT_ANY_FID, use this flag to allow testing
> also filesystems that do not support fsid or NFS file handles (e.g. fuse).
> 
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> 
> Jan,
> 
> I wanted to run an idea by you.
> 
> My motivation is to close functional gaps between fanotify and inotify.
> 
> One of the largest gaps right now is that FAN_REPORT_FID is limited
> to a subset of local filesystems.
> 
> The idea is to report fid's that are "good enough" and that there
> is no need to require that fid's can be used by open_by_handle_at()
> because that is a non-requirement for most use cases, unpriv listener
> in particular.

OK. I'd note that if you report only inode number, you are prone to the
problem that some inode gets freed (file deleted) and then reallocated (new
file created) and the resulting identifier is the same. It can be
problematic for a listener to detect these cases and deal with them.
Inotify does not have this problem at least for some cases because 'wd'
uniquely identifies the marked inode. For other cases (like watching dirs)
inotify has similar sort of problems. I'm muttering over this because in
theory filesystems not having i_generation counter on disk could approach
the problem in a similar way as FAT and then we could just use
FILEID_INO64_GEN for the file handle.

Also I have noticed your workaround with using st_dev for fsid. As I've
checked, there are actually very few filesystems that don't set fsid these
days. So maybe we could just get away with still refusing to report on
filesystems without fsid and possibly fixup filesystems which don't set
fsid yet and are used enough so that users complain?

> I chose a rather generic name for the flag to opt-in for "good enough"
> fid's.  At first, I was going to make those fid's self describing the
> fact that they are not NFS file handles, but in the name of simplicity
> to the API, I decided that this is not needed.

I'd like to discuss a bit about the meaning of the flag. On the first look
it is a bit strange to have a flag that says "give me a fh, if you don't
have it, give me ino". It would seem cleaner to have "give me fh" kind of
interface (FAN_REPORT_FID) and "give me ino" kind of interface (new
FAN_REPORT_* flag). I suspect you've chosen the more complex meaning
because you want to allow a usecase where watches of filesystems which
don't support filehandles are mixed with watches of filesystems which do
support filehandles in one notification group and getting filehandles is
actually prefered over getting just inode numbers? Do you see real benefit
in getting file handles when userspace has to implement fallback for
getting just inode numbers anyway?

> The patch below is from the LTP test [1] that verifies reported fid's.
> I am posting it because I think that the function fanotify_get_fid()
> demonstrates well, how a would-be fanotify library could be used to get
> a canonical fid.
> 
> That would-be routine can easily return the source of the fid values
> for a given filesystem and that information is constant for all objects
> on a given filesystem instance.
> 
> The choise to encode an actual file_handle of type FILEID_INO64 may
> seem controversial at first, but it simplifies things so much, that I
> grew very fond of it.

FILEID_INO64 is a bit of a hack in particular because it's difficult to
pretend FILEID_INO64 can be used for NFS. But I agree it is very convenient
:). If we were to do this cleanly we'd have to introduce a new info
structure with ino instead of handle and three new FAN_EVENT_INFO_TYPE_*
types. As I wrote above, we might be able to actually fill-in
FILEID_INO64_GEN which would be less controversial then I suppose.

> The LTP patch also demonstrated how terribly trivial it would be to
> adapt any existing fanotify programs to support any fs.
> 
> Kernel patches [2] are pretty simple IMO and
> man page patch [3] demonstrates that the API changes are minimal.
> 
> Thoughts?

								Honza

-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux