From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f68.google.com ([209.85.221.68]:39625 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725993AbeLFO6M (ORCPT ); Thu, 6 Dec 2018 09:58:12 -0500 Received: by mail-wr1-f68.google.com with SMTP id t27so787117wra.6 for ; Thu, 06 Dec 2018 06:58:10 -0800 (PST) From: Amir Goldstein To: Jan Kara Cc: Matthew Bobrowski , linux-fsdevel@vger.kernel.org, Miklos Szeredi , Al Viro Subject: [PATCH v4 16/15] fsnotify: relax WARN_ON() in fsnotify_nameremove() Date: Thu, 6 Dec 2018 16:58:03 +0200 Message-Id: <20181206145803.32228-1-amir73il@gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: After commit "fsnotify: annotate directory entry modification events" fsnotify_nameremove() requires that dentry->d_name and dentry->d_parent are stable. fsnotify_nameremove() is usually called from d_delete() from filesystem ->unlink(), ->rmdir(), ->rename() ops under parent vfs inode lock, so said commit adds a WARN_ON() if parent inode is not locked. fsnotify_nameremove() is also called from nfs_complete_sillyrename(), also under parent vfs inode lock. While the requirement of stable d_name and d_parent seems to be true for all in-tree callers, the WARN_ON() was not true is some cases. The following functions call d_delete() from pseudo filesystems without parent inode lock, but those filesystems have no rename() op, so this is not a concern: ffs_epfiles_destroy(), rpc_gssd_dummy_depopulate(), devpts_pty_kill(), efivarfs_file_write(), __ns_get_path(). In the last case, the dentry is a pseudo dentry that should not generate any event. The following functions call d_delete() from clustered filesystems without parent inode lock, but those filesystems use clustered locks to synchronize d_delete() with d_move(): ceph_fill_trace(), ocfs2_dentry_convert_worker(). Relax the WARN_ON() to accommodate for these two special cases: - fs with no dir rename() op - fs that does d_move() instead of vfs Move the WARN_ON() into fsnotify_nameremove() where it matters. Cc: Miklos Szeredi Cc: Al Viro Signed-off-by: Amir Goldstein --- Jan, I have completed my research about d_delete() callers and my conclusions are summarized in this patch. Should you decide to accept the result, you would probably want to squash this with patch 1. I am guessing you would feel more cozy with dget_parent() and take_dentry_name_snapshot(), but let's see if we can get an ack on my conclusions from either Al or Miklos first. Thanks, Amir. include/linux/fsnotify.h | 46 ++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index 14e1f43c38c9..d20c92c3cdeb 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -22,23 +22,10 @@ * * Unlike fsnotify_parent(), the event will be reported regardless of the * FS_EVENT_ON_CHILD mask on the parent inode. - * - * When called with NULL @dir (from fsnotify_nameremove()), the dentry parent - * inode is used as the inode to report the event to. */ static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry, __u32 mask) { - if (!dir) - dir = d_inode(dentry->d_parent); - - /* - * This helper assumes d_parent and d_name are stable. It must be true - * when called from fsnotify_create()/fsnotify_mkdir(). Less sure about - * all callers that get here from d_delete() => fsnotify_nameremove(). - */ - WARN_ON(!inode_is_locked(dir)); - return fsnotify(dir, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0); } @@ -158,15 +145,46 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt) /* * fsnotify_nameremove - a filename was removed from a directory + * + * Requires that d_parent and d_name are stable. + * + * Called from d_delete() and nfs_complete_sillyrename(). + * The latter is called from nfs client ->unlink() ->rmdir() ->rename() + * under parent vfs inode lock. + * + * Most filesystems call d_delete() from ->unlink() ->rmdir() ->rename() + * ops under parent vfs inode lock. + * + * Some pseudo filesystems call d_delete() without parent inode lock. + * Those filesystems have no ->rename() op and they do not call + * d_move() directly, so d_parent and d_name are stable by definition. + * Examples: devpts, efivarfs, rpc_pipefs, functionfs. + * + * Last, there are the clustered filesystems that call d_delete() on + * remote nodes, not under vfs parent inode lock, but they use cluster + * distributed locks on local and remote nodes. Those filesystems call + * d_delete() under their cluster lock. Examples: + * - in ceph_fill_trace() under CEPH_MDS_R_PARENT_LOCKED + * - in ocfs2_dentry_convert_worker() under ocfs2_dentry_lock + * But those filesystems also call d_move() under the same cluster lock + * (i.e. FS_RENAME_DOES_D_MOVE), so d_parent and d_name are also stable. */ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir) { + struct inode *dir = d_inode(dentry->d_parent); __u32 mask = FS_DELETE; + /* d_delete() of pseudo inode? (e.g. __ns_get_path() playing tricks) */ + if (IS_ROOT(dentry)) + return; + + WARN_ON_ONCE(!inode_is_locked(dir) && dir->i_op->rename && + !(dir->i_sb->s_type->fs_flags & FS_RENAME_DOES_D_MOVE)); + if (isdir) mask |= FS_ISDIR; - fsnotify_dirent(NULL, dentry, mask); + fsnotify_dirent(dir, dentry, mask); } /* -- 2.17.1