Re: [PATCH 2/5] Btrfs: fix missing flush when committing a transaction

[Miao Xie <miaox@cn.fujitsu.com>]

On thu, 1 Nov 2012 16:04:27 +0800, Liu Bo wrote:
> (sorry, forgot to cc linux-btrfs.)
> On Thu, Nov 01, 2012 at 03:51:41PM +0800, Miao Xie wrote:
>> On Thu, 1 Nov 2012 15:44:43 +0800, Liu Bo wrote:
>>> On Thu, Nov 01, 2012 at 03:33:14PM +0800, Miao Xie wrote:
>>>> Consider the following case:
>>>> 	Task1				Task2
>>>> 	start_transaction
>>>> 					commit_transaction
>>>> 					  check pending snapshots list and the
>>>> 					  list is empty.
>>>> 	add pending snapshot into list
>>>> 					  skip the delalloc flush
>>>> 	end_transaction
>>>> 					  ...
>>>>
>>>> And then the problem that the snapshot is different with the source subvolume
>>>> happen.
>>>>
>>>
>>> This is weird, create_snapshot() will first add pending snapshot into
>>> list and then commit the transaction itself, regardless of if the
>>> snapshot is different with others or not.
>>
>> But the transaction may be committed by the other task, and the snapshot
>> creation task just wait until it ends.
>>
> 
> It's possible that a commit tranaction becomes a end transaction when it
> finds itself is already in commit.
> 
> So if snapshot creation starts the transaction, it will increment the
> transaction's num_writers, why does not the other task wait for its
> end_transacion?
> 
> I doubt if this can really happen anyway...
> 
> Can you elaborate the situation more?

Task1					Task2
start_transaction
					start_transaction
					commit_transaction
					  set in_commit to 1
					  check pending snapshots list and the list is empty.
add pending snapshot into list
					  skip the delalloc flush
commit_transaction
  find in_commit is 1
  end_transaction (num_writer--)
  wait_for_commit
					  num_writer is 1
				  	  continue committing the transaction
					  ...

Thanks
Miao

> 
> thanks,
> liubo
> 
>>>
>>> How do you find this?
>>
>> Just by review the code. I think it can be triggered 
>>
>> Thanks
>> Miao
>>
>>>
>>> thanks,
>>> liubo
>>>
>>>> This patch fixes the above problem by flush all pending stuffs when all the
>>>> other tasks end the transaction.
>>>>
>>>> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
>>>> ---
>>>>  fs/btrfs/transaction.c |   74 ++++++++++++++++++++++++++++++-----------------
>>>>  1 files changed, 47 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>>>> index 6d0d5a0..d9a9a70 100644
>>>> --- a/fs/btrfs/transaction.c
>>>> +++ b/fs/btrfs/transaction.c
>>>> @@ -1401,6 +1401,48 @@ static void cleanup_transaction(struct btrfs_trans_handle *trans,
>>>>  	kmem_cache_free(btrfs_trans_handle_cachep, trans);
>>>>  }
>>>>  
>>>> +static int btrfs_flush_all_pending_stuffs(struct btrfs_trans_handle *trans,
>>>> +					  struct btrfs_root *root)
>>>> +{
>>>> +	int flush_on_commit = btrfs_test_opt(root, FLUSHONCOMMIT);
>>>> +	int snap_pending = 0;
>>>> +	int ret;
>>>> +
>>>> +	if (!flush_on_commit) {
>>>> +		spin_lock(&root->fs_info->trans_lock);
>>>> +		if (!list_empty(&trans->transaction->pending_snapshots))
>>>> +			snap_pending = 1;
>>>> +		spin_unlock(&root->fs_info->trans_lock);
>>>> +	}
>>>> +
>>>> +	if (flush_on_commit || snap_pending) {
>>>> +		btrfs_start_delalloc_inodes(root, 1);
>>>> +		btrfs_wait_ordered_extents(root, 1);
>>>> +	}
>>>> +
>>>> +	ret = btrfs_run_delayed_items(trans, root);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	/*
>>>> +	 * running the delayed items may have added new refs. account
>>>> +	 * them now so that they hinder processing of more delayed refs
>>>> +	 * as little as possible.
>>>> +	 */
>>>> +	btrfs_delayed_refs_qgroup_accounting(trans, root->fs_info);
>>>> +
>>>> +	/*
>>>> +	 * rename don't use btrfs_join_transaction, so, once we
>>>> +	 * set the transaction to blocked above, we aren't going
>>>> +	 * to get any new ordered operations.  We can safely run
>>>> +	 * it here and no for sure that nothing new will be added
>>>> +	 * to the list
>>>> +	 */
>>>> +	btrfs_run_ordered_operations(root, 1);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  /*
>>>>   * btrfs_transaction state sequence:
>>>>   *    in_commit = 0, blocked = 0  (initial)
>>>> @@ -1418,7 +1460,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>>>>  	int ret = -EIO;
>>>>  	int should_grow = 0;
>>>>  	unsigned long now = get_seconds();
>>>> -	int flush_on_commit = btrfs_test_opt(root, FLUSHONCOMMIT);
>>>>  
>>>>  	btrfs_run_ordered_operations(root, 0);
>>>>  
>>>> @@ -1491,39 +1532,14 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>>>>  		should_grow = 1;
>>>>  
>>>>  	do {
>>>> -		int snap_pending = 0;
>>>> -
>>>>  		joined = cur_trans->num_joined;
>>>> -		if (!list_empty(&trans->transaction->pending_snapshots))
>>>> -			snap_pending = 1;
>>>>  
>>>>  		WARN_ON(cur_trans != trans->transaction);
>>>>  
>>>> -		if (flush_on_commit || snap_pending) {
>>>> -			btrfs_start_delalloc_inodes(root, 1);
>>>> -			btrfs_wait_ordered_extents(root, 1);
>>>> -		}
>>>> -
>>>> -		ret = btrfs_run_delayed_items(trans, root);
>>>> +		ret = btrfs_flush_all_pending_stuffs(trans, root);
>>>>  		if (ret)
>>>>  			goto cleanup_transaction;
>>>>  
>>>> -		/*
>>>> -		 * running the delayed items may have added new refs. account
>>>> -		 * them now so that they hinder processing of more delayed refs
>>>> -		 * as little as possible.
>>>> -		 */
>>>> -		btrfs_delayed_refs_qgroup_accounting(trans, root->fs_info);
>>>> -
>>>> -		/*
>>>> -		 * rename don't use btrfs_join_transaction, so, once we
>>>> -		 * set the transaction to blocked above, we aren't going
>>>> -		 * to get any new ordered operations.  We can safely run
>>>> -		 * it here and no for sure that nothing new will be added
>>>> -		 * to the list
>>>> -		 */
>>>> -		btrfs_run_ordered_operations(root, 1);
>>>> -
>>>>  		prepare_to_wait(&cur_trans->writer_wait, &wait,
>>>>  				TASK_UNINTERRUPTIBLE);
>>>>  
>>>> @@ -1536,6 +1552,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>>>>  	} while (atomic_read(&cur_trans->num_writers) > 1 ||
>>>>  		 (should_grow && cur_trans->num_joined != joined));
>>>>  
>>>> +	ret = btrfs_flush_all_pending_stuffs(trans, root);
>>>> +	if (ret)
>>>> +		goto cleanup_transaction;
>>>> +
>>>>  	/*
>>>>  	 * Ok now we need to make sure to block out any other joins while we
>>>>  	 * commit the transaction.  We could have started a join before setting
>>>> -- 
>>>> 1.7.6.5
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>