thr3ads.net - Btrfs devel - [PATCH] Btrfs: race free update of super flags [Aug 2013]

If this information is useful, please help other people find it:
Share via:

Filipe David Borba Manana

2013-Aug-03 18:43 UTC

[PATCH] Btrfs: race free update of super flags

Before updating the super block''s flags, which is a non-atomic
operation, grab the super_lock in the fs_info structure. At
the moment only 2 different code paths can update these flags
in parallel:

1) when adding a new device
2) writing all super block copies to disk

Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
---
 fs/btrfs/ctree.h   |    4 ++--
 fs/btrfs/disk-io.c |    2 ++
 fs/btrfs/volumes.c |    2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index cbb1263..fd47c02 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1375,8 +1375,8 @@ struct btrfs_fs_info {
 	wait_queue_head_t async_submit_wait;
 
 	/*
-	 * Used to protect the incompat_flags, compat_flags, compat_ro_flags
-	 * when they are updated.
+	 * Used to protect the incompat_flags, compat_flags, compat_ro_flags,
+	 * flags and label when they are updated.
 	 *
 	 * Because we do not clear the flags for ever, so we needn''t use
 	 * the lock on the read side.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b71e882..3900c36 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3357,8 +3357,10 @@ static int write_all_supers(struct btrfs_root *root, int
max_mirrors)
 		memcpy(dev_item->uuid, dev->uuid, BTRFS_UUID_SIZE);
 		memcpy(dev_item->fsid, dev->fs_devices->fsid, BTRFS_UUID_SIZE);
 
+		spin_lock(&root->fs_info->super_lock);
 		flags = btrfs_super_flags(sb);
 		btrfs_set_super_flags(sb, flags | BTRFS_HEADER_FLAG_WRITTEN);
+		spin_unlock(&root->fs_info->super_lock);
 
 		ret = write_dev_supers(dev, sb, do_barriers, 0, max_mirrors);
 		if (ret)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 090f57c..8c67bd7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1862,9 +1862,11 @@ static int btrfs_prepare_sprout(struct btrfs_root *root)
 	generate_random_uuid(fs_devices->fsid);
 	memcpy(root->fs_info->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
 	memcpy(disk_super->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
+	spin_lock(&root->fs_info->super_lock);
 	super_flags = btrfs_super_flags(disk_super) &
 		      ~BTRFS_SUPER_FLAG_SEEDING;
 	btrfs_set_super_flags(disk_super, super_flags);
+	spin_unlock(&root->fs_info->super_lock);
 
 	return 0;
 }
-- 
1.7.9.5

--
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

Zach Brown

2013-Aug-05 22:10 UTC

head link

Re: [PATCH] Btrfs: race free update of super flags

On Sat, Aug 03, 2013 at 07:43:17PM +0100, Filipe David Borba Manana
wrote:> Before updating the super block''s flags, which is a non-atomic
> operation, grab the super_lock in the fs_info structure. At
> the moment only 2 different code paths can update these flags
> in parallel:
Hmm.  You say "the" super block, but it''s not clear to me
that it''s that
simple.
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3357,8 +3357,10 @@ static int write_all_supers(struct btrfs_root *root,
int max_mirrors)
>  		memcpy(dev_item->uuid, dev->uuid, BTRFS_UUID_SIZE);
>  		memcpy(dev_item->fsid, dev->fs_devices->fsid, BTRFS_UUID_SIZE);
>  
> +		spin_lock(&root->fs_info->super_lock);
>  		flags = btrfs_super_flags(sb);
>  		btrfs_set_super_flags(sb, flags | BTRFS_HEADER_FLAG_WRITTEN);
> +		spin_unlock(&root->fs_info->super_lock);
        sb = root->fs_info->super_for_commit;
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1862,9 +1862,11 @@ static int btrfs_prepare_sprout(struct btrfs_root
*root)
>  	generate_random_uuid(fs_devices->fsid);
>  	memcpy(root->fs_info->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
>  	memcpy(disk_super->fsid, fs_devices->fsid, BTRFS_FSID_SIZE);
> +	spin_lock(&root->fs_info->super_lock);
>  	super_flags = btrfs_super_flags(disk_super) &
>  		      ~BTRFS_SUPER_FLAG_SEEDING;
>  	btrfs_set_super_flags(disk_super, super_flags);
> +	spin_unlock(&root->fs_info->super_lock);
        struct btrfs_super_block *disk_super = root->fs_info->super_copy;

Can you explain why the lock is needed given the different super blocks
and the mutexes that each path holds?

What lead you to write this patch?

- z
--
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

Filipe David Manana

2013-Aug-05 22:27 UTC

head link

Re: [PATCH] Btrfs: race free update of super flags

On Mon, Aug 5, 2013 at 11:10 PM, Zach Brown <zab@redhat.com>
wrote:> On Sat, Aug 03, 2013 at 07:43:17PM +0100, Filipe David Borba Manana wrote:
>> Before updating the super block''s flags, which is a non-atomic
>> operation, grab the super_lock in the fs_info structure. At
>> the moment only 2 different code paths can update these flags
>> in parallel:
>
> Hmm.  You say "the" super block, but it''s not clear to
me that it''s that
> simple.
>
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3357,8 +3357,10 @@ static int write_all_supers(struct btrfs_root
*root, int max_mirrors)
>>               memcpy(dev_item->uuid, dev->uuid, BTRFS_UUID_SIZE);
>>               memcpy(dev_item->fsid, dev->fs_devices->fsid,
BTRFS_UUID_SIZE);
>>
>> +             spin_lock(&root->fs_info->super_lock);
>>               flags = btrfs_super_flags(sb);
>>               btrfs_set_super_flags(sb, flags |
BTRFS_HEADER_FLAG_WRITTEN);
>> +             spin_unlock(&root->fs_info->super_lock);
>
>         sb = root->fs_info->super_for_commit;
>
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1862,9 +1862,11 @@ static int btrfs_prepare_sprout(struct
btrfs_root *root)
>>       generate_random_uuid(fs_devices->fsid);
>>       memcpy(root->fs_info->fsid, fs_devices->fsid,
BTRFS_FSID_SIZE);
>>       memcpy(disk_super->fsid, fs_devices->fsid,
BTRFS_FSID_SIZE);
>> +     spin_lock(&root->fs_info->super_lock);
>>       super_flags = btrfs_super_flags(disk_super) &
>>                     ~BTRFS_SUPER_FLAG_SEEDING;
>>       btrfs_set_super_flags(disk_super, super_flags);
>> +     spin_unlock(&root->fs_info->super_lock);
>
>         struct btrfs_super_block *disk_super =
root->fs_info->super_copy;
>
> Can you explain why the lock is needed given the different super blocks
> and the mutexes that each path holds?
I guess got confused, must have thought both referred to
root->fs_info->super_copy.
Thanks for the heads up and review.

So yes, just ignore this as it''s useless.
>
> What lead you to write this patch?
>
> - z--
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

Zach Brown

2013-Aug-05 22:47 UTC

head link

Re: [PATCH] Btrfs: race free update of super flags

> I guess got confused, must have thought both referred to
> root->fs_info->super_copy.
No worries.  I honestly wasn''t sure if I was just missing something :)

- z
("you are in a maze of twisty little super blocks, all alike")
--
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

Btrfs devel - Aug 2013 - [PATCH] Btrfs: race free update of super flags

[PATCH] Btrfs: race free update of super flags

Re: [PATCH] Btrfs: race free update of super flags

Re: [PATCH] Btrfs: race free update of super flags

Re: [PATCH] Btrfs: race free update of super flags