From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb1-f194.google.com ([209.85.219.194]:33335 "EHLO mail-yb1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728102AbeK2Blg (ORCPT ); Wed, 28 Nov 2018 20:41:36 -0500 Received: by mail-yb1-f194.google.com with SMTP id i78-v6so10696798ybg.0 for ; Wed, 28 Nov 2018 06:39:43 -0800 (PST) MIME-Version: 1.0 References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-2-amir73il@gmail.com> <20181128125903.GE15604@quack2.suse.cz> In-Reply-To: <20181128125903.GE15604@quack2.suse.cz> From: Amir Goldstein Date: Wed, 28 Nov 2018 16:39:31 +0200 Message-ID: Subject: Re: [PATCH v3 01/13] fsnotify: annotate directory entry modification events To: Jan Kara Cc: Matthew Bobrowski , linux-fsdevel Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Nov 28, 2018 at 2:59 PM Jan Kara wrote: > > On Sun 25-11-18 15:43:40, Amir Goldstein wrote: > > "dirent" events are referring to events that modify directory entries, > > such as create,delete,rename. Those events should always be reported > > on a watched directory, regardless if FS_EVENT_ON_CHILD is set > > on the watch mask. > > > > fsnotify_nameremove() and fsnotify_move() were modified to no longer > > set the FS_EVENT_ON_CHILD event bit. This is a semantic change to > > align with the "dirent" event definition. It has no effect on any > > existing backend, because dnotify, inotify and audit always requets the > > child events and fanotify does not get the delete,rename events. > > > > The fsnotify_dirent() helper is used instead of fsnotify_parent() to > > report a dirent event to dentry->d_parent without FS_EVENT_ON_CHILD > > and regardless if parent has the FS_EVENT_ON_CHILD bit set. > > > > ALL_FSNOTIFY_DIRENT_EVENTS defines all the dirent event types and > > those event types have been extracted out of FS_EVENTS_POSS_ON_CHILD. > > > > That means for a directory with an inotify watch and only dirent > > events in the mask (i.e. create,delete,move), all children dentries > > will no longer have the DCACHE_FSNOTIFY_PARENT_WATCHED flag set. > > This will allow all events that happen on children to be optimized > > away in __fsnotify_parent() without the need to dereference > > child->d_parent->d_inode->i_fsnotify_mask. > > > > Since the dirent events are never repoted via __fsnotify_parent(), > > this results in no change of logic, but only an optimization. > > > > Signed-off-by: Amir Goldstein > > --- > > include/linux/fsnotify.h | 43 +++++++++++++++++++++++++------- > > include/linux/fsnotify_backend.h | 36 +++++++++++++++----------- > > 2 files changed, 55 insertions(+), 24 deletions(-) > > The patch looks good. Just one question and two nits below. > > > +static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry, > > + __u32 mask) > > +{ > > + struct dentry *parent = NULL; > > + int ret; > > + > > + if (!dir) { > > + parent = dget_parent(dentry); > > + dir = d_inode(parent); > > + } > > + > > + ret = fsnotify(dir, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE, > > + dentry->d_name.name, 0); > > + > > + dput(parent); > > + return ret; > > There's one more feature fsnotify_parent() provides - it calls > take_dentry_name_snapshot() before calling into fsnotify and uses > snapshotted name. I think you need to do the same here to provide the same > protection for fsnotify_nameremove, don't you? Or maybe you don't since > generally directory i_rwsem is held when unlinking and so dentry name cannot > change but then it would be good to assert that? Who knows what some weird > fs is doing... > Right.. I will add that assert and at the same time, for the same reason, this helper doesn't need to dget_parent(), because d_parent should also be stable. > > +/* > > + * This is a list of all events that may get sent to a parent based on fs event > > ^^^ Line too long. Heh?? checkpatch did not complain. It's 79 chars. > > > +#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS| \ > ^^^ space here > My patch did not change indentation AFAIK. The new definition added is with tab (ALL_FSNOTIFY_DIRENT_EVENTS) but some of the existing definitions, even ones moved around are with space(s). Do you want me to change that? Thanks, Amir.