Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755267AbcDSVjR (ORCPT ); Tue, 19 Apr 2016 17:39:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60154 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754583AbcDSVjP (ORCPT ); Tue, 19 Apr 2016 17:39:15 -0400 Date: Tue, 19 Apr 2016 23:39:06 +0200 From: Mateusz Guzik To: Florian Margaine Cc: Alexander Viro , Jeff Layton , "J. Bruce Fields" , linux-fsdevel@vger.kernel.org, "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] fs: reintroduce freezing nesting Message-ID: <20160419213905.fgd5rdgqgq5gbelt@mguzik> References: <1461098921.2921.3.camel@margaine.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1461098921.2921.3.camel@margaine.com> User-Agent: Mutt/1.6.0.1 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2900 Lines: 101 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. > --- >  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