Re: [PATCH v3] Btrfs: fix wrong super generation mismatch when scrubbing supers

[Wang Shilong <wangsl.fnst@cn.fujitsu.com>]

On 12/04/2013 03:14 AM, Sebastian Ochmann wrote:
> Hello,
>
> I know, the discussion on how to fix the problem best is still 
> on-going, but I wanted to add that I tried v3 of the patch against 
> btrfs-next on my machine. Without the patch, I was able to reproduce 
> the problem within a few minutes; after applying it, I wasn't able to 
> trigger it for 50 minutes now.
>
> I can't tell whether the problem would reoccur when running my little 
> test for another week or so, but I can tell that my machine did not 
> catch fire either. So it seems like you're on the right track. :)
>
> I can also try another version of the patch when it becomes available.

Thanks very much, new patch will be sent later.:-)

Thanks,
Wang
>
> Thanks,
> Sebastian
>
> On 02.12.2013 18:33, Wang Shilong wrote:
>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>
>> We came a race condition when scrubbing superblocks, the story is:
>>
>> In commiting transaction, we will update last_trans_commited after
>> writting superblocks. if a scrub start after writting superblocks
>> and before last_trans_commited, generation mismatch happens!
>>
>> We fix it by protecting writting superblock and updating 
>> last_trans_commited
>> with tree_log_mutex.
>>
>> Reported-by: Sebastian Ochmann <ochmann@informatik.uni-bonn.de>
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> ---
>> Changelog:
>>     v2->v3:move tree_log_mutex out of device_list_mutex.
>>     v1->v2: use right way to fix the problem.
>> ---
>>   fs/btrfs/scrub.c       | 11 +++++++----
>>   fs/btrfs/transaction.c | 13 ++++++++++---
>>   2 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>> index 561e2f1..a9ed102 100644
>> --- a/fs/btrfs/scrub.c
>> +++ b/fs/btrfs/scrub.c
>> @@ -2887,6 +2887,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info 
>> *fs_info, u64 devid, u64 start,
>>       }
>>
>>
>> +    mutex_lock(&fs_info->tree_log_mutex);
>> mutex_lock(&fs_info->fs_devices->device_list_mutex);
>>       dev = btrfs_find_device(fs_info, devid, NULL, NULL);
>>       if (!dev || (dev->missing && !is_dev_replace)) {
>> @@ -2932,14 +2933,16 @@ int btrfs_scrub_dev(struct btrfs_fs_info 
>> *fs_info, u64 devid, u64 start,
>>       atomic_inc(&fs_info->scrubs_running);
>>       mutex_unlock(&fs_info->scrub_lock);
>>
>> +    /*
>> +     * holding tree_log_mutex we can avoid generation mismatch while
>> +     * scrubbing superblocks, see comments in commiting transaction
>> +     * when updating last_trans_commited.
>> +     */
>>       if (!is_dev_replace) {
>> -        /*
>> -         * by holding device list mutex, we can
>> -         * kick off writing super in log tree sync.
>> -         */
>>           ret = scrub_supers(sctx, dev);
>>       }
>> mutex_unlock(&fs_info->fs_devices->device_list_mutex);
>> +    mutex_unlock(&fs_info->tree_log_mutex);
>>
>>       if (!ret)
>>           ret = scrub_enumerate_chunks(sctx, dev, start, end,
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index c6a872a..052eb22 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -1898,15 +1898,22 @@ int btrfs_commit_transaction(struct 
>> btrfs_trans_handle *trans,
>>           goto cleanup_transaction;
>>       }
>>
>> +    btrfs_finish_extent_commit(trans, root);
>> +
>> +    /*
>> +     * we must gurantee last_trans_commited update is protected by
>> +     * tree_log_mutex with write_ctree_super together, otherwise,
>> +     * scubbing super will come in before updating last_trans_commited
>> +     * and we will get generation mismatch when scrubbing superblocks.
>> +     */
>> +    root->fs_info->last_trans_committed = cur_trans->transid;
>> +
>>       /*
>>        * the super is written, we can safely allow the tree-loggers
>>        * to go about their business
>>        */
>>       mutex_unlock(&root->fs_info->tree_log_mutex);
>>
>> -    btrfs_finish_extent_commit(trans, root);
>> -
>> -    root->fs_info->last_trans_committed = cur_trans->transid;
>>       /*
>>        * We needn't acquire the lock here because there is no other task
>>        * which can change it.
>>
>
> -- 
> 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
>