Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>, linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [RFC][PATCH] fanotify: introduce filesystem view mark
Date: Thu, 26 Nov 2020 12:10:36 +0100
Message-ID: <20201126111036.GC422@quack2.suse.cz> (raw)
In-Reply-To: <CAOQ4uxiaaQ9X8EBS-bd2DNMdg7ezNoRXCRvu+4idikx67OFbQQ@mail.gmail.com>

On Wed 25-11-20 14:34:16, Amir Goldstein wrote:
> On Wed, Nov 25, 2020 at 1:01 PM Jan Kara <jack@suse.cz> wrote:
> > In fact I was considering for a while that we could even make subtree
> > watches completely unpriviledged - when we walk the dir tree anyway, we
> > could also check permissions along the way. Due to locking this would be
> > difficult to do when generating the event but it might be actually doable
> > if we perform the permission check when reporting the event to userspace.
> > Just a food for thought...
> 
> Maybe... but there are some other advantages to restricting to mount.
> 
> One is that with btrfs subvolumes conn->fsid can actually cache the
> subvolume's fsid and we could remove the restriction of -EXDEV
> error of FAN_MARK_FILESYSTEM on subvolume.

I'm not sure I understand this - do you mean we could support
FAN_MARK_FILESYSTEM_SUBTREE on btrfs subvolumes? I agree with that. I'm
just not sure how subtree watches are related to general
FAN_MARK_FILESYSTEM marks on btrfs...

> Another has to do with performance optimization (see below).

This is a good point. Also we can start with supporting subtree watches
only for mountpoints and if there's a need and reasonable way to do it we can
always expand the support to any directory.

> > > > Also I think this is going to get expensive
> > > > (e.g. imagine each write to page cache having to traverse potentially deep
> > > > tree hierarchy potentially multiple times - once for each subtree). My
> > > > suspicion should be verified with actual performance measurement but if I'm
> > > > right and the overhead is indeed noticeable, I think we'll need to employ
> > > > some caching or so to avoid repeated lookups...
> > >
> > > It's true, but here is a different angle to analyse the overhead - claim:
> > > "If users don't have kernel subtree mark, they will use filesystem mark
> > >  and filter is userspace". If that claim is true, than one can argue that
> > > this is fair - let the listener process pay the CPU overhead which can be
> > > contained inside a cgroup and not everyone else. But what is the cost that
> > > everyone else will be paying in that case?
> > > Everyone will still pay the cost of the fanotify backend callback including
> > > allocate, pack and queue the event.
> > > The question then becomes, what is cheaper? The d_ancestor() traversal
> > > or all the fanotify backend handler code?
> > > Note that the former can be lockless and the latter does take locks.
> >
> > I understand and it's a fair point that queueing of the event is not cheap
> > either so I'd be interested in the numbers. Something like how deep subtree
> > walk is similar to a cost of queueing event. Or similarly checking of how many
> > subtree watches is similarly costly as queueing of one event?
> >
> 
> The cost shouldn't actually be sensitive to the number of subtree watches.
> We should never do more than a single ancestor traversal per event up to
> it's sb root and for each ancestor we should be able to check with O(1) if
> a subtree watch exists on that inode/dentry.

Yeah, if the subtree check will not depend on the number of watched
subtrees on the sb (i.e., single traversal and O(1) checks on each dentry)
then I'm much more relaxed about it ;).

> If we stay with the bind mount restriction then we can use the check
> d_mountpoint() on every ancestor which practically reduces the cost per
> ancestor to zero in most cases.

Yep, even better. Although now that I'm thinking about it, if we use
connector of the directory inode for subtree root to cache info whether it
is a root of a subtree for some watch, we can still do O(1) check for any
dir pretty easily. But d_mountpoint() is even cheaper (more likely to be
cache hot).

