Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753184AbcDVVzO (ORCPT ); Fri, 22 Apr 2016 17:55:14 -0400 Received: from mail-vk0-f50.google.com ([209.85.213.50]:35105 "EHLO mail-vk0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752372AbcDVVzM (ORCPT ); Fri, 22 Apr 2016 17:55:12 -0400 MIME-Version: 1.0 X-Originating-IP: [2a01:cb1c:13e:de00:2ed0:5aff:fe00:9f21] In-Reply-To: <20160419213905.fgd5rdgqgq5gbelt@mguzik> References: <1461098921.2921.3.camel@margaine.com> <20160419213905.fgd5rdgqgq5gbelt@mguzik> From: Florian Margaine Date: Fri, 22 Apr 2016 23:54:51 +0200 Message-ID: Subject: Re: [PATCH] fs: reintroduce freezing nesting To: Mateusz Guzik Cc: Alexander Viro , Jeff Layton , "J. Bruce Fields" , linux-fsdevel@vger.kernel.org, "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3486 Lines: 112 On Tue, Apr 19, 2016 at 11:39 PM, Mateusz Guzik wrote: > On Tue, Apr 19, 2016 at 10:48:41PM +0200, Florian Margaine wrote: >> The behavior was removed in 18e9e5104fcd9a973ffe3eed3816c87f2a1b6cd2 >> noting that this was a better idea than using a counter. However, this >> behavior is actually wanted if multiple applications want to freeze >> concurrently while remaining non-racy. >> >> This patch reintroduces this feature by using a counter. >> > > This patch is wrong. > > It uses non-atomic ops to modify the counter and no locks are > held to protect it. > > I would argue the code should track that freezing has started and > additional freezers must only return when the state is > SB_FREEZE_COMPLETE. Does it all additional freezers must return in order, or all additional freezers can return asap when SB_FREEZE_COMPLETE is set? I'm not sure the former is easily doable. > >> --- >> fs/super.c | 15 +++++++++++---- >> include/linux/fs.h | 1 + >> 2 files changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/fs/super.c b/fs/super.c >> index 74914b1..9fa8ca1 100644 >> --- a/fs/super.c >> +++ b/fs/super.c >> @@ -231,6 +231,7 @@ static struct super_block *alloc_super(struct >> file_system_type *type, int flags) >> */ >> down_write_nested(&s->s_umount, SINGLE_DEPTH_NESTING); >> s->s_count = 1; >> + s->s_freezers = 0; >> atomic_set(&s->s_active, 1); >> mutex_init(&s->s_vfs_rename_mutex); >> lockdep_set_class(&s->s_vfs_rename_mutex, &type- >> >s_vfs_rename_key); >> @@ -1275,12 +1276,12 @@ int freeze_super(struct super_block *sb) >> { >> int ret; >> >> + sb->s_freezers++; >> + if (sb->s_writers.frozen != SB_UNFROZEN) >> + return 0; >> + >> atomic_inc(&sb->s_active); >> down_write(&sb->s_umount); >> - if (sb->s_writers.frozen != SB_UNFROZEN) { >> - deactivate_locked_super(sb); >> - return -EBUSY; >> - } >> >> if (!(sb->s_flags & MS_BORN)) { >> up_write(&sb->s_umount); >> @@ -1338,14 +1339,20 @@ EXPORT_SYMBOL(freeze_super); >> * @sb: the super to thaw >> * >> * Unlocks the filesystem and marks it writeable again after >> freeze_super(). >> + * Since nesting freezes is allowed, only the last freeze actually >> unlocks. >> */ >> int thaw_super(struct super_block *sb) >> { >> int error; >> >> + sb->s_freezers--; >> + if (sb->s_freezers > 0) >> + return 0; >> + >> down_write(&sb->s_umount); >> if (sb->s_writers.frozen == SB_UNFROZEN) { >> up_write(&sb->s_umount); >> + sb->s_freezers++; >> return -EINVAL; >> } >> >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index e514f76..c045e2a 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -1333,6 +1333,7 @@ struct super_block { >> struct quota_info s_dquot; /* Diskquota specific >> options */ >> >> struct sb_writers s_writers; >> + int s_freezers; >> >> char s_id[32]; /* Informational >> name */ >> u8 s_uuid[16]; /* UUID */ >> -- >> 2.8.0 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Mateusz Guzik -- Florian Margaine