Issue #2776 has been updated by tkusumi.
hi tuxillo, thanks for your reply. > - Is this really needed? Seems like a bit overkill, I don't find dd that low > level. Anyways. > - If needed at all, why have two options (remove header, remove signature) > separately? Just scrap the whole header and be done maybe? > - I would put the hammer_misc.c patch in another patch. > > In any case the code looks clean to me. 1. The code does look overkill and redundant at this point, but I kept it that way as it's only intended to be rfc version at this point. It could make it less redundant if I remove either of the two modes as I mention in below. The implementation has some duplication with other hammer commands like volume-list and info, so it might needs some refactoring to make it less redundant. 2. The purpose is this command is, in short, if it's the hammer command that provides a feature of adding a volume, why is it not providing a way to (somewhat safely) erase the volume. 3. The problem with forcing dd is that you could happen to kill existing filesystem very easily by operation error. I think BSDs in general return i/o error when you try to dd to the volume (either raw block device or some sort of block-level-abstraction like device mapper, or maybe devfs for dfly particularly) that is currently mounted as some filesystem, as following example shows, but this isn't always the case with, say some other OS. Also if you're using raw device paths like /dev/ad0,1,2, this type of operation error is more likely to happen. You could instantly kill the living filesystem with dd. I want to make it robust, portable, and explicit by explicitly providing a way of doing it without depending on external commands whether that's dd or some hex editor. [root@]~# dd if=/dev/zero of=/dev/serno/VB1679fd54-1faff0a7.s1a dd: /dev/serno/VB1679fd54-1faff0a7.s1a: Device busy 4. I've made two modes signature and header, since volume-add only requires that you've erased signature (first 8 bytes of the volume). It could scrap the whole header and be done as you mentioned. Also note that if you use dd, it's not easy to erase the whole header and only the header part, even if you wanted to do it that way for some reason (size of volume header isn't multiple of sector size and you need to know sizeof(hammer_volume_ondisk)). 5. Putting this patch into two series of patches is obviously possible. It does make it easier to review, so I can put it into 2 series of patches. ---------------------------------------- Submit #2776: [PATCH RFC] sbin/hammer: add hammer volume-erase command http://bugs.dragonflybsd.org/issues/2776#change-12565 * Author: tkusumi * Status: In Progress * Priority: Normal * Assignee: tuxillo * Category: Userland * Target version: 4.2.x ---------------------------------------- This RFC patch adds a new hammer command "volume-erase". It erases the hammer volume signature (0xC8414D4DC5523031) or the entire header without using dd. It aims to do the following. Currently hammer doesn't allow you to volume-add a volume with the signature, even if the volume is not currently used (mounted). When adding a volume, ioctl reads the volume header sector and see if the signature exists. If it does exist then the ioctl tells you to erase it using dd on dmesg. This is right behavior as it protects users from possible operation error, however forcing users to manually do dd is too low level IMO. Filesystem should provide a way to do that with its userspace utility and it's not difficult to implement it with some safety measures. There isn't anything new in terms of implementation because hammer's userspace already has all the functionalities to do this. It simply finds existing hammer filesystems using getmntinfo(3) and then check each volume to make sure it's currently not used. If it's ok to erase it overwrites the signature or the entire header with 0 after 5 seconds of grace periods. Specifying "header" keyword makes this command erase whole volume header instead of just signature. Note that hammer volume-del command also erases the signature when removing a device from filesystem and it does that in kernel space. volume-erase command erases the signature by simply memset(3)ing 0 to ondisk structure of the volume header and then write(2) it because it's possible to do it that way with less code change (no need to add another ioctl like HAMMERIOC_ERASE_VOLUME only to expose hammer_clear_volume_header() functionality to userspace). Also note that this patch moves some code from sbin/hammer/cmd_pseudofs.c to sbin/hammer/misc.c as independent functions that are available from hammer userspace, instead of being static functions, as they are general enough for other hammer commands to use. volume-erase command uses them to do user interaction before erasing. ----- some examples // newfs some devices independently and mount /dev/ad1 # newfs_hammer -L TEST /dev/ad1 > /dev/null # newfs_hammer -L TEST /dev/ad2 > /dev/null # newfs_hammer -L TEST /dev/ad3 > /dev/null # mount_hammer /dev/ad1 /HAMMER // erase signature # hammer volume-add /dev/ad2 /HAMMER hammer volume-add ioctl: Inappropriate file type or format # dmesg ... hammer_volume_add: Formatting of valid HAMMER volume TEST denied. Erase with dd or hammer volume-erase! An error occurred: 79 # od -N 16 -tx1 -Ax /dev/ad2 0000000 31 30 52 c5 4d 4d 41 c8 00 00 04 00 00 00 00 00 0000010 # hammer volume-erase /dev/ad2 You have requested that volume signature of /dev/ad2 be erased Do you really want to do this? y Erasing volume signature of /dev/ad2 5 4 3 2 1 Erased # od -N 16 -tx1 -Ax /dev/ad2 0000000 00 00 00 00 00 00 00 00 00 00 04 00 00 00 00 00 0000010 # hammer volume-add /dev/ad2 /HAMMER # hammer volume-list /HAMMER /dev/ad1 /dev/ad2 // erase the entire header # hammer volume-add /dev/ad3 /HAMMER hammer volume-add ioctl: Inappropriate file type or format # dmesg ... hammer_volume_add: Formatting of valid HAMMER volume TEST denied. Erase with dd or hammer volume-erase! An error occurred: 79 hammer_volume_add: Formatting of valid HAMMER volume TEST denied. Erase with dd or hammer volume-erase! An error occurred: 79 # od -N 1928 -tx1 -Ax /dev/ad3 0000000 31 30 52 c5 4d 4d 41 c8 00 00 04 00 00 00 00 00 0000010 00 00 04 04 00 00 00 00 00 00 04 14 00 00 00 00 0000020 00 00 00 00 19 00 00 00 00 00 00 00 00 00 00 00 0000030 56 1d 07 07 21 a3 e4 11 8f a0 09 00 27 00 47 c3 0000040 ac 63 dc 61 38 6e dc 11 85 13 01 30 1b b8 a9 f5 0000050 54 45 53 54 00 00 00 00 00 00 00 00 00 00 00 00 0000060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 * ... # hammer volume-erase /dev/ad3 header You have requested that volume header of /dev/ad3 be erased Do you really want to do this? y Erasing volume header of /dev/ad3 5 4 3 2 1 Erased # od -N 1928 -tx1 -Ax /dev/ad3 0000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 * 0000780 # hammer volume-add /dev/ad3 /HAMMER # hammer volume-list /HAMMER /dev/ad1 /dev/ad2 /dev/ad3 // try to erase an already erased volume # umount /HAMMER # hammer volume-erase /dev/ad3 header You have requested that volume header of /dev/ad3 be erased Do you really want to do this? y Erasing volume header of /dev/ad3 5 4 3 2 1 Erased # hammer volume-erase /dev/ad3 signature hammer volume-erase: /dev/ad3 is already erased # hammer volume-erase /dev/ad3 header hammer volume-erase: /dev/ad3 is already erased // try to erase a volume used by mounted HAMMER filesystem # mount | grep ROOT ROOT on / (hammer, local) # hammer volume-list / /dev/serno/VB1679fd54-1faff0a7.s1d # hammer volume-erase /dev/serno/VB1679fd54-1faff0a7.s1d hammer volume-erase: /dev/serno/VB1679fd54-1faff0a7.s1d is used by / # echo $? 1 ---Files-------------------------------- 0001-sbin-hammer-add-hammer-volume-erase-command.patch (14.7 KB) -- You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: http://bugs.dragonflybsd.org/my/account
