From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fernando Luis Vazquez Cao Subject: Re: [PATCH 2/8] fsfreeze: emergency thaw will deadlock on s_umount Date: Thu, 09 Aug 2012 15:00:11 +0900 Message-ID: <502351EB.2080909@lab.ntt.co.jp> References: <1342083464.7033.6.camel@nexus.lab.ntt.co.jp> <1342083849.7033.10.camel@nexus.lab.ntt.co.jp> <20120713131755.GE20361@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Al Viro , Dave Chinner , Christoph Hellwig , linux-fsdevel@vger.kernel.org To: Jan Kara Return-path: Received: from tama500.ecl.ntt.co.jp ([129.60.39.148]:35179 "EHLO tama500.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754110Ab2HIGAd (ORCPT ); Thu, 9 Aug 2012 02:00:33 -0400 In-Reply-To: <20120713131755.GE20361@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 2012/07/13 22:17, Jan Kara wrote: > On Thu 12-07-12 18:04:09, Fernando Luis V=E1zquez Cao wrote: >> diff -urNp vfs-orig/fs/super.c vfs/fs/super.c >> --- vfs-orig/fs/super.c 2012-07-04 18:57:54.000000000 +0900 >> +++ vfs/fs/super.c 2012-07-12 14:21:09.736566768 +0900 >> @@ -1221,23 +1221,25 @@ int freeze_super(struct super_block *sb) >> EXPORT_SYMBOL(freeze_super); >> =20 >> /** >> - * thaw_super -- unlock filesystem >> + * __thaw_super -- unlock filesystem >> * @sb: the super to thaw >> + * @emergency: emergency thaw >> * >> * Unlocks the filesystem and marks it writeable again after freez= e_super(). >> + * This is the unlocked version of thaw_super and has to be called = with the >> + * sb->s_umount lock held in the non-emergency thaw case. >> */ >> -int thaw_super(struct super_block *sb) >> +static int __thaw_super(struct super_block *sb, int emergency) >> { >> - int error; >> + int error =3D 0; >> =20 >> - down_write(&sb->s_umount); >> if (sb->s_frozen =3D=3D SB_UNFROZEN) { >> - up_write(&sb->s_umount); >> - return -EINVAL; >> + error =3D -EINVAL; >> + goto out; >> } >> =20 >> if (sb->s_flags & MS_RDONLY) >> - goto out; >> + goto out_thaw; >> =20 >> if (sb->s_op->unfreeze_fs) { >> error =3D sb->s_op->unfreeze_fs(sb); >> @@ -1245,17 +1247,52 @@ int thaw_super(struct super_block *sb) >> printk(KERN_ERR >> "VFS:Filesystem thaw failed\n"); >> sb->s_frozen =3D SB_FREEZE_TRANS; >> - up_write(&sb->s_umount); >> - return error; >> + goto out; >> } >> } >> =20 >> -out: >> +out_thaw: >> sb->s_frozen =3D SB_UNFROZEN; >> smp_wmb(); >> wake_up(&sb->s_wait_unfrozen); >> - deactivate_locked_super(sb); >> =20 >> - return 0; >> + /* >> + * When called from emergency scope, we cannot grab the s_umount l= ock >> + * so we cannot deactivate the superblock. This may leave unbalanc= ed >> + * superblock references which could prevent unmount, but given th= is is >> + * an emergency operation.... >> + */ >> + if (!emergency) >> + deactivate_locked_super(sb); >> +out: >> + return error; >> +} > This is just wrong. deactivate_locked_super() will unlock the supe= rblock > for you (and may even free it). So just do this in thaw_super() inste= ad of > up_write(&sb->s_umount) which is bogus there. That will also allow yo= u to > get rid of that ugly 'emergency' argument... Ouch! I am sorry for sending a botched patch. In my local tree the emergency argument is gone and thaw_super looks like this (up_write(&sb->s_umount) is needed when __thaw_super() fails because we do not want to release a reference to the superblock in that case): int thaw_super(struct super_block *sb) { int res; down_write(&sb->s_umount); res =3D __thaw_super(sb); if (!res) deactivate_locked_super(sb); else up_write(&sb->s_umount); return res; } >> +/** >> + * thaw_super_emergency -- unlock filesystem >> + * @sb: the super to thaw >> + * >> + * Unlocks the filesystem and marks it writeable again after freeze= _super(). >> + * The kernel gets here holding the sb->s_umount lock in read mode = so we cannot >> + * grab it again in write mode. >> + */ >> +int thaw_super_emergency(struct super_block *sb) >> +{ >> + return __thaw_super(sb, 1); >> +} > And this function can be deleted as well. Just call __thaw_super()= from > that one place. Good point. The code is easier to read that way. Thanks! - Fernando -- 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