Btrfs: use bit operation for ->fs_state
PATCH 10/11 Btrfs: use bit operation for ->fs_state
There is no lock to protect fs_info->fs_state, it will introduce some problems, such as the value may be covered by the other task when several tasks modify it. Now we use bit operation for it to fix the above problem.
Signed-off-by Miao Xie <[email protected]>
--- fs/btrfs/ctree.h | 4 +++- fs/btrfs/disk-io.c | 5 +++-- fs/btrfs/file.c | 2 +- fs/btrfs/scrub.c | 2 +- fs/btrfs/super.c | 4 ++-- fs/btrfs/transaction.c | 9 ++++----- 6 files changed, 14 insertions(+), 12 deletions(-)
fs/btrfs/ctree.h +3 -1
@@ -338,7 +338,9 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)/* * File system states */ +#define BTRFS_FS_STATE_ERROR 0 +/* Super block flags */ /* Errors detected */ #define BTRFS_SUPER_FLAG_ERROR (1ULL << 2)
@@ -1540,7 +1542,7 @@ struct btrfs_fs_info {u64 qgroup_seq;/* filesystem state */ -u64 fs_state; +unsigned long fs_state;struct btrfs_delayed_root *delayed_root;
fs/btrfs/disk-io.c +3 -2
@@ -2196,7 +2196,8 @@ int open_ctree(struct super_block *sb, goto fail_alloc;/* check FS state, whether FS is broken. */ - fs_info->fs_state |=btrfs_super_flags (disk_super); +if (btrfs_super_flags (disk_super) &BTRFS_SUPER_FLAG_ERROR ) +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); ret =btrfs_check_super_valid (fs_info, sb->s_flags &MS_RDONLY );if (ret) {
@@ -3354,7 +3355,7 @@ int close_ctree(struct btrfs_root *root)printk (KERN_ERR "btrfs: commit super ret %d\n" , ret); } -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state))btrfs_error_commit_super (root);btrfs_put_block_group_cache (fs_info);
fs/btrfs/file.c +1 -1
@@ -1531,7 +1531,7 @@ static ssize_t btrfs_file_aio_write(struct kiocb *iocb,* although we have opened a file as writable, we have * to stop this write operation to ensure FS consistency. */ -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) {mutex_unlock (&inode->i_mutex); err = -EROFS ; goto out;
fs/btrfs/scrub.c +1 -1
@@ -2700,7 +2700,7 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx,int ret;struct btrfs_root *root = sctx->dev_root; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return -EIO ; gen =atomic64_read (&root->fs_info->last_trans_committed);
fs/btrfs/super.c +2 -2
@@ -98,7 +98,7 @@ static void __save_error_info(struct btrfs_fs_info *fs_info)* today we only save the error info into ram. Long term we'll * also send it down to the disk */ - fs_info->fs_state =BTRFS_SUPER_FLAG_ERROR ; +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); }static void save_error_info (struct btrfs_fs_info *fs_info)
@@ -114,7 +114,7 @@ static void btrfs_handle_error(struct btrfs_fs_info *fs_info)if (sb->s_flags &MS_RDONLY )return ; -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) { sb->s_flags |=MS_RDONLY ;printk (KERN_INFO "btrfs is forced readonly\n" );/*
fs/btrfs/transaction.c +4 -5
@@ -62,7 +62,7 @@ static noinline int join_transaction(struct btrfs_root *root, int type)spin_lock (&fs_info->trans_lock); loop:/* The file system has been taken offline. No new transactions. */ -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);return -EROFS ; }
@@ -114,7 +114,7 @@ loop:kmem_cache_free (btrfs_transaction_cachep, cur_trans); cur_trans = fs_info->running_transaction; goto loop; - }else if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { + }else if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);kmem_cache_free (btrfs_transaction_cachep, cur_trans);return -EROFS ;
@@ -302,7 +302,7 @@ start_transaction(struct btrfs_root *root, u64 num_items, int type,int ret;u64 qgroup_reserved =0 ; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return ERR_PTR (-EROFS );if (current->journal_info) {
@@ -635,9 +635,8 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,btrfs_run_delayed_iputs (root);if (trans->aborted || - root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) err = -EIO ; - }assert_qgroups_uptodate (trans);memset (trans,0 ,sizeof (*trans));
Signature · 1 line
1.7.11.7
Quoted 3 lines · Miao Xie
There is no lock to protect
fs_info->fs_state, it will introduce some problems, such as the value may be covered by the other task when several tasks modify it. Now we use bit operation for it to fix the above problem.
Can you please describe in more detail how does that happen and to what problems it leads?
thanks,
david
Quoted 152 lines
Signed-off-by Miao Xie <[email protected]>--- fs/btrfs/ctree.h | 4 +++- fs/btrfs/disk-io.c | 5 +++-- fs/btrfs/file.c | 2 +- fs/btrfs/scrub.c | 2 +- fs/btrfs/super.c | 4 ++-- fs/btrfs/transaction.c | 9 ++++----- 6 files changed, 14 insertions(+), 12 deletions(-)fs/btrfs/ctree.h +3
-1@@ -338,7 +338,9 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)/* * File system states */ +#define BTRFS_FS_STATE_ERROR 0 +/* Super block flags */ /* Errors detected */ #define BTRFS_SUPER_FLAG_ERROR (1ULL << 2)@@ -1540,7 +1542,7 @@ struct btrfs_fs_info {u64 qgroup_seq;/* filesystem state */ -u64 fs_state; +unsigned long fs_state;struct btrfs_delayed_root *delayed_root;fs/btrfs/disk-io.c +3
-2@@ -2196,7 +2196,8 @@ int open_ctree(struct super_block *sb, goto fail_alloc;/* check FS state, whether FS is broken. */ - fs_info->fs_state |=btrfs_super_flags (disk_super); +if (btrfs_super_flags (disk_super) &BTRFS_SUPER_FLAG_ERROR ) +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); ret =btrfs_check_super_valid (fs_info, sb->s_flags &MS_RDONLY );if (ret) {@@ -3354,7 +3355,7 @@ int close_ctree(struct btrfs_root *root)printk (KERN_ERR "btrfs: commit super ret %d\n" , ret); } -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state))btrfs_error_commit_super (root);btrfs_put_block_group_cache (fs_info);fs/btrfs/file.c +1
-1@@ -1531,7 +1531,7 @@ static ssize_t btrfs_file_aio_write(struct kiocb *iocb,* although we have opened a file as writable, we have * to stop this write operation to ensure FS consistency. */ -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) {mutex_unlock (&inode->i_mutex); err = -EROFS ; goto out;fs/btrfs/scrub.c +1
-1@@ -2700,7 +2700,7 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx,int ret;struct btrfs_root *root = sctx->dev_root; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return -EIO ; gen =atomic64_read (&root->fs_info->last_trans_committed);fs/btrfs/super.c +2
-2@@ -98,7 +98,7 @@ static void __save_error_info(struct btrfs_fs_info *fs_info)* today we only save the error info into ram. Long term we'll * also send it down to the disk */ - fs_info->fs_state =BTRFS_SUPER_FLAG_ERROR ; +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); }static void save_error_info (struct btrfs_fs_info *fs_info)@@ -114,7 +114,7 @@ static void btrfs_handle_error(struct btrfs_fs_info *fs_info)if (sb->s_flags &MS_RDONLY )return ; -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) { sb->s_flags |=MS_RDONLY ;printk (KERN_INFO "btrfs is forced readonly\n" );/* fs/btrfs/transaction.c +4
-5@@ -62,7 +62,7 @@ static noinline int join_transaction(struct btrfs_root *root, int type)spin_lock (&fs_info->trans_lock); loop:/* The file system has been taken offline. No new transactions. */ -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);return -EROFS ; }@@ -114,7 +114,7 @@ loop:kmem_cache_free (btrfs_transaction_cachep, cur_trans); cur_trans = fs_info->running_transaction; goto loop; - }else if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { + }else if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);kmem_cache_free (btrfs_transaction_cachep, cur_trans);return -EROFS ;@@ -302,7 +302,7 @@ start_transaction(struct btrfs_root *root, u64 num_items, int type,int ret;u64 qgroup_reserved =0 ; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return ERR_PTR (-EROFS );if (current->journal_info) {@@ -635,9 +635,8 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,btrfs_run_delayed_iputs (root);if (trans->aborted || - root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) err = -EIO ; - }assert_qgroups_uptodate (trans);memset (trans,0 ,sizeof (*trans));Signature · 5 lines
1.7.11.7 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 7 lines · David Sterba
Quoted 3 lines · Miao Xie
There is no lock to protect
fs_info->fs_state, it will introduce some problems, such as the value may be covered by the other task when several tasks modify it. Now we use bit operation for it to fix the above problem.Can you please describe in more detail how does that happen and to what problems it leads?
For example:
Task0 - CPU0 Task1 - CPU1
mov %fs_state rax
or $0x1 rax
mov %fs_state rax
or $0x2 rax
mov rax %fs_state
mov rax %fs_state
The expected value is 3, but in fact, it is 2
Thanks
Miao
Quoted 161 lines
thanks,
davidQuoted 152 lines
Signed-off-by Miao Xie <[email protected]>--- fs/btrfs/ctree.h | 4 +++- fs/btrfs/disk-io.c | 5 +++-- fs/btrfs/file.c | 2 +- fs/btrfs/scrub.c | 2 +- fs/btrfs/super.c | 4 ++-- fs/btrfs/transaction.c | 9 ++++----- 6 files changed, 14 insertions(+), 12 deletions(-)fs/btrfs/ctree.h +3
-1@@ -338,7 +338,9 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)/* * File system states */ +#define BTRFS_FS_STATE_ERROR 0 +/* Super block flags */ /* Errors detected */ #define BTRFS_SUPER_FLAG_ERROR (1ULL << 2)@@ -1540,7 +1542,7 @@ struct btrfs_fs_info {u64 qgroup_seq;/* filesystem state */ -u64 fs_state; +unsigned long fs_state;struct btrfs_delayed_root *delayed_root;fs/btrfs/disk-io.c +3
-2@@ -2196,7 +2196,8 @@ int open_ctree(struct super_block *sb, goto fail_alloc;/* check FS state, whether FS is broken. */ - fs_info->fs_state |=btrfs_super_flags (disk_super); +if (btrfs_super_flags (disk_super) &BTRFS_SUPER_FLAG_ERROR ) +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); ret =btrfs_check_super_valid (fs_info, sb->s_flags &MS_RDONLY );if (ret) {@@ -3354,7 +3355,7 @@ int close_ctree(struct btrfs_root *root)printk (KERN_ERR "btrfs: commit super ret %d\n" , ret); } -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state))btrfs_error_commit_super (root);btrfs_put_block_group_cache (fs_info);fs/btrfs/file.c +1
-1@@ -1531,7 +1531,7 @@ static ssize_t btrfs_file_aio_write(struct kiocb *iocb,* although we have opened a file as writable, we have * to stop this write operation to ensure FS consistency. */ -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) {mutex_unlock (&inode->i_mutex); err = -EROFS ; goto out;fs/btrfs/scrub.c +1
-1@@ -2700,7 +2700,7 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx,int ret;struct btrfs_root *root = sctx->dev_root; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return -EIO ; gen =atomic64_read (&root->fs_info->last_trans_committed);fs/btrfs/super.c +2
-2@@ -98,7 +98,7 @@ static void __save_error_info(struct btrfs_fs_info *fs_info)* today we only save the error info into ram. Long term we'll * also send it down to the disk */ - fs_info->fs_state =BTRFS_SUPER_FLAG_ERROR ; +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); }static void save_error_info (struct btrfs_fs_info *fs_info)@@ -114,7 +114,7 @@ static void btrfs_handle_error(struct btrfs_fs_info *fs_info)if (sb->s_flags &MS_RDONLY )return ; -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) { sb->s_flags |=MS_RDONLY ;printk (KERN_INFO "btrfs is forced readonly\n" );/* fs/btrfs/transaction.c +4
-5@@ -62,7 +62,7 @@ static noinline int join_transaction(struct btrfs_root *root, int type)spin_lock (&fs_info->trans_lock); loop:/* The file system has been taken offline. No new transactions. */ -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);return -EROFS ; }@@ -114,7 +114,7 @@ loop:kmem_cache_free (btrfs_transaction_cachep, cur_trans); cur_trans = fs_info->running_transaction; goto loop; - }else if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { + }else if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);kmem_cache_free (btrfs_transaction_cachep, cur_trans);return -EROFS ;@@ -302,7 +302,7 @@ start_transaction(struct btrfs_root *root, u64 num_items, int type,int ret;u64 qgroup_reserved =0 ; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return ERR_PTR (-EROFS );if (current->journal_info) {@@ -635,9 +635,8 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,btrfs_run_delayed_iputs (root);if (trans->aborted || - root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) err = -EIO ; - }assert_qgroups_uptodate (trans);memset (trans,0 ,sizeof (*trans));Signature · 5 lines
1.7.11.7 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 18 lines · Miao Xie
Quoted 7 lines · David Sterba
Quoted 3 lines · Miao Xie
There is no lock to protect
fs_info->fs_state, it will introduce some problems, such as the value may be covered by the other task when several tasks modify it. Now we use bit operation for it to fix the above problem.Can you please describe in more detail how does that happen and to what problems it leads?
For example:
Task0 - CPU0 Task1 - CPU1
mov %fs_state rax
or $0x1 rax
mov %fs_state rax
or $0x2 rax
mov rax %fs_state
mov rax %fs_state
Quoted 1 line
The expected value is 3, but in fact, it is 2
The code shows that fs_state is only set by open_ctree() and save_error_info(), how could the above race can happen?
Although I'm ok with this as a harmless cleanup patch, I'm afraid the commit log is not persuadable anyway.
thanks,
liubo
Quoted 170 lines
Thanks
MiaoQuoted 149 lines
thanks,
davidQuoted 145 lines
Signed-off-by Miao Xie <[email protected]>--- fs/btrfs/ctree.h | 4 +++- fs/btrfs/disk-io.c | 5 +++-- fs/btrfs/file.c | 2 +- fs/btrfs/scrub.c | 2 +- fs/btrfs/super.c | 4 ++-- fs/btrfs/transaction.c | 9 ++++----- 6 files changed, 14 insertions(+), 12 deletions(-)fs/btrfs/ctree.h +3
-1@@ -338,7 +338,9 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)/* * File system states */ +#define BTRFS_FS_STATE_ERROR 0 +/* Super block flags */ /* Errors detected */ #define BTRFS_SUPER_FLAG_ERROR (1ULL << 2)@@ -1540,7 +1542,7 @@ struct btrfs_fs_info {u64 qgroup_seq;/* filesystem state */ -u64 fs_state; +unsigned long fs_state;struct btrfs_delayed_root *delayed_root;fs/btrfs/disk-io.c +3
-2@@ -2196,7 +2196,8 @@ int open_ctree(struct super_block *sb, goto fail_alloc;/* check FS state, whether FS is broken. */ - fs_info->fs_state |=btrfs_super_flags (disk_super); +if (btrfs_super_flags (disk_super) &BTRFS_SUPER_FLAG_ERROR ) +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); ret =btrfs_check_super_valid (fs_info, sb->s_flags &MS_RDONLY );if (ret) {@@ -3354,7 +3355,7 @@ int close_ctree(struct btrfs_root *root)printk (KERN_ERR "btrfs: commit super ret %d\n" , ret); } -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state))btrfs_error_commit_super (root);btrfs_put_block_group_cache (fs_info);fs/btrfs/file.c +1
-1@@ -1531,7 +1531,7 @@ static ssize_t btrfs_file_aio_write(struct kiocb *iocb,* although we have opened a file as writable, we have * to stop this write operation to ensure FS consistency. */ -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) {mutex_unlock (&inode->i_mutex); err = -EROFS ; goto out;fs/btrfs/scrub.c +1
-1@@ -2700,7 +2700,7 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx,int ret;struct btrfs_root *root = sctx->dev_root; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return -EIO ; gen =atomic64_read (&root->fs_info->last_trans_committed);fs/btrfs/super.c +2
-2@@ -98,7 +98,7 @@ static void __save_error_info(struct btrfs_fs_info *fs_info)* today we only save the error info into ram. Long term we'll * also send it down to the disk */ - fs_info->fs_state =BTRFS_SUPER_FLAG_ERROR ; +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); }static void save_error_info (struct btrfs_fs_info *fs_info)@@ -114,7 +114,7 @@ static void btrfs_handle_error(struct btrfs_fs_info *fs_info)if (sb->s_flags &MS_RDONLY )return ; -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) { sb->s_flags |=MS_RDONLY ;printk (KERN_INFO "btrfs is forced readonly\n" );/* fs/btrfs/transaction.c +4
-5@@ -62,7 +62,7 @@ static noinline int join_transaction(struct btrfs_root *root, int type)spin_lock (&fs_info->trans_lock); loop:/* The file system has been taken offline. No new transactions. */ -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);return -EROFS ; }@@ -114,7 +114,7 @@ loop:kmem_cache_free (btrfs_transaction_cachep, cur_trans); cur_trans = fs_info->running_transaction; goto loop; - }else if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { + }else if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);kmem_cache_free (btrfs_transaction_cachep, cur_trans);return -EROFS ;@@ -302,7 +302,7 @@ start_transaction(struct btrfs_root *root, u64 num_items, int type,int ret;u64 qgroup_reserved =0 ; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return ERR_PTR (-EROFS );if (current->journal_info) {@@ -635,9 +635,8 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,btrfs_run_delayed_iputs (root);if (trans->aborted || - root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) err = -EIO ; - }assert_qgroups_uptodate (trans);Quoted 12 lines
Quoted 7 lines
fs/btrfs/transaction.c +0
-0memset (trans,0 ,sizeof (*trans));Signature · 5 lines
1.7.11.7 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] 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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 19 lines · Liu Bo
Quoted 18 lines · Miao Xie
Quoted 7 lines · David Sterba
Quoted 3 lines · Miao Xie
There is no lock to protect
fs_info->fs_state, it will introduce some problems, such as the value may be covered by the other task when several tasks modify it. Now we use bit operation for it to fix the above problem.Can you please describe in more detail how does that happen and to what problems it leads?
For example:
Task0 - CPU0 Task1 - CPU1
mov %fs_state rax
or $0x1 rax
mov %fs_state rax
or $0x2 rax
mov rax %fs_state
mov rax %fs_state
Quoted 4 lines
Quoted 1 line
The expected value is 3, but in fact, it is 2
The code shows that
fs_stateis only set byopen_ctree()andsave_error_info(), how could the above race can happen?
The reason that the above race can not happen is because there is only one flag currently. But as we know, ->fs_state can be accessed and updated by multi-task, so the current code is error prone, if we add other flags, the above problem will happen to a certainty. (Adding new flags is very likely) So why not write right and robust code at the beginning?
Quoted 2 lines
Although I'm ok with this as a harmless cleanup patch, I'm afraid the commit log is not persuadable anyway.
I think the changelog is right since ->fs_state can be accessed and updated by multi-task actually.
Thanks
Miao
Quoted 179 lines
thanks,
liuboQuoted 166 lines
Thanks
MiaoQuoted 149 lines
thanks,
davidQuoted 145 lines
Signed-off-by Miao Xie <[email protected]>--- fs/btrfs/ctree.h | 4 +++- fs/btrfs/disk-io.c | 5 +++-- fs/btrfs/file.c | 2 +- fs/btrfs/scrub.c | 2 +- fs/btrfs/super.c | 4 ++-- fs/btrfs/transaction.c | 9 ++++----- 6 files changed, 14 insertions(+), 12 deletions(-)fs/btrfs/ctree.h +3
-1@@ -338,7 +338,9 @@ static inline unsigned long btrfs_chunk_item_size(int num_stripes)/* * File system states */ +#define BTRFS_FS_STATE_ERROR 0 +/* Super block flags */ /* Errors detected */ #define BTRFS_SUPER_FLAG_ERROR (1ULL << 2)@@ -1540,7 +1542,7 @@ struct btrfs_fs_info {u64 qgroup_seq;/* filesystem state */ -u64 fs_state; +unsigned long fs_state;struct btrfs_delayed_root *delayed_root;fs/btrfs/disk-io.c +3
-2@@ -2196,7 +2196,8 @@ int open_ctree(struct super_block *sb, goto fail_alloc;/* check FS state, whether FS is broken. */ - fs_info->fs_state |=btrfs_super_flags (disk_super); +if (btrfs_super_flags (disk_super) &BTRFS_SUPER_FLAG_ERROR ) +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); ret =btrfs_check_super_valid (fs_info, sb->s_flags &MS_RDONLY );if (ret) {@@ -3354,7 +3355,7 @@ int close_ctree(struct btrfs_root *root)printk (KERN_ERR "btrfs: commit super ret %d\n" , ret); } -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state))btrfs_error_commit_super (root);btrfs_put_block_group_cache (fs_info);fs/btrfs/file.c +1
-1@@ -1531,7 +1531,7 @@ static ssize_t btrfs_file_aio_write(struct kiocb *iocb,* although we have opened a file as writable, we have * to stop this write operation to ensure FS consistency. */ -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) {mutex_unlock (&inode->i_mutex); err = -EROFS ; goto out;fs/btrfs/scrub.c +1
-1@@ -2700,7 +2700,7 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx,int ret;struct btrfs_root *root = sctx->dev_root; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return -EIO ; gen =atomic64_read (&root->fs_info->last_trans_committed);fs/btrfs/super.c +2
-2@@ -98,7 +98,7 @@ static void __save_error_info(struct btrfs_fs_info *fs_info)* today we only save the error info into ram. Long term we'll * also send it down to the disk */ - fs_info->fs_state =BTRFS_SUPER_FLAG_ERROR ; +set_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state); }static void save_error_info (struct btrfs_fs_info *fs_info)@@ -114,7 +114,7 @@ static void btrfs_handle_error(struct btrfs_fs_info *fs_info)if (sb->s_flags &MS_RDONLY )return ; -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) { sb->s_flags |=MS_RDONLY ;printk (KERN_INFO "btrfs is forced readonly\n" );/* fs/btrfs/transaction.c +4
-5@@ -62,7 +62,7 @@ static noinline int join_transaction(struct btrfs_root *root, int type)spin_lock (&fs_info->trans_lock); loop:/* The file system has been taken offline. No new transactions. */ -if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);return -EROFS ; }@@ -114,7 +114,7 @@ loop:kmem_cache_free (btrfs_transaction_cachep, cur_trans); cur_trans = fs_info->running_transaction; goto loop; - }else if (fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { + }else if (test_bit (BTRFS_FS_STATE_ERROR , &fs_info->fs_state)) {spin_unlock (&fs_info->trans_lock);kmem_cache_free (btrfs_transaction_cachep, cur_trans);return -EROFS ;@@ -302,7 +302,7 @@ start_transaction(struct btrfs_root *root, u64 num_items, int type,int ret;u64 qgroup_reserved =0 ; -if (root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) +if (test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state))return ERR_PTR (-EROFS );if (current->journal_info) {@@ -635,9 +635,8 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,btrfs_run_delayed_iputs (root);if (trans->aborted || - root->fs_info->fs_state &BTRFS_SUPER_FLAG_ERROR ) { +test_bit (BTRFS_FS_STATE_ERROR , &root->fs_info->fs_state)) err = -EIO ; - }assert_qgroups_uptodate (trans);Quoted 12 lines
Quoted 7 lines
fs/btrfs/transaction.c +0
-0memset (trans,0 ,sizeof (*trans));Signature · 5 lines
1.7.11.7 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlQuoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 18 lines · Miao Xie
Quoted 7 lines · David Sterba
Quoted 3 lines · Miao Xie
There is no lock to protect
fs_info->fs_state, it will introduce some problems, such as the value may be covered by the other task when several tasks modify it. Now we use bit operation for it to fix the above problem.Can you please describe in more detail how does that happen and to what problems it leads?
For example:
Task0 - CPU0 Task1 - CPU1
mov %fs_state rax
or $0x1 rax
mov %fs_state rax
or $0x2 rax
mov rax %fs_state
mov rax %fs_state
Quoted 1 line
The expected value is 3, but in fact, it is 2
Yes, generally this happens like this, but lacks connection to btrfs. What's the call stack on the cpus when this happens? This would mean that abort transaction is called at the same time from 2 cpus and 2 different flag bits need to be set on fs_state. As there's only one such flag that's not the case right now.
Losing an error flag here is potentailly bad, so I agree this needs to be fixed and be future proof, although I doubt that the case of 2 simultaneous aborts is even remotely possible. (Personal note: look for such occurences.)
The changelog is not very specific and wants to be improved.
david