From mboxrd@z Thu Jan 1 00:00:00 1970 From: Amir Goldstein Subject: Re: [PATCH v4] ovl: lockdep annotate of nested stacked overlayfs inode lock Date: Fri, 3 Mar 2017 10:22:37 +0200 Message-ID: References: <1484129266-8968-1-git-send-email-amir73il@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Received: from mail-ot0-f196.google.com ([74.125.82.196]:34991 "EHLO mail-ot0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751843AbdCCKO4 (ORCPT ); Fri, 3 Mar 2017 05:14:56 -0500 Received: by mail-ot0-f196.google.com with SMTP id a12so8013454ota.2 for ; Fri, 03 Mar 2017 02:14:50 -0800 (PST) In-Reply-To: Sender: linux-unionfs-owner@vger.kernel.org List-Id: linux-unionfs@vger.kernel.org To: Miklos Szeredi Cc: linux-unionfs@vger.kernel.org, Xiong Zhou On Wed, Feb 8, 2017 at 9:42 AM, Amir Goldstein wrote: > On Tue, Jan 31, 2017 at 9:15 AM, Amir Goldstein wrote: >> On Wed, Jan 11, 2017 at 12:07 PM, Amir Goldstein wrote: >>> An overlayfs instance can be the lower layer of another overlayfs >>> instance. This setup triggers a lockdep splat of possible recursive >>> locking of sb->s_type->i_mutex_key in iterate_dir(). Trimmed snip: >>> >>> [ INFO: possible recursive locking detected ] >>> bash/2468 is trying to acquire lock: >>> &sb->s_type->i_mutex_key#14, at: iterate_dir+0x7d/0x15c >>> but task is already holding lock: >>> &sb->s_type->i_mutex_key#14, at: iterate_dir+0x7d/0x15c >>> >>> One problem observed with this splat is that ovl_new_inode() >>> does not call lockdep_annotate_inode_mutex_key() to annotate >>> the dir inode lock as &sb->s_type->i_mutex_dir_key like other >>> fs do. >>> >>> The other problem is that the 2 nested levels of overlayfs inode >>> lock are annotated using the same key, which is the cause of the >>> false positive lockdep warning. >>> >>> Fix this by annotating overlayfs inode lock in ovl_fill_inode() >>> according to stack level of the super block instance and use >>> different key for dir vs. non-dir like other fs do. >>> >>> Here is an edited snip from /proc/lockdep_chains after >>> iterate_dir() of nested overlayfs: >>> >>> [...] &ovl_i_mutex_dir_key[depth] (stack_depth=2) >>> [...] &ovl_i_mutex_dir_key[depth]#2 (stack_depth=1) >>> [...] &type->i_mutex_dir_key (stack_depth=0) >>> >>> Signed-off-by: Amir Goldstein >>> --- >>> fs/overlayfs/inode.c | 38 ++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 38 insertions(+) >>> >>> v4: >>> - further simplify by removing unneeded ofs->nested >>> >> >> Miklos, >> >> This patch version addresses your review comments from previous merge cycle >> and it has zero impact without CONFIG_LOCKDEP as you requested. >> > > Ping. > > Would you mind queuing this one as well for 4.11? > Ping^2 This fixes a false positive lockdep splat on new xfstest overlay/029. > Thanks, > Amir. > >> >>> v3: >>> - discard different annotation for nesting level 0 >>> - compile away without CONFIG_LOCKDEP >>> >>> v2: >>> - specific implementation in overlayfs >>> >>> v1: >>> - generic implemetnation in vfs >>> >>> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c >>> index 08643ac..b954622 100644 >>> --- a/fs/overlayfs/inode.c >>> +++ b/fs/overlayfs/inode.c >>> @@ -301,6 +301,42 @@ static const struct inode_operations ovl_symlink_inode_operations = { >>> .update_time = ovl_update_time, >>> }; >>> >>> +/* >>> + * It is possible to stack overlayfs instance on top of another >>> + * overlayfs instance as lower layer. We need to annonate the >>> + * stackable i_mutex locks according to stack level of the super >>> + * block instance. An overlayfs instance can never be in stack >>> + * depth 0 (there is always a real fs below it). An overlayfs >>> + * inode lock will use the lockdep annotaion ovl_i_mutex_key[depth]. >>> + * >>> + * For example, here is a snip from /proc/lockdep_chains after >>> + * dir_iterate of nested overlayfs: >>> + * >>> + * [...] &ovl_i_mutex_dir_key[depth] (stack_depth=2) >>> + * [...] &ovl_i_mutex_dir_key[depth]#2 (stack_depth=1) >>> + * [...] &type->i_mutex_dir_key (stack_depth=0) >>> + */ >>> + >>> +static struct lock_class_key ovl_i_mutex_key[FILESYSTEM_MAX_STACK_DEPTH]; >>> +static struct lock_class_key ovl_i_mutex_dir_key[FILESYSTEM_MAX_STACK_DEPTH]; >>> + >>> +static inline void ovl_lockdep_annotate_inode_mutex_key(struct inode *inode) >>> +{ >>> +#ifdef CONFIG_LOCKDEP >>> + int depth = inode->i_sb->s_stack_depth - 1; >>> + >>> + if (WARN_ON_ONCE(depth < 0)) >>> + depth = 0; >>> + >>> + if (S_ISDIR(inode->i_mode)) >>> + lockdep_set_class(&inode->i_rwsem, >>> + &ovl_i_mutex_dir_key[depth]); >>> + else >>> + lockdep_set_class(&inode->i_rwsem, >>> + &ovl_i_mutex_key[depth]); >>> +#endif >>> +} >>> + >>> static void ovl_fill_inode(struct inode *inode, umode_t mode, dev_t rdev) >>> { >>> inode->i_ino = get_next_ino(); >>> @@ -310,6 +346,8 @@ static void ovl_fill_inode(struct inode *inode, umode_t mode, dev_t rdev) >>> inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE; >>> #endif >>> >>> + ovl_lockdep_annotate_inode_mutex_key(inode); >>> + >>> switch (mode & S_IFMT) { >>> case S_IFREG: >>> inode->i_op = &ovl_file_inode_operations; >>> -- >>> 2.7.4 >>>