* [PATCH] Btrfs: race free update of super flags
@ 2013-08-03 18:43 Filipe David Borba Manana
2013-08-05 22:10 ` Zach Brown
0 siblings, 1 reply; 4+ messages in thread
From: Filipe David Borba Manana @ 2013-08-03 18:43 UTC (permalink / raw)
To: linux-btrfs; +Cc: Filipe David Borba Manana
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs: race free update of super flags
2013-08-03 18:43 [PATCH] Btrfs: race free update of super flags Filipe David Borba Manana
@ 2013-08-05 22:10 ` Zach Brown
2013-08-05 22:27 ` Filipe David Manana
0 siblings, 1 reply; 4+ messages in thread
From: Zach Brown @ 2013-08-05 22:10 UTC (permalink / raw)
To: Filipe David Borba Manana; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs: race free update of super flags
2013-08-05 22:10 ` Zach Brown
@ 2013-08-05 22:27 ` Filipe David Manana
2013-08-05 22:47 ` Zach Brown
0 siblings, 1 reply; 4+ messages in thread
From: Filipe David Manana @ 2013-08-05 22:27 UTC (permalink / raw)
To: Zach Brown; +Cc: linux-btrfs@vger.kernel.org
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs: race free update of super flags
2013-08-05 22:27 ` Filipe David Manana
@ 2013-08-05 22:47 ` Zach Brown
0 siblings, 0 replies; 4+ messages in thread
From: Zach Brown @ 2013-08-05 22:47 UTC (permalink / raw)
To: Filipe David Manana; +Cc: linux-btrfs@vger.kernel.org
> 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")
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-08-05 22:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-03 18:43 [PATCH] Btrfs: race free update of super flags Filipe David Borba Manana
2013-08-05 22:10 ` Zach Brown
2013-08-05 22:27 ` Filipe David Manana
2013-08-05 22:47 ` Zach Brown
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).