hyperv/vss: implement the VSS (Volume Shadow Copy Service)

[honzhan_microsoft.com]

Summary

This feature is to support VSS (Volume Shadow Copy Service) on Hyper-V.

VSS is a service on Hyper-V. Any VM wants to support this feature should implement such service on VM side.
When Hyper-V host wants to do a snapshot for a given FreeBSD VM, it sends request of freezing file system to VM. VM freezes its file system, and responses to Hyper-V. Then Hyper-V starts to take snapshot. After that, Hyper-V will inform VM to thaw the file system.

On FreeBSD side, the VSS driver implements the protocol to communicate with Hyper-V host and a user space daemon (hv_vss_daemon). The daemon hv_vss_daemon conducts the real UFS freeze/thaw.

Submitted by: Hongjiang Zhang <honzhan microsoft com>

[howard0su_gmail.com]

Did we consider the generic apporach to implement suspend inside vfs and VM?

contrib/hyperv/tools/hv_kvp_daemon.c

835 ↗

(On Diff #21288)

Suggest to create a separate patch for this renaming.

contrib/hyperv/tools/hv_vss_daemon.c

22 ↗

(On Diff #21288)

how about zfs? or other filesystem doesn't support suspend?

[honzhan_microsoft.com]

contrib/hyperv/tools/hv_kvp_daemon.c

835 ↗

(On Diff #21288)

hv_kvp_daemon and hv_vss_daemon has something common to share. Error code is one of them. Another is negotiation with host. That is why this patch impacts kvp daemon. Of course, I can separate them to another patch.

contrib/hyperv/tools/hv_vss_daemon.c

22 ↗

(On Diff #21288)

As I know, ZFS does not support it. I need to double check it. If we want to extend this driver to support the other filesystem, what we need to modify is the daemon. So, the effort is not big.

[kib]

Do you have specific questions to me, e.g. about the interfaces used ? I definitely would not read the hv daemon code, but sure can answer questions about the supposed use of the FreeBSD interfaces to VFS/VM.

[sepherosa_gmail.com]

In D8224#170848, @kib wrote:

Do you have specific questions to me, e.g. about the interfaces used ? I definitely would not read the hv daemon code, but sure can answer questions about the supposed use of the FreeBSD interfaces to VFS/VM.

Yeah, that's why I propose him to add you :). Mainly for the UFS freeze syscall.

[honzhan_microsoft.com]

In D8224#170848, @kib wrote:

Do you have specific questions to me, e.g. about the interfaces used ? I definitely would not read the hv daemon code, but sure can answer questions about the supposed use of the FreeBSD interfaces to VFS/VM.

I'm not sure about the usage of API for freeze/thaw is correct or not. Could you please help me review the file of contrib/hyperv/tools/hv_vss_daemon.c?

As I was told once the file handle for "/dev/ufssuspend" was closed, the freezed file system will be thawed. So, here I implement thaw functionality by close the file handle. Is it correct?

[kib]

contrib/hyperv/tools/hv_vss_daemon.c

22 ↗

(On Diff #21288)

This path is already defiined in paths.h.

26 ↗

(On Diff #21288)

Why do you need additional define for 'unopened handle' ? Common unix practice is to mark closed file descriptor with the -1 value, which is very convenient since error return from open(2) is also -1. Below you test for -1 and UNOPENED_HANDLE, which would remove several lines of code if testing only for -1.

49 ↗

(On Diff #21288)

Use ANSI C definitions (freeze(void)).

72 ↗

(On Diff #21288)

space between for and '('.

84 ↗

(On Diff #21288)

This is excessive. The suspend ioctl does race-free sync when needed (after the writes are blocked), and only for the suspended volume.

95 ↗

(On Diff #21288)

This is rather useless label. Why not write return (1); instead of all error = 1; goto handle_error; places ?

103 ↗

(On Diff #21288)

Use ANSI C.

124 ↗

(On Diff #21288)

static void
usage(const char *cmd)
{

   fprintf();

148 ↗

(On Diff #21288)

You silently ignore invalid options or stray arguments there.

171 ↗

(On Diff #21288)

Excessive ().

197 ↗

(On Diff #21288)

Do not do that. errno values and exit(3) codes are different value spaces. Please see sysexits(3) for common exit(3) codes.

250 ↗

(On Diff #21288)

This return is not needed, and it is in fact strange to see use of exit() and return mixed in the main() function.

[honzhan_microsoft.com]

contrib/hyperv/tools/hv_vss_daemon.c

26 ↗

(On Diff #21288)

Yeah, I'll remove UNOPEND_HANDLE.

72 ↗

(On Diff #21288)

Yeah.

84 ↗

(On Diff #21288)

Thanks. That is what I want to confirm.

103 ↗

(On Diff #21288)

Sure.

124 ↗

(On Diff #21288)

Thanks for your example.

148 ↗

(On Diff #21288)

Oh, I need to print usage information for wrong options.

171 ↗

(On Diff #21288)

Ok.

197 ↗

(On Diff #21288)

Thanks for pointing this preferable exit.

250 ↗

(On Diff #21288)

Yes.

[sepherosa_gmail.com]

contrib/hyperv/tools/hv_vss_daemon.8

79 ↗

(On Diff #22069)

It was whacked.

include/Makefile

50 ↗

(On Diff #22069)

Add this after dev/hwpmc. This list is sorted in alphabetic order.

158 ↗

(On Diff #22069)

Probably after evdev, not acpica. ditto

174 ↗

(On Diff #22069)

After evdev.

264 ↗

(On Diff #22069)

After evdev.

sys/dev/hyperv/utilities/hv_snapshot.c

95 ↗

(On Diff #22069)

Use __packed.

764–765 ↗

(On Diff #22069)

callout_reset(..., hz * sec, );

sys/dev/hyperv/utilities/hv_snapshot.h

52 ↗

(On Diff #22069)

Better comment it as: reserved 0.

[mckusick]

I have nothing to add beyond the comments made by kib.