Re: [PATCH 10/11] Btrfs: use bit operation for ->fs_state

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

On tue, 15 Jan 2013 12:03:03 +0800, Liu Bo wrote:
> On Mon, Jan 14, 2013 at 03:50:31PM +0800, Miao Xie wrote:
>> On 	thu, 10 Jan 2013 18:57:35 +0100, David Sterba wrote:
>>> On Thu, Jan 10, 2013 at 08:51:59PM +0800, Miao Xie wrote:
>>>> 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
> 
> The code shows that fs_state is only set by open_ctree() and
> save_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?

> 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

> 
> thanks,
> liubo
> 
>>
>> Thanks
>> Miao
>>
>>>
>>> thanks,
>>> david
>>>
>>>> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
>>>> ---
>>>>  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(-)
>>>>
>>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>>> index c95b539..c34e36e 100644
>>>> --- a/fs/btrfs/ctree.h
>>>> +++ b/fs/btrfs/ctree.h
>>>> @@ -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;
>>>>  
>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>> index cf03a45..d06e50c 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -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);
>>>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>>>> index 3e9fa0e..ec87b69 100644
>>>> --- a/fs/btrfs/file.c
>>>> +++ b/fs/btrfs/file.c
>>>> @@ -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;
>>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>>>> index af0b566..2e91b56 100644
>>>> --- a/fs/btrfs/scrub.c
>>>> +++ b/fs/btrfs/scrub.c
>>>> @@ -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);
>>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>>> index 6f0524d..f714379 100644
>>>> --- a/fs/btrfs/super.c
>>>> +++ b/fs/btrfs/super.c
>>>> @@ -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");
>>>>  		/*
>>>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>>>> index 7999bf8..a950d48 100644
>>>> --- a/fs/btrfs/transaction.c
>>>> +++ b/fs/btrfs/transaction.c
>>>> @@ -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));
>>>> -- 
>>>> 1.7.11.7
>>>> --
>>>> 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
> --
> 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
>