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.
