Project

General

Profile

Actions

Submit #2776

closed

[PATCH RFC] sbin/hammer: add hammer volume-erase command

Added by tkusumi about 9 years ago. Updated about 9 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Userland
Target version:
Start date:
01/23/2015
Due date:
% Done:

0%

Estimated time:

Description

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
  1. newfs_hammer -L TEST /dev/ad1 > /dev/null
  2. newfs_hammer -L TEST /dev/ad2 > /dev/null
  3. newfs_hammer -L TEST /dev/ad3 > /dev/null
  4. mount_hammer /dev/ad1 /HAMMER
// erase signature
  1. hammer volume-add /dev/ad2 /HAMMER
    hammer volume-add ioctl: Inappropriate file type or format
  2. dmesg
    ...
    hammer_volume_add: Formatting of valid HAMMER volume TEST denied. Erase with dd or hammer volume-erase!
    An error occurred: 79
  3. 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
  4. 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
  5. 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
  6. hammer volume-add /dev/ad2 /HAMMER
  7. hammer volume-list /HAMMER
    /dev/ad1
    /dev/ad2
// erase the entire header
  1. hammer volume-add /dev/ad3 /HAMMER
    hammer volume-add ioctl: Inappropriate file type or format
  2. 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
  3. 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 *
    ...
  4. 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
  5. 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
  6. hammer volume-add /dev/ad3 /HAMMER
  7. hammer volume-list /HAMMER
    /dev/ad1
    /dev/ad2
    /dev/ad3
// try to erase an already erased volume
  1. umount /HAMMER
  2. 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
  3. hammer volume-erase /dev/ad3 signature
    hammer volume-erase: /dev/ad3 is already erased
  4. hammer volume-erase /dev/ad3 header
    hammer volume-erase: /dev/ad3 is already erased
// try to erase a volume used by mounted HAMMER filesystem
  1. mount | grep ROOT
    ROOT on / (hammer, local)
  2. hammer volume-list /
    /dev/serno/VB1679fd54-1faff0a7.s1d
  3. hammer volume-erase /dev/serno/VB1679fd54-1faff0a7.s1d
    hammer volume-erase: /dev/serno/VB1679fd54-1faff0a7.s1d is used by /
  4. echo $?
    1

Files

Actions #1

Updated by tuxillo about 9 years ago

  • Status changed from New to In Progress
  • Assignee set to tuxillo
  • Target version set to 4.2

Hi,

Just a few comments:

- 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.

Cheers,
Antonio Huete

Actions #2

Updated by tkusumi about 9 years ago

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.

Actions #3

Updated by tkusumi about 9 years ago

  • Status changed from In Progress to Closed

Closing the ticket. After having some talk on irc with tuxillo and a few other people on libhammer's goal, I've realized code integration of libhammer (which did exist for years but not completed) is the first thing to be done.

If that's ever finished implementing a new hammer command as well as outbox application will be much easier. This command should be implemented in less than +100 lines of code.

However I'll be working on it on my local repository for now before I ever start to send them. There isn't anything technically difficult on libhammer code integration since all the hammer utilities do use ioctls everywhere, however it needs to be carefully done so as not to break anything that's already working as production. If the integration ever gets matured and stabilized I may send them.

Actions

Also available in: Atom PDF