Return-Path: Received: from mx2.suse.de ([195.135.220.15]:39788 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S2436515AbeKWXlB (ORCPT ); Fri, 23 Nov 2018 18:41:01 -0500 Date: Fri, 23 Nov 2018 13:56:53 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-fsdevel , Mark Fasheh , Chris Mason , Josef Bacik Subject: Re: [PATCH v2 1/5] fsnotify: pass dentry instead of inode when available Message-ID: <20181123125653.GD31877@quack2.suse.cz> References: <20181114174344.17530-2-amir73il@gmail.com> <20181120113206.GG8842@quack2.suse.cz> <20181121125154.GA28182@quack2.suse.cz> <20181122095219.GB9840@quack2.suse.cz> <20181122132640.GB14186@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu 22-11-18 21:42:45, Amir Goldstein wrote: > On Thu, Nov 22, 2018 at 5:18 PM Amir Goldstein wrote: > > > > On Thu, Nov 22, 2018 at 3:26 PM Jan Kara wrote: > > > > > > 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? > > > > > > > That's interesting. I like the simplicity. > > What happens when there are 2 btrfs subvols /subvol1 /subvol2 > > and the application obviously doesn't know about this and does: > > fanotify_mark(fd, FAN_MARK_ADD|FAN_MARK_FILESYSTEM, ... /subvol1); > > statfs("/subvol1",...); > > fanotify_mark(fd, FAN_MARK_ADD|FAN_MARK_FILESYSTEM, ... /subvol2); > > statfs("/subvol2",...); > > > > Now the second fanotify_mark() call just updates the existing super block > > mark mask, but does not change the fsid on the mark, so all events will have > > fsid of subvol1 that was stored when first creating the mark. > > > > fanotify-watch application (for example) hashes the watches (paths) under > > /subvol2 by fid with fsid of subvol2, so events cannot get matched back to > > "watch" (i.e. path). > > > > And when trying to open_by_handle fid with fhandle from /subvol2 > > using mount_fd of /subvol1, I suppose we can either get ESTALE > > or a disconnected dentry, because object from /subvol2 cannot > > have a path inside /subvol1.... > > > > Am I making the issue clear? or maybe I am missing something? > > > > How about this compromise: > > - On fanotify_mark() call statfs() for both dentry and dentry->d_sb->sb_root > - If they produce the same fsid, cache the fsid in the mark > If they do not match invalidate existing cache and never check again > - When encoding fid, use cached fsid if exists, otherwise fallback to > statfs_by_dentry(find_any_alias(inode) ?: inode->i_sb->sb_root) I don't think this is really better - see my previous email. > Maybe better to return -EXDEV from fanotify_mark() on mismatch of fsid > of dentry and dentry->d_sb->sb_root? because it doesn't look like the > FAN_MARK_FILESYSTEM is going to be very useful for btrfs (??). Well, I think FAN_MARK_FILESYSTEM is useful. Path events have no problem, events using FID will work as well when using open_by_handle(), just the reported fsid may be confusing... I'm undecided what is better: returning EXDEV or just silently dropping the other fsid. Honza -- Jan Kara SUSE Labs, CR