Re: [PATCH 2/2] nbd: add support for nbd as root device

[Eric Blake]

On 6/12/19 11:31 AM, roman.stratiienko@xxxxxxxxxxxxxxx wrote:
> From: Roman Stratiienko <roman.stratiienko@xxxxxxxxxxxxxxx>
> 
> Adding support to nbd to use it as a root device. This code essentially
> provides a minimal nbd-client implementation within the kernel. It opens
> a socket and makes the negotiation with the server. Afterwards it passes
> the socket to the normal nbd-code to handle the connection.
> 
> The arguments for the server are passed via kernel command line.
> The kernel command line has the format
> 'nbdroot=[<SERVER_IP>:]<SERVER_PORT>/<EXPORT_NAME>'.
> SERVER_IP is optional. If it is not available it will use the
> root_server_addr transmitted through DHCP.
> 
> Based on those arguments, the connection to the server is established
> and is connected to the nbd0 device. The rootdevice therefore is
> root=/dev/nbd0.
> 
> Patch was initialy posted by Markus Pargmann <mpa@xxxxxxxxxxxxxx>
> and can be found at https://lore.kernel.org/patchwork/patch/532556/
> 
> Change-Id: I78f7313918bf31b9dc01a74a42f0f068bede312c
> Signed-off-by: Roman Stratiienko <roman.stratiienko@xxxxxxxxxxxxxxx>
> Reviewed-by: Aleksandr Bulyshchenko <A.Bulyshchenko@xxxxxxxxxxxxxxx>

> +static int nbd_connection_negotiate(struct socket *sock, char *export_name,
> +				    size_t *rsize, u16 *nflags)
> +{

> +
> +	ret = sock_xmit_buf(sock, 0, &flags, sizeof(flags));
> +	if (ret < 0)
> +		return ret;
> +
> +	*nflags = ntohs(flags);
> +
> +	client_flags = 0;

Why not reply with NBD_FLAG_C_FIXED_NEWSTYLE? Which in turn would be
necessary...

> +
> +	ret = sock_xmit_buf(sock, 1, &client_flags, sizeof(client_flags));
> +	if (ret < 0)
> +		return ret;
> +
> +	magic = cpu_to_be64(nbd_opts_magic);
> +	ret = sock_xmit_buf(sock, 1, &magic, sizeof(magic));
> +	if (ret < 0)
> +		return ret;
> +
> +	opt = htonl(NBD_OPT_EXPORT_NAME);

Why not use NBD_OPT_GO?  That's a better interface (better error
recovery, and some servers can even use it to advertise preferred block
sizing - particularly if a server insists on a 4k minimum block instead
of 512).

...using NBD_OPT_GO would require checking that the server advertised
FIXED_NEWSTYLE as well as replying that we intend to rely on it.

Otherwise the idea looks interesting to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature