[PATCH] Btrfs: fix race in sync and freeze again

[PATCH] Btrfs: fix race in sync and freeze again

[Josef Bacik]

I screwed this up, there is a race between checking if there is a running
transaction and actually starting a transaction in sync where we could race
with a freezer and get ourselves into trouble.  To fix this we need to make
a new join type to only do the try lock on the freeze stuff.  If it fails
we'll return EPERM and just return from sync.  This fixes a hang Liu Bo
reported when running xfstest 68 in a loop.  Thanks,

Reported-by: Liu Bo <bo.li.liu@oracle.com>
Signed-off-by: Josef Bacik <jbacik@fusionio.com>
---
 fs/btrfs/super.c       |   15 ++++++---------
 fs/btrfs/transaction.c |   12 +++++++++++-
 fs/btrfs/transaction.h |    1 +
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 867e8e7..903ab2d 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -854,16 +854,13 @@ int btrfs_sync_fs(struct super_block *sb, int wait)
 
 	btrfs_wait_ordered_extents(root, 0, 0);
 
-	spin_lock(&fs_info->trans_lock);
-	if (!fs_info->running_transaction) {
-		spin_unlock(&fs_info->trans_lock);
-		return 0;
-	}
-	spin_unlock(&fs_info->trans_lock);
-
-	trans = btrfs_join_transaction(root);
-	if (IS_ERR(trans))
+	trans = btrfs_join_transaction_freeze(root);
+	if (IS_ERR(trans)) {
+		/* Frozen, don't bother */
+		if (PTR_ERR(trans) == -EPERM)
+			return 0;
 		return PTR_ERR(trans);
+	}
 	return btrfs_commit_transaction(trans, root);
 }
 
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index c01dec7..e4bfac8 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -272,6 +272,7 @@ enum btrfs_trans_type {
 	TRANS_JOIN,
 	TRANS_USERSPACE,
 	TRANS_JOIN_NOLOCK,
+	TRANS_JOIN_FREEZE,
 };
 
 static int may_wait_transaction(struct btrfs_root *root, int type)
@@ -341,7 +342,11 @@ again:
 	if (!h)
 		return ERR_PTR(-ENOMEM);
 
-	sb_start_intwrite(root->fs_info->sb);
+	if (!__sb_start_write(root->fs_info->sb, SB_FREEZE_FS, false)) {
+		if (type == TRANS_JOIN_FREEZE)
+			return ERR_PTR(-EPERM);
+		sb_start_intwrite(root->fs_info->sb);
+	}
 
 	if (may_wait_transaction(root, type))
 		wait_current_trans(root);
@@ -424,6 +429,11 @@ struct btrfs_trans_handle *btrfs_start_ioctl_transaction(struct btrfs_root *root
 	return start_transaction(root, 0, TRANS_USERSPACE, 0);
 }
 
+struct btrfs_trans_handle *btrfs_join_transaction_freeze(struct btrfs_root *root)
+{
+	return start_transaction(root, 0, TRANS_JOIN_FREEZE, 0);
+}
+
 /* wait for a transaction commit to be fully complete */
 static noinline void wait_for_commit(struct btrfs_root *root,
 				    struct btrfs_transaction *commit)
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index b6463e1..fbf8313 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -102,6 +102,7 @@ struct btrfs_trans_handle *btrfs_start_transaction_noflush(
 					struct btrfs_root *root, int num_items);
 struct btrfs_trans_handle *btrfs_join_transaction(struct btrfs_root *root);
 struct btrfs_trans_handle *btrfs_join_transaction_nolock(struct btrfs_root *root);
+struct btrfs_trans_handle *btrfs_join_transaction_freeze(struct btrfs_root *root);
 struct btrfs_trans_handle *btrfs_start_ioctl_transaction(struct btrfs_root *root);
 int btrfs_wait_for_commit(struct btrfs_root *root, u64 transid);
 int btrfs_write_and_wait_transaction(struct btrfs_trans_handle *trans,
-- 
1.7.7.6

Re: [PATCH] Btrfs: fix race in sync and freeze again

[Liu Bo]

On 09/14/2012 10:37 PM, Josef Bacik wrote:
> I screwed this up, there is a race between checking if there is a running
> transaction and actually starting a transaction in sync where we could race
> with a freezer and get ourselves into trouble.  To fix this we need to make
> a new join type to only do the try lock on the freeze stuff.  If it fails
> we'll return EPERM and just return from sync.  This fixes a hang Liu Bo
> reported when running xfstest 68 in a loop.  Thanks,
> 

This is also a trylock, why don't we just add a trylock for sb_start_intwrite directly
just as what I've done before, that'd be cleaner:

https://patchwork.kernel.org/patch/1318131/

> Reported-by: Liu Bo <bo.li.liu@oracle.com>
> Signed-off-by: Josef Bacik <jbacik@fusionio.com>
> ---
>  fs/btrfs/super.c       |   15 ++++++---------
>  fs/btrfs/transaction.c |   12 +++++++++++-
>  fs/btrfs/transaction.h |    1 +
>  3 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 867e8e7..903ab2d 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -854,16 +854,13 @@ int btrfs_sync_fs(struct super_block *sb, int wait)
>  
>  	btrfs_wait_ordered_extents(root, 0, 0);
>  
> -	spin_lock(&fs_info->trans_lock);
> -	if (!fs_info->running_transaction) {
> -		spin_unlock(&fs_info->trans_lock);
> -		return 0;
> -	}
> -	spin_unlock(&fs_info->trans_lock);
> -
> -	trans = btrfs_join_transaction(root);
> -	if (IS_ERR(trans))
> +	trans = btrfs_join_transaction_freeze(root);
> +	if (IS_ERR(trans)) {
> +		/* Frozen, don't bother */
> +		if (PTR_ERR(trans) == -EPERM)
> +			return 0;
>  		return PTR_ERR(trans);
> +	}
>  	return btrfs_commit_transaction(trans, root);
>  }
>  
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index c01dec7..e4bfac8 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -272,6 +272,7 @@ enum btrfs_trans_type {
>  	TRANS_JOIN,
>  	TRANS_USERSPACE,
>  	TRANS_JOIN_NOLOCK,
> +	TRANS_JOIN_FREEZE,
>  };
>  
>  static int may_wait_transaction(struct btrfs_root *root, int type)
> @@ -341,7 +342,11 @@ again:
>  	if (!h)
>  		return ERR_PTR(-ENOMEM);
>  
> -	sb_start_intwrite(root->fs_info->sb);
> +	if (!__sb_start_write(root->fs_info->sb, SB_FREEZE_FS, false)) {
> +		if (type == TRANS_JOIN_FREEZE)
> +			return ERR_PTR(-EPERM);

and here we need to free trans handle 'h', don't we?

> +		sb_start_intwrite(root->fs_info->sb);
> +	}
>  
>  	if (may_wait_transaction(root, type))
>  		wait_current_trans(root);
> @@ -424,6 +429,11 @@ struct btrfs_trans_handle *btrfs_start_ioctl_transaction(struct btrfs_root *root
>  	return start_transaction(root, 0, TRANS_USERSPACE, 0);
>  }
>  
> +struct btrfs_trans_handle *btrfs_join_transaction_freeze(struct btrfs_root *root)
> +{
> +	return start_transaction(root, 0, TRANS_JOIN_FREEZE, 0);
> +}
> +

Seems that this is not based on btrfs's upstream git repo, I don't have four args in start_transaction...

thanks,
liubo

>  /* wait for a transaction commit to be fully complete */
>  static noinline void wait_for_commit(struct btrfs_root *root,
>  				    struct btrfs_transaction *commit)
> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
> index b6463e1..fbf8313 100644
> --- a/fs/btrfs/transaction.h
> +++ b/fs/btrfs/transaction.h
> @@ -102,6 +102,7 @@ struct btrfs_trans_handle *btrfs_start_transaction_noflush(
>  					struct btrfs_root *root, int num_items);
>  struct btrfs_trans_handle *btrfs_join_transaction(struct btrfs_root *root);
>  struct btrfs_trans_handle *btrfs_join_transaction_nolock(struct btrfs_root *root);
> +struct btrfs_trans_handle *btrfs_join_transaction_freeze(struct btrfs_root *root);
>  struct btrfs_trans_handle *btrfs_start_ioctl_transaction(struct btrfs_root *root);
>  int btrfs_wait_for_commit(struct btrfs_root *root, u64 transid);
>  int btrfs_write_and_wait_transaction(struct btrfs_trans_handle *trans,
> 

Re: [PATCH] Btrfs: fix race in sync and freeze again

[Josef Bacik]

On Fri, Sep 14, 2012 at 08:54:08AM -0600, Liu Bo wrote:
> On 09/14/2012 10:37 PM, Josef Bacik wrote:
> > I screwed this up, there is a race between checking if there is a running
> > transaction and actually starting a transaction in sync where we could race
> > with a freezer and get ourselves into trouble.  To fix this we need to make
> > a new join type to only do the try lock on the freeze stuff.  If it fails
> > we'll return EPERM and just return from sync.  This fixes a hang Liu Bo
> > reported when running xfstest 68 in a loop.  Thanks,
> > 
> 
> This is also a trylock, why don't we just add a trylock for sb_start_intwrite directly
> just as what I've done before, that'd be cleaner:
> 
> https://patchwork.kernel.org/patch/1318131/
> 

Yeah if we get a sb_start_intwrite_trylock() then we can switch to using that in
the future, but for now I'm not going to block behind something needing to be
accepted into someone elses tree in order to get this fixed in btrfs.  Also the
patch you've posted isn't right because we hold the rwsem and an ref, so we have
to drop the rwsem before calling btrfs_join_transaction and then we need to make
sure to do sb_end_intwrite() after we do the commit to clean up our ref, my
approach is cleaner since it still lets the transaction deal with all of that
cleanup and we don't have to play with the freeze rwsem.  Thanks,

Josef