Re: Fanotify API - Tracking File Movement

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

 



> > @@ -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.



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux