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=-6.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_PASS,URIBL_BLOCKED autolearn=ham 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 E2C89C43387 for ; Thu, 10 Jan 2019 09:50:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AB2D3214DA for ; Thu, 10 Jan 2019 09:50:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="JHdLJHQS" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728054AbfAJJuo (ORCPT ); Thu, 10 Jan 2019 04:50:44 -0500 Received: from mail-yw1-f65.google.com ([209.85.161.65]:37235 "EHLO mail-yw1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727916AbfAJJun (ORCPT ); Thu, 10 Jan 2019 04:50:43 -0500 Received: by mail-yw1-f65.google.com with SMTP id h193so4144368ywc.4; Thu, 10 Jan 2019 01:50:43 -0800 (PST) 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=9O/7esJ/LOlI3jZWd6JbtEJXxXuo9VeyBgM7b2yOyI0=; b=JHdLJHQSojF1akEWu1VfLD/S/KzU/91gXRBWXXKYJWSrNliFodv0KJCNMDg0tIWHAf 7RGpy7w/FPP7p9qtuK2ZORRfdZAvYxjLD4fsho1Lq/ZkzrKiPBmQKV5IU6LAmM/BuI/B VMNQeNrj6qtTLlEEgXUe5uCINHRGLpgLjdM6TGQbpgPgt0zomhI810VE6H6KOkmIcRjz yENNyGPTk//jQ4qP1ESzNzN4qMeEHQKhDRQ1Hj1cUiq4dU1t+5tu7enx2LhK+lZQMAPA 3ey9NQcdShIPbXOOSAcyAZVV0HDpTjqMhzu5MPYHmm+5kuXDfblSJHb6y+4Nni/CYP99 k3uw== 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=9O/7esJ/LOlI3jZWd6JbtEJXxXuo9VeyBgM7b2yOyI0=; b=BIJS51YmHoet8c6X6uznmt2/Ru6/bzNdl/5l5B2kEVPZyaXgsMAmYFYjI6T2t819bD 0vBcu5mXoH5AyxhCQuc65S7PE1g7kfiA26aTAVu1zQg2Xxaw4yVzRfEFsQNyofg1luwU S4GFVgkQ3hQGtQ23I2689x2mNblTdimFGMtt4qsEc6J+u1Pp3SnwSlfnpwHh5wkM/X+2 9pHTHFrDdEth7gga8uPg84mj75sbkb7T98j7/E7I21XgJJSb7mfvOWqxWeY5EAnt+ebq cbRCzUdnPJ4NqwU/N/yhUDUviP99ZmZ9iJ7sGbnNpIXct9K9+Cq4xyvm0K5AUmNMg3kD qM9Q== X-Gm-Message-State: AJcUukdomiW5Y2yNVXn2RVgctKYplJkprEqDJytf9CgJDodAFiox5stk DGMNCIROtq7w4qs9i7Nl+G4VgbQUoGHsiikXUao= X-Google-Smtp-Source: ALg8bN4hHOBDxCsmL6L78xzAphHoOR2NJretB/J/TwVFEDVJwUCLA1t1tWsMlrWVDKf09z3pDsAmnyCmBE9LDtgi11M= X-Received: by 2002:a81:34d3:: with SMTP id b202mr8965214ywa.241.1547113842312; Thu, 10 Jan 2019 01:50:42 -0800 (PST) MIME-Version: 1.0 References: <20181202113826.32133-1-amir73il@gmail.com> <20190104110033.GK22409@quack2.suse.cz> <20190109140235.GH15397@quack2.suse.cz> <20190110092212.GB15790@quack2.suse.cz> In-Reply-To: <20190110092212.GB15790@quack2.suse.cz> From: Amir Goldstein Date: Thu, 10 Jan 2019 11:50:30 +0200 Message-ID: Subject: Re: [PATCH v4 00/15] fanotify: add support for more event types To: Jan Kara Cc: Matthew Bobrowski , linux-fsdevel , linux-api@vger.kernel.org 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, Jan 10, 2019 at 11:22 AM Jan Kara wrote: > > On Thu 10-01-19 09:49:24, Amir Goldstein wrote: > > On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein wrote: > > > > > > On Wed, Jan 9, 2019 at 4:02 PM Jan Kara wrote: > > > > > > > > On Mon 07-01-19 09:46:37, Amir Goldstein wrote: > > > > > On Fri, Jan 4, 2019 at 1:00 PM Jan Kara wrote: > > > > > > On Sun 02-12-18 13:38:11, Amir Goldstein wrote: > > > > > > > This is the 4th revision of patch series to add support for filesystem > > > > > > > change monitoring to fanotify. > > > > > > > It incorporates the changes you requested in review of v3 FAN_REPORT_FID > > > > > > > patches. > > > > > > > The complete work is available on fanotify_dirent branch [1] on my tree. > > > > > > > > > > > > > > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and > > > > > > > dirent modification events is demonstrated with a prototype of global > > > > > > > filesystem monitor based on inotify-tools [2]. > > > > > > > > > > > > > > In your review of v3 patched you only got as far as patch v3 9/13. > > > > > > > Because this patch marks the end of the FAN_REPORT_FID sub series, > > > > > > > I found it best to re-post the entire series with the changes you > > > > > > > requested thus far. For convenience of review, I pushed branches > > > > > > > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you > > > > > > > reviewed so far and its re-worked version. > > > > > > > > > > > > > > One thing that we discussed and I did NOT do is move struct file_handle > > > > > > > to uapi headers. This got complicated due to existing definitions in > > > > > > > glibc header files and I realized we could do without it. > > > > > > > > > > > > > > I have added the vfs_get_fsid() helper as you requested, but since it > > > > > > > wasn't required by the patch set, I added it as two new cleanup patches > > > > > > > at the end of the FAN_REPORT_FID series, so you will be able to stage > > > > > > > the feature with or without the VFS change. > > > > > > > > > > > > So overall the series looks very good. I've had only some smaller comments > > > > > > / disagreements. So once we settle those please resend the series and I'll > > > > > > pick it up to my tree. > > > > > > > > > > > > > > > > Jan, > > > > > > > > > > I addressed all your comments, rebased on v5.0-rc1 and pushed to: > > > > > https://github.com/amir73il/linux/commits/fanotify_dirent > > > > > > > > > > When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series. > > > > > The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir > > > > > events (as you suggested) and FAN_ONDIR is reported as output flag > > > > > for FAN_REPORT_FID group for all events that occur on directories > > > > > and never reported to non FAN_REPORT_FID group. > > > > > > > > Agreed. But thinking about it a bit more and looking at your patch on > > > > Github I think your solution doesn't quite work in presence of event > > > > merging since an event with FAN_ONDIR can get merged with event without > > > > FAN_ONDIR, can't it? That seems to be generally a problem with > > > > should_merge() for direntry events that it doesn't take ISDIR flag into > > > > account... > > > > > > > > > > Right... good catch! will fix should_merge(). > > > Do you want me to re-post the series or just push to Github? > > > > > > > In the mean while, I pushed the fix to branch fanotify_dirent. > > A branch with the same name in LTP tree has a WIP dirent events > > test that includes the test for expected merge behavior. > > Matthew intends to work this test into shape. > > OK, thanks! > > > I did notice another wrinkle. > > There is a group of events that this patch set adds support to > > that are not dirent events, namely: DELETE_SELF, MOVE_SELF > > and ATTRIB. > > > > The first two never carry the ISDIR flag (in inotify as well). > > ATTRIB does usually carry the ISDIR flag, except for the odd case > > of fsnotify_link_count(target) being called on rename over an empty > > directory. > > > > If we add FS_ISDIR in those hooks, we risk breaking inotify apps. > > If we don't do anything, we will get inconsistent behavior w.r.t > > FAN_ONDIR (especially relevant for XXX_SELF). > > I see. So how about adding ISDIR flag properly to all events and just mask > it out in inotify_handle_event() for bug-to-bug compatibility? I think we > can start sending ISDIR flag properly for fsnotify_link_count() from because > that is unlikely to surprise anybody given other ATTRIB events can have it. > Because the fact that ISDIR flag is missing looks like a bug to me. > Agreed. I will work this into the series and post v5. TBH, I think that the ATTRIB event just before renaming over an empty dir is grossly uninteresting. Not to mention that there is no such event on rmdir() nor on unlink(), even if nlink is decremented to non zero. So I would rather if we just removed the fsnotify_link_count(target) call altogether or add the missing calls, but I won't do that now. Thanks, Amir.