> > > I have a pretty good bet on the answer, but as you say only actual performance
> > > benchmarks can tell.
> > >
> > > From my experience, real life fsevent listeners do not listen on FAN_MODIFY
> > > but they listen on FAN_CLOSE_WRITE, because the the former is too noisy.
> >
> > Agreed.
> >
> > > The best case scenario is that users step forward to say that they want to
> > > use fanotify but need the subtree filterting and can provide us with real life
> > > performance measurement of the userspace vs. kernel alternatives (in terms
> > > of penalty on the rest of the system).
> >
> > With the cost of having to go to userspace and there do essentially the same
> > subtree walk as you do in the kernel, I have no doubt what's going to be
> > faster (by orders of magnitude). What I'm somewhat uncertain is whether the
> > subtree check is OK at the time of event generation or whether it should
> > better be moved to the moment when we are about to report the event to
> > userspace (when the cost of the subtree check goes to the process
> > interested in the event which is fine - but as you properly note we already
> > had to pay the cost of queueing the event so it isn't clear this is a win
> > even for the processes generating events).
> >
> 
> I think the only semantics that make sense are:
> 
> * If all object's present and past ancestors were always under subtree -
>    guaranty to get an event
> * If all object's present and past ancestors were never under subtree -
>    guaranty to not get an event
> * Otherwise - undefined
> 
> So I think it is ok to check for subtree on event generation time.

I meant that I'm uncertain whether the check is OK at the time of event
generation due to performance. Regarding semantics, I agree that both
options are fine. But after somewhat optimizing the subtree check as you
describe above, I'm not much concerned anymore. So the only reason I'd see
for moving the checks to event report time would be if we decided to do
more fancy stuff like permission checks (which I'd find really neat as that
would allow unpriviledged subtree watching and there are *lots* of users
for that from desktop world - but that's the next level ;))...

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

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-09 18:00 Amir Goldstein
2020-11-10  5:07 ` Amir Goldstein
2020-11-17  7:09 ` [fanotify] a23a7dc576: unixbench.score -3.7% regression kernel test robot
2020-11-24 13:49 ` [RFC][PATCH] fanotify: introduce filesystem view mark Jan Kara
2020-11-24 14:47   ` Amir Goldstein
2020-11-25 11:01     ` Jan Kara
2020-11-25 12:34       ` Amir Goldstein
2020-11-26 11:10         ` Jan Kara [this message]
2020-11-26 11:50           ` Amir Goldstein
2020-11-26  3:42       ` Amir Goldstein
2020-11-26 11:17         ` Jan Kara
2021-04-28 18:28           ` Amir Goldstein
2021-05-03 16:53             ` Jan Kara
2021-05-03 18:44               ` Amir Goldstein
2021-05-05 12:28                 ` Jan Kara
2021-05-05 14:24                   ` Christian Brauner
2021-05-05 14:42                     ` Amir Goldstein
2021-05-05 14:56                       ` Christian Brauner
2021-05-10 10:13                     ` Jan Kara
2021-05-10 11:37                       ` Amir Goldstein
2021-05-10 14:21                         ` Jan Kara
2021-05-10 15:08                           ` Amir Goldstein
2021-05-10 15:27                             ` Jan Kara
2021-05-12 13:07                             ` Christian Brauner
2021-05-12 13:34                               ` Jan Kara
2021-05-12 16:15                                 ` Christian Brauner
2021-05-12 15:26                         ` Christian Brauner
2021-05-13 10:55                           ` Jan Kara
2021-05-14 13:56                             ` Christian Brauner
2021-05-15 14:28                               ` Amir Goldstein
2021-05-17  9:09                                 ` Jan Kara
2021-05-17 12:45                                   ` Amir Goldstein
2021-05-17 13:07                                     ` Jan Kara
2021-05-18 10:11                                 ` Christian Brauner
2021-05-18 16:02                                   ` Amir Goldstein
2021-05-19  9:31                                     ` Christian Brauner
2021-05-12 16:11                         ` Christian Brauner
2021-05-05 13:25               ` Christian Brauner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201126111036.GC422@quack2.suse.cz \
    --to=jack@suse.cz \
    --cc=amir73il@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git