On tue, 3 Dec 2013 14:08:24 +0800, Liu Bo wrote:
> On Tue, Dec 03, 2013 at 01:06:34PM +0800, Wang Shilong wrote:
>> Hi Liu,
>>
>> On 12/03/2013 12:57 PM, Liu Bo wrote:
>>> On Tue, Dec 03, 2013 at 01:33:39AM +0800, Wang Shilong wrote:
>>>> From: Wang Shilong <wangsl.fnst@xxxxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxxxxxxxxxxxx>
>>>> Signed-off-by: Wang Shilong <wangsl.fnst@xxxxxxxxxxxxxx>
>>>> ---
>>>> 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);
>>> IIRC, we already have btrfs_scrub_{pause, continue}() to avoid race
>>> situations between committing transaction and scrub processes, why not use that
>>> instead?
>> btrfs_scrub_{pause,continue} can not stop the following case from happening:
>>
>> thread 1 thread 2
>> |->write_supers
>> |->start scrub
>> |->using last_trans_commited(not updated yet) when scrubbing supers
>> generation in disk is up to date but in memory is not.
>> |->updating last_trans_commited
>>
>> Pleae correct me if i am wrong here. :-)
>
> One possible way is to check @scrub_pause_req inside scrub_supers(),
> before starting the real scrubing super work.
>
> scrub_super()
> {
> while (scrub_pause_req)
> wait for (scrub_pause_req == 0);
>
> ...
> }
>
> As we have a atomic_inc(&fs_info->scrubs_running) before scrub_supers(),
> it'd force committing transaction to wait for scrub if the scrub process
> is the former one in timeline.
Great minds think alike!
Thanks
Miao
>
> thanks,
> -liubo
>
>>>
>>> (Actually I don't like adding another lock unless it's been proved necessary
>>> and correct with lockdep.)
>> Right, i should test if it can pass lockdep.
>>
>> Thanks for comments.
>> Wang
>>>
>>> thanks,
>>> -liubo
>>>
>>>> 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.
>>>> --
>>>> 1.8.4
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
