Hi Anand, first of all, thanks for the review! More comments inline below. On 23/08/2023 13:31, Anand Jain wrote: > [...] > On 8/3/23 23:43, Guilherme G. Piccoli wrote: >> Btrfs doesn't currently support to mount 2 different devices holding the >> same filesystem - the fsid is used as a unique identifier in the driver. > > fsid is for the external frontend, systemd, and udev stuff; > metadata_uuid pertains to the actual btrfs on-disk. > True, agreed - but I guess my phrase is not wrong per se, right? It's more like an "abstraction" of the concept...I'm in fact talking about the fsid as the exposed entity to the system, like for udev, etc. I guess I'll keep it on V3 if you don't oppose. > [...] >> one of the reasons for which is not trivial supporting this case >> on btrfs is due to its multi-device filesystem nature, native RAID, etc. > > How is it related to the multi-device aspect? The main limitation is > that a disk image can be cloned without maintaining a unique device- > uuid. > Oh okay, I'll rephrase / drop this part, you're right - thanks. > [...] >> Without this support, it's not safe for users to keep the >> same "image version" in both A and B partitions, a setup that is quite >> common for development, for example. Also, as a big bonus, it allows fs >> integrity check based on block devices for RO devices (whereas currently >> it is required that both have different fsid, breaking the block device >> hash comparison). > > Does it apply to smaller disk images? Otherwise, it will be very > time-consuming. Just curious, how is the checksum verified for the entire > disk? (Btrfs might provide a checksum tree-based solution at > some point.) > The disk image is currently 5G, NVMe device - it's usually fast in our case. After discussing internally with the folks knowledgeable about the update process, it seems the Desync tool is responsible for that, taking an index chunk-comparison during the update. Based in my quick check, seems the codes related to that are here: https://github.com/folbricht/desync/blob/master/verifyindex.go https://github.com/folbricht/desync/blob/master/index.go >> + */ >> + if (btrfs_fs_compat_ro(fs_info, SINGLE_DEV)) >> + fsid = fs_info->fs_devices->metadata_uuid; >> + else >> + fsid = fs_info->fs_devices->fsid; > > Below alloc_fs_device(), fsid is still being kept equal to metadata_uuid > in memory for single_dev. So, this distinction is unnecessary. > > >> + >> + if (memcmp(fsid, sb->fsid, BTRFS_FSID_SIZE)) { > > David prefers memcmp to be either compared to == or != to 0 > depending on the requirement. > OK, thanks - I'll experiment dropping this part, and I see you have code present in for-next, changing this specific memcmp. So hopefully don't need my code anymore but if we do, I'll keep the convention =) > [...] >> -static u8 *btrfs_sb_metadata_uuid_or_null(struct btrfs_super_block *sb) >> +static u8 *btrfs_sb_metadata_uuid_single_dev(struct btrfs_super_block *sb, >> + bool has_metadata_uuid, >> + bool single_dev) >> { >> - bool has_metadata_uuid = (btrfs_super_incompat_flags(sb) & >> - BTRFS_FEATURE_INCOMPAT_METADATA_UUID); >> + if (has_metadata_uuid || single_dev) >> + return sb->metadata_uuid; >> >> - return has_metadata_uuid ? sb->metadata_uuid : NULL; >> + return NULL; >> } >> >> u8 *btrfs_sb_fsid_ptr(struct btrfs_super_block *sb) > > You can rebase the code onto the latest misc-next branch. > This is because we have dropped the function > btrfs_sb_metadata_uuid_or_null() in the final integration. > [...] >> - btrfs_sb_metadata_uuid_or_null(disk_super)); >> + btrfs_sb_metadata_uuid_single_dev(disk_super, >> + has_metadata_uuid, single_dev)); > > I think it is a good idea to rebase on latest misc-next and add the > below patch, as the arguments of alloc_fs_device() have been simplified. > > [PATCH resend] btrfs: simplify alloc_fs_devices() remove arg2 > > Very good idea, thanks for pointing that, will do it for V3! Cheers, Guilherme
