| Message ID | 1386005619-7750-1-git-send-email-wangshilong1991@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Dec 03, 2013 at 01:33:39AM +0800, 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); IIRC, we already have btrfs_scrub_{pause, continue}() to avoid race situations between committing transaction and scrub processes, why not use that instead? (Actually I don't like adding another lock unless it's been proved necessary and correct with lockdep.) 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@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
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@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); > 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. :-) > > (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@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
On tue, 03 Dec 2013 13:06:34 +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@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); >> 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. :-) Moving btrfs_finish_extent_commit() into the log mutex may make the log tasks be blocked for a lot time. I think the better way to fix is prevent the scrubber from starting while the transaction is being committed.(wait scrub_pause_req == 0 before scrubbing the super block) Thanks Miao >> >> (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@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 > -- 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
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@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); > >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. 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@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
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@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); >>> 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@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 > -- 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
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, 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
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 > -- 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
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.