Btrfs: race free update of super flags
PATCH 1/1 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
--- fs/btrfs/ctree.h | 4 ++-- fs/btrfs/disk-io.c | 2 ++ fs/btrfs/volumes.c | 2 ++ 3 files changed, 6 insertions(+), 2 deletions(-)
fs/btrfs/ctree.h +2 -2
@@ -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.
fs/btrfs/disk-io.c +2 -0
@@ -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)
fs/btrfs/volumes.c +2 -0
@@ -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 ; }
Signature · 1 line
1.7.9.5
Quoted 4 lines · Filipe David Borba Manana
Before updating the super block's flags, which is a non-atomic operation, grab the
super_lockin thefs_infostructure. 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.
fs/btrfs/disk-io.c static int write_all_supers(struct btrfs_root *root, int max_mirrors) · line 3363
Quoted 10 lines
fs/btrfs/disk-io.c +2
-0@@ -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;
fs/btrfs/volumes.c static int btrfs_prepare_sprout(struct btrfs_root *root) · line 1869
Quoted 11 lines
fs/btrfs/volumes.c +2
-0@@ -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
Quoted 36 lines · Zach Brown
Quoted 4 lines · Filipe David Borba Manana
Before updating the super block's flags, which is a non-atomic operation, grab the
super_lockin thefs_infostructure. 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.
fs/btrfs/disk-io.c static int write_all_supers(struct btrfs_root *root, int max_mirrors) · line 3363
Quoted 10 lines
fs/btrfs/disk-io.c +2
-0@@ -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;fs/btrfs/volumes.c static int btrfs_prepare_sprout(struct btrfs_root *root) · line 1869
Quoted 11 lines
fs/btrfs/volumes.c +2
-0@@ -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;
Quoted 2 lines
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.
Quoted 4 lines
What lead you to write this patch?
- z
Quoted 2 lines
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")