From patchwork Mon Sep 26 16:55:25 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Oleg Nesterov X-Patchwork-Id: 9351025 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 23FD56077B for ; Mon, 26 Sep 2016 16:55:47 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1003228B80 for ; Mon, 26 Sep 2016 16:55:47 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F3FE528D2D; Mon, 26 Sep 2016 16:55:46 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 85D2E28B80 for ; Mon, 26 Sep 2016 16:55:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423371AbcIZQz3 (ORCPT ); Mon, 26 Sep 2016 12:55:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35432 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1422838AbcIZQz2 (ORCPT ); Mon, 26 Sep 2016 12:55:28 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6D9F67F74F; Mon, 26 Sep 2016 16:55:27 +0000 (UTC) Received: from dhcp-27-174.brq.redhat.com ([10.34.27.30]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id u8QGtPdw032565; Mon, 26 Sep 2016 12:55:25 -0400 Received: by dhcp-27-174.brq.redhat.com (nbSMTP-1.00) for uid 1000 oleg@redhat.com; Mon, 26 Sep 2016 18:55:27 +0200 (CEST) Date: Mon, 26 Sep 2016 18:55:25 +0200 From: Oleg Nesterov To: Jan Kara Cc: Al Viro , Dave Chinner , Nikolay Borisov , "Paul E. McKenney" , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and thaw_super() paths Message-ID: <20160926165525.GA9338@redhat.com> References: <20160926160724.GA6739@redhat.com> <20160926160806.GB6748@redhat.com> <20160926161856.GB32458@quack2.suse.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20160926161856.GB32458@quack2.suse.cz> User-Agent: Mutt/1.5.24 (2015-08-30) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Mon, 26 Sep 2016 16:55:27 +0000 (UTC) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 09/26, Jan Kara wrote: > > On Mon 26-09-16 18:08:06, Oleg Nesterov wrote: > > +/* > > + * Tell lockdep we are holding these locks before we call ->unfreeze_fs(sb). > > + */ > > +static void sb_freeze_acquire(struct super_block *sb) > > Can we call this lockdep_sb_freeze_acquire() or something like that so that > it is clear this is only about lockdep annotations? Similarly with > sb_freeze_unlock()... OK, thanks, done. See V2 below. > and I hope you really tested > there are no more lockdep false positives ;). Heh ;) if only I knew how to test this... I ran the following script under qemu mkfs.xfs -f /dev/vda mkfs.xfs -f /dev/vdb mkdir -p TEST SCRATCH TEST_DEV=/dev/vda TEST_DIR=TEST SCRATCH_DEV=/dev/vdb SCRATCH_MNT=SCRATCH \ ./check `grep -il freeze tests/*/???` this time. And yes, I'm afraid this change can uncover some false positives later. But at the same time potentially it can find the real problems. It would be nice to remove another hack in __sb_start_write under ifdef(CONFIG_LOCKDEP), but iirc XFS actually takes the same rw_sem twice for reading, so we can't do this. ------------------------------------------------------------------------------- From 70e774533ab6319f9b90a490b036150ad9604af7 Mon Sep 17 00:00:00 2001 From: Oleg Nesterov Date: Mon, 26 Sep 2016 17:23:24 +0200 Subject: [PATCH V2 2/2] fs/super.c: don't fool lockdep in freeze_super() and thaw_super() paths sb_wait_write()->percpu_rwsem_release() fools lockdep to avoid the false-positives. Now that xfs was fixed by Dave's commit dbad7c993053 ("xfs: stop holding ILOCK over filldir callbacks") we can remove it and change freeze_super() and thaw_super() to run with s_writers.rw_sem locks held; we add two trivial helpers for that, lockdep_sb_freeze_release() and lockdep_sb_freeze_acquire(). xfstests-dev/check `grep -il freeze tests/*/???` does not trigger any warning from lockdep. Signed-off-by: Oleg Nesterov Reviewed-by: Jan Kara --- fs/super.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/fs/super.c b/fs/super.c index 2549896c..0447afe 100644 --- a/fs/super.c +++ b/fs/super.c @@ -1214,25 +1214,34 @@ EXPORT_SYMBOL(__sb_start_write); static void sb_wait_write(struct super_block *sb, int level) { percpu_down_write(sb->s_writers.rw_sem + level-1); - /* - * We are going to return to userspace and forget about this lock, the - * ownership goes to the caller of thaw_super() which does unlock. - * - * FIXME: we should do this before return from freeze_super() after we - * called sync_filesystem(sb) and s_op->freeze_fs(sb), and thaw_super() - * should re-acquire these locks before s_op->unfreeze_fs(sb). However - * this leads to lockdep false-positives, so currently we do the early - * release right after acquire. - */ - percpu_rwsem_release(sb->s_writers.rw_sem + level-1, 0, _THIS_IP_); } -static void sb_freeze_unlock(struct super_block *sb) +/* + * We are going to return to userspace and forget about these locks, the + * ownership goes to the caller of thaw_super() which does unlock(). + */ +static void lockdep_sb_freeze_release(struct super_block *sb) +{ + int level; + + for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--) + percpu_rwsem_release(sb->s_writers.rw_sem + level, 0, _THIS_IP_); +} + +/* + * Tell lockdep we are holding these locks before we call ->unfreeze_fs(sb). + */ +static void lockdep_sb_freeze_acquire(struct super_block *sb) { int level; for (level = 0; level < SB_FREEZE_LEVELS; ++level) percpu_rwsem_acquire(sb->s_writers.rw_sem + level, 0, _THIS_IP_); +} + +static void sb_freeze_unlock(struct super_block *sb) +{ + int level; for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--) percpu_up_write(sb->s_writers.rw_sem + level); @@ -1328,6 +1337,7 @@ int freeze_super(struct super_block *sb) * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super(). */ sb->s_writers.frozen = SB_FREEZE_COMPLETE; + lockdep_sb_freeze_release(sb); up_write(&sb->s_umount); return 0; } @@ -1354,11 +1364,14 @@ int thaw_super(struct super_block *sb) goto out; } + lockdep_sb_freeze_acquire(sb); + if (sb->s_op->unfreeze_fs) { error = sb->s_op->unfreeze_fs(sb); if (error) { printk(KERN_ERR "VFS:Filesystem thaw failed\n"); + lockdep_sb_freeze_release(sb); up_write(&sb->s_umount); return error; }