> > @@ -491,19 +491,31 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
> > if (!inode) {
> > /* Dirent event - report on TYPE_INODE to dir */
> > inode = dir;
> > - /* For FS_RENAME, inode is old_dir and inode2 is new_dir */
> > - if (mask & FS_RENAME) {
> > - moved = fsnotify_data_dentry(data, data_type);
> > - inode2 = moved->d_parent->d_inode;
> > - inode2_type = FSNOTIFY_ITER_TYPE_INODE2;
> > - }
> > + } else if (mask & FS_RENAME) {
> > + /* For FS_RENAME, dir1 is old_dir and dir2 is new_dir */
> > + moved = fsnotify_data_dentry(data, data_type);
> > + dir1 = moved->d_parent->d_inode;
> > + dir2 = dir;
> > + if (dir1->i_fsnotify_marks || dir2->i_fsnotify_marks)
> > + dir1_type = FSNOTIFY_ITER_TYPE_OLD_DIR;
> > + /*
> > + * Send FS_RENAME to groups watching the moved inode itself
> > + * only if the moved inode is a non-dir.
> > + * Sending FS_RENAME to a moved watched directory would be
> > + * confusing and FS_MOVE_SELF provided enough information to
> > + * track the movements of a watched directory.
> > + */
> > + if (mask & FS_ISDIR)
> > + inode = NULL;
>
> So I agree that sending FS_RENAME to a directory when the directory itself
> moves is confusing. But then it makes me wonder whether it would not be
> more logical if we extended FS_MOVE_SELF rather than FS_RENAME. Currently
> FS_MOVE_SELF is an inode event but we could expand it to provide new parent
> directory to priviledged listeners (and even old directory if we wanted).
> But I guess the concern is that we'd have to introduce a group flag for
> this to make sure things are backward compatible, whereas with FAN_RENAME
> we could mostly get away without a feature flag?
>
This thought has crossed my mind.
There are several issues with extending FS_MOVE_SELF besides the one
that you mentioned.
1. It is not needed for tracking movement of dir - the only reason we need to
extend rename/create/delete for non-dir is because fid alone is not enough
to find the moved inode new location
2. While we could extend FS_MOVE_SELF, we cannot extend FS_DELETE_SELF
(and there is no FS_CREATE_SELF), so it is better to extend all the
dirent events and not any self events
3. But the nock-out argument is the one that you wrote - we can (ab)use
FAN_ERPORT_TARGET_FID as the backward compat barrier for changing
the behavior of dirent events and it would almost look like we had
thought about
this in advance ;)
> > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> > index bb8467cd11ae..75f1048443a5 100644
> > --- a/include/linux/fsnotify.h
> > +++ b/include/linux/fsnotify.h
> > @@ -28,18 +28,19 @@
> > */
> > static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
> > struct inode *dir, const struct qstr *name,
> > - u32 cookie)
> > + struct inode *inode, u32 cookie)
>
> So you add 'inode' argument to fsnotify_name() but never actually use it?
> The only place where inode would be non-NULL is changed to use fsnotify()
> directly...
My bad, I wasn't going to extend fsnotify_name(), but then I thought of the
FAN_CREATE/FAN_DELETE case, so I wanted to make this more obvious
in the code that dirent events should be reported to the child inode,
but I forget
to use the extended fsnotify_name() for FAN_RENAME and left it using fsnotify().
>
> Also I'm not sure why you pass the inode for rename event now when the same
> inode is passed as 'dentry' in data? I feel like I'm missing something
> here.
The semantics of @inode argument vs. fsnotify_data_inode() are hard to
remember, but we documented them in fsnotify().
The meaning of @inode is that an event should be reported to @inode if inode
has a mark. Surely, one of the reasons for this distinction was fsnotify_name()
which carries the child inode information, but was not traditionally reported
to the child inode mark.
So when I pass positive @inode with FS_RENAME that is all it takes to
report the event on a watching child inode. The only needed minor change in
fsnotify() logic was to move if (mask & FS_RENAME) outside of if (!inode) {}.
All the rest is just tidying up the code.
If we are going to change fsnotify_name() then we need to see if there are
any other cases left where @inode is NULL and fsnotify_data_inode() is positive
(I didn't check) If not, then we may be able to drop the @inode argument.
Thanks,
Amir.
