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.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_PASS,URIBL_BLOCKED, USER_AGENT_MUTT 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 ABE52C43387 for ; Thu, 10 Jan 2019 09:22:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 85681214C6 for ; Thu, 10 Jan 2019 09:22:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727815AbfAJJWP (ORCPT ); Thu, 10 Jan 2019 04:22:15 -0500 Received: from mx2.suse.de ([195.135.220.15]:56366 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727214AbfAJJWP (ORCPT ); Thu, 10 Jan 2019 04:22:15 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 08F66AD81; Thu, 10 Jan 2019 09:22:13 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id C367B1E1519; Thu, 10 Jan 2019 10:22:12 +0100 (CET) Date: Thu, 10 Jan 2019 10:22:12 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-fsdevel , linux-api@vger.kernel.org Subject: Re: [PATCH v4 00/15] fanotify: add support for more event types Message-ID: <20190110092212.GB15790@quack2.suse.cz> References: <20181202113826.32133-1-amir73il@gmail.com> <20190104110033.GK22409@quack2.suse.cz> <20190109140235.GH15397@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org 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. Generally the ISDIR flag handling might deserve a larger cleanup but I don't want to mix it into your series as that's big enough as is. Honza -- Jan Kara SUSE Labs, CR