From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5E971C433E0 for ; Thu, 16 Jul 2020 07:40:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 26F12206C1 for ; Thu, 16 Jul 2020 07:40:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="dUwgFTCA" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726069AbgGPHkF (ORCPT ); Thu, 16 Jul 2020 03:40:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40714 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725867AbgGPHkF (ORCPT ); Thu, 16 Jul 2020 03:40:05 -0400 Received: from mail-io1-xd41.google.com (mail-io1-xd41.google.com [IPv6:2607:f8b0:4864:20::d41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 27084C061755 for ; Thu, 16 Jul 2020 00:40:05 -0700 (PDT) Received: by mail-io1-xd41.google.com with SMTP id v8so5047079iox.2 for ; Thu, 16 Jul 2020 00:40:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=5CwZCbVxmRQeO8n0yUch6Ir4FmibXgjeDMMpPhV31ZE=; b=dUwgFTCAPN+Mn59xdtjU9tuv1Cj0IyBuiu4fLcNjPfi0WWXE0HtiJPqRa7M9G7hQ/b qKQ9tf/DEJU7fN8PsyKRwHdcNvRONOCWTrypbzI/euc0ZI3wyoCjIxaedaH8UC4mzSNd 5bZWy4QJhiqEaTohjwf8YZiOQX0Em51Ohxq2zrEV57Cb3QAuk4f9bsVY4K9UUHyEJmb0 Qc/C83WmVuZg+tIFJcWSLNGKicNAuFDj5AX1HIA/JqEC+/i87rzujEOlTpKKZXvOY/tS Mj0Z3XNF70gANXpnzjYp1aZ/1n6UhcNkXlzsnLfsC0SBaGIo9Cp9XsMjNfJbDg4ov+DA jBTg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=5CwZCbVxmRQeO8n0yUch6Ir4FmibXgjeDMMpPhV31ZE=; b=i1FGTfzQChIAPZMs3ohb68EA/ry6w/kdBXI9OQ6ioXsqaBmUpYrUutQu93XSsPEDPo Pj/qTYaWJa7ItKy/uI3p9AqJh7GN9ZHN83St6MFzgOPQT+dMOczv70yVDnImkVuKLhNp epIh53Kf4QiTkn+CjfIoIje34Pv9rvA5/w0LikE6EoGRF1+IdwqH12AlvOYYAKL9ky41 9CTehIWom0AQsWTFnC9hf3jhjhr8wlzZlgHnumQZEYNvIAN40N7F/VA+ehVC1rFSzwU5 qbth4L4ids0mIP6NNRk+aTZPfclRSwT5pwlYZVCVNt4/94U4IQJCMZAvTE6RuRK3r6lT w+lg== X-Gm-Message-State: AOAM531PbTGCUR73xy70FuOzI1Iuqx54uB8jnkdpfgMyvPVyy32p+ysO 4nv1F1hHHlxj1iRf3bxDyx9mWmL9BC7zMY7Tm5lNoTza X-Google-Smtp-Source: ABdhPJwQh4+tADzgi5BLbnYnlCJ09mBpKzIZnY+e5r9a0VNijiVMtSsdbLW+G0+FpTvr5VfW9cH66riGgIjha+hFA3U= X-Received: by 2002:a02:4b81:: with SMTP id q123mr2096817jaa.120.1594885204280; Thu, 16 Jul 2020 00:40:04 -0700 (PDT) MIME-Version: 1.0 References: <20200702125744.10535-1-amir73il@gmail.com> <20200702125744.10535-4-amir73il@gmail.com> <20200714103455.GD23073@quack2.suse.cz> <20200715170937.GQ23073@quack2.suse.cz> In-Reply-To: From: Amir Goldstein Date: Thu, 16 Jul 2020 10:39:53 +0300 Message-ID: Subject: Re: [PATCH v4 03/10] fsnotify: send event to parent and child with single callback To: Jan Kara Cc: Matthew Bobrowski , linux-fsdevel Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Thu, Jul 16, 2020 at 9:38 AM Amir Goldstein wrote: > > On Wed, Jul 15, 2020 at 8:42 PM Amir Goldstein wrote: > > > > On Wed, Jul 15, 2020 at 8:09 PM Jan Kara wrote: > > > > > > On Tue 14-07-20 14:54:44, Amir Goldstein wrote: > > > > On Tue, Jul 14, 2020 at 1:34 PM Jan Kara wrote: > > > > > > > > > > On Thu 02-07-20 15:57:37, Amir Goldstein wrote: > > > > > > Instead of calling fsnotify() twice, once with parent inode and once > > > > > > with child inode, if event should be sent to parent inode, send it > > > > > > with both parent and child inodes marks in object type iterator and call > > > > > > the backend handle_event() callback only once. > > > > > > > > > > > > The parent inode is assigned to the standard "inode" iterator type and > > > > > > the child inode is assigned to the special "child" iterator type. > > > > > > > > > > > > In that case, the bit FS_EVENT_ON_CHILD will be set in the event mask, > > > > > > the dir argment to handle_event will be the parent inode, the file_name > > > > > > argument to handle_event is non NULL and refers to the name of the child > > > > > > and the child inode can be accessed with fsnotify_data_inode(). > > > > > > > > > > > > This will allow fanotify to make decisions based on child or parent's > > > > > > ignored mask. For example, when a parent is interested in a specific > > > > > > event on its children, but a specific child wishes to ignore this event, > > > > > > the event will not be reported. This is not what happens with current > > > > > > code, but according to man page, it is the expected behavior. > > > > > > > > > > > > Signed-off-by: Amir Goldstein > > > > > > > > > > I like the direction where this is going. But can't we push it even a bit > > > > > further? I like the fact that we now have "one fs event" -> "one fsnotify() > > > > > call". Ideally I'd like to get rid of FS_EVENT_ON_CHILD in the event mask > > > > > because it's purpose seems very weak now and it complicates code (and now > > > > > > > > Can you give an example where it complicates the code? > > > > Don't confuse this with the code in fanotify_user.c that subscribes for > > > > events on child/with name. > > > > > > I refer mostly to the stuff like: > > > > > > /* An event "on child" is not intended for a mount/sb mark */ > > > if (mask & FS_EVENT_ON_CHILD) > > > ... > > > > > I need to explain something that was not an obvious decision for me. > > When sending the same event on two inodes marks I considered a few options: > > 1. TYPE_INODE is the mark on the object referred to in data > TYPE_PARENT is the mark on the parent if event is sent to a watching > parent or to sb/mnt/child with parent/name info > 2. TYPE_CHILD is the mark on the object referred to in data > TYPE_INODE is the mark on the fsnotify to_tell inode if not same as data > 3. TYPE_INODE is the mark on the fsnotify to_tell inode > TYPE_CHILD is the mark on the object referred to in data if it is > not to_tell > > The first option with TYPE_PARENT would require changing audit > and dnotify to look at TYPE_PARENT mark in addition to TYPE_INODE > mark, so it adds more friction and I ruled it out. > > I think you had option #2 in mind when reading the code, but I went > for option #3. > There is a minor difference between them related to how we deal with the case > that the parent is watching and the case that only the child is watching. > > If the parent is not watching (and child/sb/mnt not interested in name) we do > not snapshot the name and do not set the ON_CHILD flag in the mask. > In that case, should we add the child mark as TYPE_INODE or TYPE_CHILD? > > I chose TYPE_INODE because this meant I did not have to change audit/dnotify > for that case. I didn't even care to look if they needed to be changed or not, > just wanted to keep things as they were. > > Looking now, I see that dnotify would have needed to check TYPE_CHILD to > get FS_ATTRIB event on self. > > It looks like audit would not have needed to change because although they set > FS_EVENT_ON_CHILD in mask, none of the events they care about are > "possible on child": > #define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\ > FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT) > #define AUDIT_FS_EVENTS (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\ > FS_MOVE_SELF | FS_EVENT_ON_CHILD) > > Having written that decision process down made me realize there is a bug in > my unified inotify event handler implementation - it does not clear > FS_EVENT_ON_CHILD when reporting without name. > > It is interesting to note that the result of sending FS_OPEN only to a watching > child to inotify_handle_event() is the same for design choices #2 and #3 above. > But the bug fix of clearing FS_EVENT_ON_CHILD when reporting without name > would look different depending on said choice. > > Since I had to change inotify handler anyway, I prefer to stick with my choice > and fix inotify handler using goto notify_child which is a bit uglier, > instead of > having to adapt dnotify to choice #2. > It turns out it's the other way around. inotify handler has no bug (FS_EVENT_ON_CHILD is not exposed to the user) just a confusing comment, so I will fix that. But dnotify does have a bug - it also needs to be taught about the unified event so that DN_ATTRIB event can be reported twice on both parent dir and child subdir if both are watching. Alas, we have no test coverage for dnotify in LTP... This means that we could also go with choice #2. But we can also make that internal change later on, because it does not impact the logic. Thanks, Amir.