On Mon 12-07-21 16:00:54, Amir Goldstein wrote:
> On Mon, Jul 12, 2021 at 2:10 PM Jan Kara <jack@xxxxxxx> wrote:
> > On Sun 11-07-21 20:02:29, Amir Goldstein wrote:
> > > I am struggling with an attempt to extend the fanotify API and
> > > I wanted to ask your opinion before I go too far in the wrong direction.
> > >
> > > I am working with an application that used to use inotify rename
> > > cookies to match MOVED_FROM/MOVED_TO events.
> > > The application was converted to use fanotify name events, but
> > > the rename cookie functionality was missing, so I am carrying
> > > a small patch for FAN_REPORT_COOKIE.
> > >
> > > I do not want to propose this patch for upstream, because I do
> > > not like this API.
> > >
> > > What I thought was that instead of a "cookie" I would like to
> > > use the child fid as a way to pair up move events.
> > > This requires that the move events will never be merged and
> > > therefore not re-ordered (as is the case with inotify move events).
> > >
> > > My thinking was to generalize this concept and introduce
> > > FAN_REPORT_CHILD_FID flag. With that flag, dirent events
> > > will report additional FID records, like events on a non-dir child
> > > (but also for dirent events on subdirs).
> >
> > I'm starting to get lost in what reports what so let me draw a table here:
> >
> > Non-directories
> > DFID FID CHILD_FID
> > ACCESS/MODIFY/OPEN/CLOSE/ATTRIB parent self self
> > CREATE/DELETE/MOVE - - -
> > DELETE_SELF/MOVE_SELF x self self
> > ('-' means cannot happen, 'x' means not generated)
> >
> > Directories
> > DFID FID CHILD_FID
> > ACCESS/MODIFY/OPEN/CLOSE/ATTRIB self self self
> > CREATE/DELETE/MOVE self self target
> > DELETE_SELF/MOVE_SELF x self self
> >
> > Did I get this right?
>
> I am not sure if the columns in your table refer to group flags
> or to info records types? or a little bit of both, but I did not
> mean for CHILD_FID to be a different record type.
Yeah, a bit of both.
> Anyway, the only complexity missing from the table is that
> for events reporting a single record with fid of a directory,
> (i.e. self event on dir or dirent event) the record type depends
> on the group flags.
>
> FAN_REPORT_FID => FAN_EVENT_INFO_TYPE_FID
> FAN_REPORT_DIR_FID => FAN_EVENT_INFO_TYPE_DFID
Right, I didn't realize this.
> > I guess "CHILD_FID" seems somewhat confusing as it isn't immediately clear
> > from the name what it would report e.g. for open of a non-directory.
>
> I agree it is a bit confusing. FWIW for events on a non-dir child (not dirent)
> FAN_REPORT_FID and FAN_REPORT_CHILD_FID flags yield the exact
> same event info.
>
> > Maybe
> > we could call it "TARGET_FID"? Also I'm not sure it needs to be exclusive
> > with FID. Sure it doesn't make much sense to report both FID and CHILD_FID
> > but does the exclusivity buy us anything? I guess I don't have strong
> > opinion either way, I'm just curious.
> >
>
> FAN_REPORT_TARGET_FID sounds good to me.
> You are right. I don't think that exclusivity buys us anything.
OK. I've realized that the exclusivity is needed if we want to report info
enabled by FAN_REPORT_TARGET_FID as FAN_EVENT_INFO_TYPE_FID. Because
otherwise it would not be well defined what information is contained in
FAN_EVENT_INFO_TYPE_FID. So either we have to go for exclusivity or for new
type of event information.
> > > There are other benefits from FAN_REPORT_CHILD_FID which are
> > > not related to matching move event pairs, such as the case described
> > > in this discussion [2], where I believe you suggested something along
> > > the lines of FAN_REPORT_CHILD_FID.
> > >
> > > [2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxhEsbfA5+sW4XPnUKgCkXtwoDA-BR3iRO34Nx5c4y7Nug@xxxxxxxxxxxxxx/
> >
> > Yes, I can see FAN_REPORT_CHILD_FID (or however we call it) can be useful
> > at times (in fact I think we made a mistake that we didn't make reported
> > FID to always be what you now suggest as CHILD_FID, but we found that out
> > only when DFID+NAME implementation settled so that train was long gone).
>
> Yes, we did. FAN_REPORT_TARGET_FID is also about trying to make amends.
> We could have just as well called it FAN_REPORT_FID_V2, but no ;-)
OK, I was suspecting that but wasn't sure :). I guess that's another reason
why exclusivity makes more sense.
> > > Either FAN_REPORT_CHILD_FID would also prevent dirent events
> > > from being merged or we could use another flag for that purpose,
> > > but I wasn't able to come up with an idea for a name for this flag :-/
> > >
> > > I sketched this patch [1] to implement the flag and to document
> > > the desired semantics. It's only build tested and I did not even
> > > implement the merge rules listed in the commit message.
> > >
> > > [1] https://github.com/amir73il/linux/commits/fanotify_child_fid
> >
> > WRT changes to merging: Whenever some application wants to depend on the
> > ordering of events I'm starting to get suspicious.
>
> I completely agree with that sentiment.
>
> But note that the application does NOT require event ordering.
>
> I was proposing the strict ordering of MOVE_ events as a method
> to allow for matching of MOVE_ pairs of the same target as
> a *replacement* for the inotify rename cookie method.
Aha, I see I got confused a bit. Sorry about that.
> > What is it using these events for?
>
> The application is trying to match MOVE_ event pairs.
> It's a best effort situation - when local file has been renamed,
> a remote rename can also be attempted while verifying that the
> recorded fid of the source (in remote file) matches the fid of the
> local target.
>
> > How is renaming different from linking a file into a new dir
> > and unlinking it from the previous one which is a series of events that
> > could be merged?
>
> It is different because renames are common operations that actual people
> often do and link+unlink are less common so we do not care to optimize
> them. Anyway, as many other applications, our application does not
> support syncing hardlinks to remote location, so link+unlink would be
> handled as plain copy+delete and dedup of copied file is handled
> is handled by the remote sync protocol.
>
> As a matter of fact, a rename could also be (and sometimes is) handled
> as copy+delete. In that case, the remote content would be fine but the
> logs and change history would be inaccurate.
>
> BTW, in my sketch commit message I offer to prevent merge of all dirent
> events, not only MOVE_, because I claim that there is not much to be
> gained from merging the CREATE/DELETE of a specific TARGET_FID
> event with other events as there are normally very few of those events.
>
> However, while I can argue why it is useful to avoid merge of dirent events,
> it's not as easy for me to come up with a name for that flag not to
> easily explain the semantics in man page :-/
> so any help with coming up with simplified semantics would be appreciated.
Just a brainstorming idea: How about creating new event FAN_RENAME that
would report two DFIDs (if it is cross directory rename)? On the uAPI side
it is very straightforward I think (unlike inotify where this could not be
easily done because of fixed sized event + name). On the kernel API side we
need to somehow feed the second directory into fsnotify() but with the
changes Gabriel is doing it should not be that painful anymore... And then
we can just avoid any problems with event matching, event merging etc.
Thoughts?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
