Re: [PATCH v4 15/25] ibnbd: private headers with IBNBD protocol structs and helpers

[Bart Van Assche <bvanassche@acm.org>]

On 6/20/19 8:03 AM, Jack Wang wrote:
> +#define ibnbd_log(fn, dev, fmt, ...) ({				\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(				\
> +			typeof(dev), struct ibnbd_clt_dev *),		\
> +		fn("<%s@%s> " fmt, (dev)->pathname,			\
> +		(dev)->sess->sessname,					\
> +		   ##__VA_ARGS__),					\
> +		__builtin_choose_expr(					\
> +			__builtin_types_compatible_p(typeof(dev),	\
> +					struct ibnbd_srv_sess_dev *),	\
> +			fn("<%s@%s>: " fmt, (dev)->pathname,		\
> +			   (dev)->sess->sessname, ##__VA_ARGS__),	\
> +			unknown_type()));				\
> +})

Please remove the __builtin_choose_expr() / 
__builtin_types_compatible_p() construct and split this macro into two 
macros or inline functions: one for struct ibnbd_clt_dev and another one 
for struct ibnbd_srv_sess_dev.

> +#define IBNBD_PROTO_VER_MAJOR 2
> +#define IBNBD_PROTO_VER_MINOR 0
> +
> +#define IBNBD_PROTO_VER_STRING __stringify(IBNBD_PROTO_VER_MAJOR) "." \
> +			       __stringify(IBNBD_PROTO_VER_MINOR)
> +
> +#ifndef IBNBD_VER_STRING
> +#define IBNBD_VER_STRING __stringify(IBNBD_PROTO_VER_MAJOR) "." \
> +			 __stringify(IBNBD_PROTO_VER_MINOR)

Upstream code should not have a version number.

> +/* TODO: should be configurable */
> +#define IBTRS_PORT 1234

How about converting this macro into a kernel module parameter?

> +enum ibnbd_access_mode {
> +	IBNBD_ACCESS_RO,
> +	IBNBD_ACCESS_RW,
> +	IBNBD_ACCESS_MIGRATION,
> +};

Some more information about what IBNBD_ACCESS_MIGRATION represents would 
be welcome.

> +#define _IBNBD_FILEIO  0
> +#define _IBNBD_BLOCKIO 1
> +#define _IBNBD_AUTOIO  2
 >
> +enum ibnbd_io_mode {
> +	IBNBD_FILEIO = _IBNBD_FILEIO,
> +	IBNBD_BLOCKIO = _IBNBD_BLOCKIO,
> +	IBNBD_AUTOIO = _IBNBD_AUTOIO,
> +};

Since the IBNBD_* and _IBNBD_* constants have the same numerical value, 
are the former constants really necessary?

> +/**
> + * struct ibnbd_msg_sess_info - initial session info from client to server
> + * @hdr:		message header
> + * @ver:		IBNBD protocol version
> + */
> +struct ibnbd_msg_sess_info {
> +	struct ibnbd_msg_hdr hdr;
> +	u8		ver;
> +	u8		reserved[31];
> +};

Since the wire protocol is versioned, is it really necessary to add 31 
reserved bytes?

> +struct ibnbd_msg_sess_info_rsp {
> +	struct ibnbd_msg_hdr hdr;
> +	u8		ver;
> +	u8		reserved[31];
> +};

Same comment here.

> +/**
> + * struct ibnbd_msg_open_rsp - response message to IBNBD_MSG_OPEN
> + * @hdr:		message header
> + * @nsectors:		number of sectors

What is the size of a single sector?

> + * @device_id:		device_id on server side to identify the device

Please use the same order for the members in the kernel-doc header as in 
the structure.

> + * @queue_flags:	queue_flags of the device on server side

Where is the queue_flags member?

> + * @discard_granularity: size of the internal discard allocation unit
> + * @discard_alignment: offset from internal allocation assignment
> + * @physical_block_size: physical block size device supports
> + * @logical_block_size: logical block size device supports

What is the unit for these four members?

> + * @max_segments:	max segments hardware support in one transfer

Does 'hardware' refer to the RDMA adapter that transfers the IBNBD 
message or to the storage device? In the latter case, I assume that 
transfer refers to a DMA transaction?

> + * @io_mode:		io_mode device is opened.

Should a reference to enum ibnbd_io_mode be added?

> +	u8			__padding[10];

Why ten padding bytes? Does alignment really matter for a data structure 
like this one?

> +/**
> + * struct ibnbd_msg_io_old - message for I/O read/write for
> + * ver < IBNBD_PROTO_VER_MAJOR
> + * This structure is there only to know the size of the "old" message format
> + * @hdr:	message header
> + * @device_id:	device_id on server side to find the right device
> + * @sector:	bi_sector attribute from struct bio
> + * @rw:		bitmask, valid values are defined in enum ibnbd_io_flags
> + * @bi_size:    number of bytes for I/O read/write
> + * @prio:       priority
> + */
> +struct ibnbd_msg_io_old {
> +	struct ibnbd_msg_hdr hdr;
> +	__le32		device_id;
> +	__le64		sector;
> +	__le32		rw;
> +	__le32		bi_size;
> +};

Since this is the first version of IBNBD that is being sent upstream, I 
think that ibnbd_msg_io_old should be left out.

> +
> +/**
> + * struct ibnbd_msg_io - message for I/O read/write
> + * @hdr:	message header
> + * @device_id:	device_id on server side to find the right device
> + * @sector:	bi_sector attribute from struct bio
> + * @rw:		bitmask, valid values are defined in enum ibnbd_io_flags

enum ibnbd_io_flags doesn't look like a bitmask but rather like a bit 
field (https://en.wikipedia.org/wiki/Bit_field)?

> +static inline u32 ibnbd_to_bio_flags(u32 ibnbd_flags)
> +{
> +	u32 bio_flags;

The names ibnbd_flags and bio_flags are confusing since these two 
variables not only contain flags but also an operation. How about 
changing 'flags' into 'opf' or 'op_flags'?

> +static inline const char *ibnbd_io_mode_str(enum ibnbd_io_mode mode)
> +{
> +	switch (mode) {
> +	case IBNBD_FILEIO:
> +		return "fileio";
> +	case IBNBD_BLOCKIO:
> +		return "blockio";
> +	case IBNBD_AUTOIO:
> +		return "autoio";
> +	default:
> +		return "unknown";
> +	}
> +}
> +
> +static inline const char *ibnbd_access_mode_str(enum ibnbd_access_mode mode)
> +{
> +	switch (mode) {
> +	case IBNBD_ACCESS_RO:
> +		return "ro";
> +	case IBNBD_ACCESS_RW:
> +		return "rw";
> +	case IBNBD_ACCESS_MIGRATION:
> +		return "migration";
> +	default:
> +		return "unknown";
> +	}
> +}

These two functions are not in the hot path and hence should not be 
inline functions.

Note: I plan to review the entire patch series but it may take some time 
before I have finished reviewing the entire patch series.

Bart.