On 19.10.2018 17:18, David Sterba wrote: > On Thu, Oct 11, 2018 at 06:03:20PM +0300, Nikolay Borisov wrote: >> Here is the second posting of the fsid change support for the kernel. For >> background information you can refer to v1 [0]. The main changes in this version >> are around the handling of possible split-brain scenarios. I've changed a bit >> how the userspace code works and now the process is split among 2 transactions. >> The first one flagging "we are about to change fsid" and once it's persisted on >> all disks a second one does the actual change. This of course is not enough >> to guarantee full consistency so I had to extend the device scanning to >> gracefully handle such cases. I believe I have covered everything but more >> review will be appreciated. > > All the cases seem to be covered. Do you intend to add the design > document somewhere? The references in the code seem stale and puzzling. I believe I have the critical portions of the design document (i.e the handling of various cases) described in each of the 3 patches that add split-brain handling. The doc could be added to the btrfs-dev-doc repo I guess. > >> So patch 1 implements the basic functionality with no split-brain handling >> whatsoever. Patch 2 is a minor cleanup. Patch 3 deals with a split-brain that >> can occur if power loss happens during the initial transaction (the one setting >> the beginning flag). Patch 4 adds some information that is needed in the last 2 >> patches. Patch 5 handles failure between transaction 1 and transaction 2 and >> finally patch 6 handles the case of power loss during transaction 1 but for an >> fs which has already undergone at least one successful fsid change. More >> details about the exact failure modes are in each respective patch. >> >> One thing which could be improved but I ran out of ideas is the naming of the >> ancillary functions - find_fsid_inprogress and find_fsid_changed. > > Hm, no better ideas so it can stay and be changed later. > >> I've actually tested the split-brain handing code with specially crafted images. >> They will be part of the user-space submissions and I believe I have full >> coverage for that. > > Perfect, thanks. > > Now the bad news from me :) There are several coding style and style > issues all over the patches so I'll list them here. > > * BTRFS_SUPER_FLAG_CHANGING_FSID_v2 -- V2 with uppercase V, as other > versioned symbols > > * all references in changelogs or comments should refer to the super flag > as CHANGING_FSID_v2, not FSID_CHANGING, not just CHANGING_FSID, nor > FSID_CHANGE. This is because we want to be able to search for it and > find all occurences > > * error messages should stick to the preferred format, > https://btrfs.wiki.kernel.org/index.php/Development_notes#Error_messages > > * comments referring to UUID should use "UUID", there's a mixt of > both ways added by the patches, error messages should use the > uppercase form too > > * find_fsid - type and name should be on one line, parameters on the > next if they don't fit > > * device_list_add - missing space before = > > As these are not functional problems, I'll add the patchset to for-next > for testing. >
