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

[Filipe David Manana <fdmanana@gmail.com>]

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