Re: [PATCH 2/8] fsfreeze: emergency thaw will deadlock on s_umount

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2012/07/13 22:17, Jan Kara wrote:
On Thu 12-07-12 18:04:09, Fernando Luis Vázquez 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);
/**
- * 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 freeze_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 = 0;
- down_write(&sb->s_umount);
  	if (sb->s_frozen == SB_UNFROZEN) {
-		up_write(&sb->s_umount);
-		return -EINVAL;
+		error = -EINVAL;
+		goto out;
  	}
if (sb->s_flags & MS_RDONLY)
-		goto out;
+		goto out_thaw;
if (sb->s_op->unfreeze_fs) {
  		error = 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 = SB_FREEZE_TRANS;
-			up_write(&sb->s_umount);
-			return error;
+			goto out;
  		}
  	}
-out:
+out_thaw:
  	sb->s_frozen = SB_UNFROZEN;
  	smp_wmb();
  	wake_up(&sb->s_wait_unfrozen);
-	deactivate_locked_super(sb);
- return 0;
+	/*
+	 * When called from emergency scope, we cannot grab the s_umount lock
+	 * so we cannot deactivate the superblock. This may leave unbalanced
+	 * superblock references which could prevent unmount, but given this is
+	 * an emergency operation....
+	 */
+	if (!emergency)
+		deactivate_locked_super(sb);
+out:
+	return error;
+}
   This is just wrong. deactivate_locked_super() will unlock the superblock
for you (and may even free it). So just do this in thaw_super() instead of
up_write(&sb->s_umount) which is bogus there. That will also allow you 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 = __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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux