search

pojntfx / go-nbd

Pure Go NBD server and client library.
Apache License 2.0
346 stars 18 forks source link

Add support for NBD_FLAG_CAN_MULTI_CONN to the client #7

Open pojntfx opened 1 year ago

pojntfx commented 1 year ago

Currently, the server supports advertising multi-conn capabilities (see #4) , but the integrated client is not able to make use of this. If we call NBD_SET_SOCK more times than 1, we just get invalid argument when calling NBD_DO_IT.

If we can get this to work, we'll probably be able to get very significant performance increases in related repos like r3map. Here is how the C version implements it: https://github.com/NetworkBlockDevice/nbd/blob/master/nbd-client.c#L1206-L1239

I basically ported the C version (if the whitespace diff is hidden in go-nbd's client its just +4 lines), it should be a simple enough change in theory.

pojntfx commented 1 year ago

Any ideas what might be going on here @pPanda-beta?

pPanda-beta commented 1 year ago

I used TransmissionFlags: 0b1000_0001_1000_0000 and nbd-client -N default -t 120 -p -R -C 12 localhost 10811 /dev/nbd100 to create the client with multiple connections.

version: This is nbd-client, from nbd 3.24 I think you can add some log lines in your handle function of the nbd-server and then see how your nbd client is different from linux's nbd-client binary.

pPanda-beta commented 1 year ago

Somehow my implementation is not on-par with your commit https://github.com/pojntfx/go-nbd/commit/de4e1be34f67ec13dd1adbc26c8db312a920ce4c

May be due to little endian and big endian mismatch.

So may be NEGOTIATION_REPLY_FLAGS_HAS_FLAGS := 1<<15 // == 0b1000_0000_0000_0000 and NEGOTIATION_REPLY_FLAGS_CAN_MULTI_CONN := 1<<7 // 0b0000_0000_1000_0000. Please note that I also did a mistake of setting both 1<<8 and 1<<7 (0b1000_0001_1000_0000)

pojntfx commented 1 year ago

Hmmm, are you sure you are setting the new flag in the server options (SupportsMultiConn - its the default if Options is nil)? I tagged a hotfix for it, looks like the C client implementation behaves slightly differently to the Go one when it comes to specified request lengths. It works with the C client in the newly tagged v0.3.1:

$ go run ./cmd/go-nbd-example-server-memory .
2023/10/06 01:10:09 Listening on [::]:10809
2023/10/06 01:10:17 1 clients connected
2023/10/06 01:10:25 0 clients connected
2023/10/06 01:10:25 1 clients connected
2023/10/06 01:10:25 2 clients connected
2023/10/06 01:10:25 3 clients connected
2023/10/06 01:10:25 4 clients connected
2023/10/06 01:10:25 5 clients connected
2023/10/06 01:10:25 6 clients connected
2023/10/06 01:10:25 7 clients connected
2023/10/06 01:10:25 8 clients connected
2023/10/06 01:10:25 9 clients connected
2023/10/06 01:10:25 10 clients connected
2023/10/06 01:10:25 11 clients connected
2023/10/06 01:10:25 12 clients connected
$ sudo umount ~/Downloads/mnt; sudo nbd-client -d /dev/nbd1 &&s
echo 'NBD starting' | sudo tee /dev/kmsg && sudo nbd-client -C 12 -N default localhost 10809 /d
ev/nbd1
umount: /home/pojntfx/Downloads/mnt: not mounted.
NBD starting
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Negotiation: ..size = 1024MB
Connected /dev/nbd1
$ sudo mkfs.ext4 /dev/nbd1

mke2fs 1.47.0 (5-Feb-2023)
Creating filesystem with 262144 4k blocks and 65536 inodes
Filesystem UUID: 8be3f81e-501f-48f3-85e4-a6308b2d1d68
Superblock backups stored on blocks: 
        32768, 98304, 163840, 229376

Allocating group tables: done                            
Writing inode tables: done                            
Creating journal (8192 blocks): done
Writing superblocks and filesystem accounting information: 0/done

This works for the C client, but for the Go client when trying the same approach (calling SET_SOCK with multiple sockets, as mentioned in the linked branch) we still get invalid argument for some reason. I wonder whether we might be forced to use Netlink when setting up the client instead of the current ioctl-based approach.

pPanda-beta commented 1 year ago

At least from the documentation it seems by default is the Netlink approach.

       -nonetlink

       -L     Starting  with  version 3.17, nbd-client will default to using the netlink interface to configure an NBD device. This option allows to use the older ioctl() interface to
              configure the device.

              This option is only available if nbd-client was compiled against libnl-genl. If that is not the case, nbd-client will only be able to use the ioctl  interface  (and  the
              option will not be available).

              Note that a future version of nbd-client will require the use of netlink, but it has not yet been decided when that will be the case.
pojntfx commented 1 year ago

Hmmm yeah, it looks like you're right. Switching the client to Netlink would be possible, but would require re-writing significant parts since we currently use ioctl to configure the kernel client.