From: Miao Xie <miaox@cn.fujitsu.com>
To: dsterba@suse.cz
Cc: Linux Btrfs <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH 10/11] Btrfs: use bit operation for ->fs_state
Date: Mon, 14 Jan 2013 15:50:31 +0800 [thread overview]
Message-ID: <50F3B8C7.3000902@cn.fujitsu.com> (raw)
In-Reply-To: <20130110175735.GI20089@twin.jikos.cz>
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
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
>
next prev parent reply other threads:[~2013-01-14 8:00 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-10 12:51 [PATCH 10/11] Btrfs: use bit operation for ->fs_state Miao Xie
2013-01-10 17:57 ` David Sterba
2013-01-14 7:50 ` Miao Xie [this message]
2013-01-15 4:03 ` Liu Bo
2013-01-15 6:03 ` Miao Xie
2013-01-16 13:03 ` David Sterba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50F3B8C7.3000902@cn.fujitsu.com \
--to=miaox@cn.fujitsu.com \
--cc=dsterba@suse.cz \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).