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

[Jan Kara <jack@suse.cz>]

On Thu 22-11-18 14:36:35, Amir Goldstein wrote:
> > > Regardless, IIUC, btrfs_statfs() returns an fsid which is associated with
> > > the single super block struct, so all dentries in all subvolumes will
> > > return the same fsid: btrfs_sb(dentry->d_sb)->fsid.
> >
> > This is not true AFAICT. Looking at btrfs_statfs() I can see:
> >
> >         buf->f_fsid.val[0] = be32_to_cpu(fsid[0]) ^ be32_to_cpu(fsid[2]);
> >         buf->f_fsid.val[1] = be32_to_cpu(fsid[1]) ^ be32_to_cpu(fsid[3]);
> >         /* Mask in the root object ID too, to disambiguate subvols */
> >         buf->f_fsid.val[0] ^=
> >                 BTRFS_I(d_inode(dentry))->root->root_key.objectid >> 32;
> >         buf->f_fsid.val[1] ^=
> >                 BTRFS_I(d_inode(dentry))->root->root_key.objectid;
> >
> > So subvolume root is xored into the FSID. Thus dentries from different
> > subvolumes are going to return different fsids...
> >
> 
> Right... how could I have missed that :-/
> 
> I do not want to bring back FSNOTIFY_EVENT_DENTRY just for that
> and I saw how many flaws you pointed to in $SUBJECT patch.
> Instead, I will use:
> statfs_by_dentry(d_find_any_alias(inode) ?: inode->i_sb->sb_root,...

So what about my proposal to store fsid in the notification mark when it is
created and then use it when that mark results in event being generated?
When mark is created, we have full path available, so getting proper fsid
is trivial. Furthermore if the behavior is documented like that, it is
fairly easy for userspace to track fsids it should care about and translate
them to proper file descriptors for open_by_handle().

This would get rid of statfs() on every event creation (which I don't like
much) and also avoids these problems "how to get fsid for inode". What do
you think?

								Honza

> So we are only left with the corner case of fsnotify_inoderemove()
> on btrfs and the like. I checked that for all the rest of fsnotify hooks
> an alias is guarantied to exist at the time of the hook.
> 
> I could go with either of two options:
> 1. No support for FAN_DELETE_SELF as my v2 patch set already does
> 2. Best effort support for FAN_DELETE_SELF - it works for non-btrfs
>     and if application listens on a single filesystem and ignores fsid
>     (document this culprit).
> 
> What do you think?
> 
> Thanks,
> Amir.
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR