Re: [PATCH v2 1/5] fsnotify: pass dentry instead of inode when available

[Jan Kara <jack@suse.cz>]

On Tue 20-11-18 16:36:31, Amir Goldstein wrote:
> On Tue, Nov 20, 2018 at 1:32 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 14-11-18 19:43:40, Amir Goldstein wrote:
> > > Define a new data type to pass for event FSNOTIFY_EVENT_DENTRY
> > > and use it whenever a dentry is available instead of passing
> > > it's ->d_inode as data type FSNOTIFY_EVENT_INODE.
> > >
> > > None of the current fsnotify backends make use of the inode data
> > > with data type FSNOTIFY_EVENT_INODE - only the data of type
> > > FSNOTIFY_EVENT_PATH is ever used, so this change has no immediate
> > > consequences.
> > >
> > > Soon, we are going to use the dentry data type to support more
> > > events with fanotify backend.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > One general coment: Audit is using FSNOTIFY_EVENT_INODE and this change is
> > going to break it. So it needs updating as well.
> >
> 
> Ouch! I should really add the audit test suite to my testing routine...
> 
> > Also I'd prefer a more justified selection of dentry events than 'those
> > where we can do it'. Because eventually that set will translate to events
> > available to userspace in fanotify and that should be a reasonably
> > consistent set. So here I suspect you target at directory modification
> > events (you call them filename events in the next patch). So just define
> > which FS_<foo> events these exactly are and convert exactly those event
> > helpers to dentry ones...
> >
> 
> It is not accurate that I target only "directory modification" events.
> I also target FS_ATTRIB and in the future also FS_DELETE_SELF,
> both applicable to files as well as directories.
> 
> But when I think about it... fanotify does not really need the dentry
> information,
> so I might just be able to do without FSNOTIFY_EVENT_DENTRY
> and avert the questions about stability of dentry->d_inode

OK, that would certainly make things simpler.

> fanotify_dentry branch uses the dentry information in 3 occasions:
> 
> 1. if (d_is_dir(dentry) in fanotify_group_event_mask()
>     This could be converted to S_ISDIR(inode->i_mode)

Sure.

> 2. exportfs_encode_fh(dentry,... in fanotify_alloc_fid()
>     Can be converted to exportfs_encode_inode_fh(inode,...

If you have the parent inode then yes. In lot of cases we do have it, not
sure if in all of them (but likely yes so that we can do proper
FS_EVENT_ON_CHILD) handling.

> 3. statfs_by_dentry(dentry,... in fanotify_alloc_fid()
>     Can be converted to statfs_by_dentry(inode->i_sb->sb_root,...

This might be problematic e.g. with btrfs subvolumes which have each
different fsid so the fsid-handle pair might be actually invalid or even
point to a file with different contents. Maybe we could just store the
fsid in the fsnotify_mark when it is created and use it when generating
events? That would also get rid of the overhead of calling statfs for each
generated event which I don't like...

> I will leave the patch to annotate "directory change" events,
> but the rest of this prep series can be discarded.
> 
> Does that sound reasonable?

Yes.

								Honza

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