Re: [PATCH v3 08/13] fanotify: enable FAN_REPORT_FID init flag

[Jan Kara <jack@suse.cz>]

On Thu 29-11-18 13:03:03, Amir Goldstein wrote:
> On Thu, Nov 29, 2018 at 11:46 AM Jan Kara <jack@suse.cz> wrote:
> > > +     err = vfs_statfs(path, &stat);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     if (!stat.f_fsid.val[0] && !stat.f_fsid.val[1])
> > > +             return -ENODEV;
> > > +
> > > +     /*
> > > +      * Make sure path is not inside a filesystem subvolume (e.g. btrfs)
> > > +      * which uses a different fsid than sb root.
> > > +      */
> > > +     err = statfs_by_dentry(path->dentry->d_sb->s_root, &root_stat);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     if (root_stat.f_fsid.val[0] != stat.f_fsid.val[0] ||
> > > +         root_stat.f_fsid.val[1] != stat.f_fsid.val[1])
> > > +             return -EXDEV;
> >
> > I think inode watches in subvolumes are actually fine? The same fs object
> > is going to get different struct inode for different subvolumes if I
> > remember right. So there won't be any surprises with unexpected fsid being
> > reported.
> >
> > Also mount watches are actually fine for subvolume as different subvolumes
> > appear as different mountpoints, don't they? And I think implementation
> > that would have different fsid for inodes in the same mountpoint would be
> > indeed very weird. So again no problem with fsid mismatch.
> >
> > So we need this check only for superblock watches.
> >
> 
> Not so simple (or is it?).
> If a group has inode, mount and filesystem marks, not all added at the
> same time.
> When event on an object that is associated with all the above marks,
> which cached fsid should be used in the report?
> Naturally, it makes sense to prefer to more accurate fsid of mount/inode
> over the broader fsid of filesystem. Right?
> But what happens when mount/inode marks are removed?
> Or if filesystem has events in the mask that inode/mount do not?
> Then the same object reports events with different fsid depending
> on the type of event and time it took place (which marks existed).
> 
> Not a good situation to get ourselves into.
> 
> The simple way out of this is: we do not support FAN_REPORT_FID
> on marks using path that is not relative to main volume. period.
> 
> Considering the fact that FAN_REPORT_FID is mainly indented to
> enable reporting directory modification events and mount marks
> are not supported with reporting directory modification events, we
> only loose the ability to watch modification on selective directories
> inside btrfs subvolume.
> 
> I also don't like the fact that I disabled filesystem watch over tmpfs,
> because for the case of watching a single filesystem or single
> directory, which is quite a common case, we don't need fsid
> to be non-zero and we don't care if it mismatches with s_root fsid.
> 
> A solution I was contemplating was to allow zero fsid and non
> root fsid as long as it is the only sb watched by the group, so
> for non unique fsid:
> - store group->sb and group->fsid
> - return -EXDEV for an attempt to add mark from a different sb
> (no matter if it is inode/mount/sb mark)
> - when trying to add mark with zero or non root fsid (common case)
> set group->sb to a special value so no fs will match it and then
> attempt to add any mark with zero/non-root fsid will fail
> 
> This is something that is quite easy for me to implement and less
> easy to document the expected behavior.
> I donno, maybe:
> EXDEV    watching several filesystems and either new mark or existing marks
>                  are on filesystems with non unique fsid
> 
> The easy way out of it for me was: no support for FAN_REPORT_FID
> on btrfs subvolumes at the moment - it could be added with restrictions
> in the future.
> 
> Do you have a different view of the problem than mine?

Yeah, OK, you're right the semantics isn't really obvious. So I'm fine with
going for EXDEV now and we can open that can of worms later.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR