From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:37074 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727979AbeK2AAk (ORCPT ); Wed, 28 Nov 2018 19:00:40 -0500 Date: Wed, 28 Nov 2018 13:59:03 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v3 01/13] fsnotify: annotate directory entry modification events Message-ID: <20181128125903.GE15604@quack2.suse.cz> References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-2-amir73il@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181125134352.21499-2-amir73il@gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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... > +/* > + * This is a list of all events that may get sent to a parent based on fs event ^^^ Line too long. > +#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS| \ ^^^ space here Honza -- Jan Kara SUSE Labs, CR