Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: Bart Van Assche <bvanassche@acm.org>
To: Jack Wang <jinpuwang@gmail.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org
Cc: axboe@kernel.dk, hch@infradead.org, sagi@grimberg.me,
	jgg@mellanox.com, dledford@redhat.com,
	danil.kipnis@cloud.ionos.com, rpenyaev@suse.de,
	Roman Pen <roman.penyaev@profitbricks.com>,
	Jack Wang <jinpu.wang@cloud.ionos.com>
Subject: Re: [PATCH v4 15/25] ibnbd: private headers with IBNBD protocol structs and helpers
Date: Fri, 13 Sep 2019 15:10:13 -0700
Message-ID: <4fbad80b-f551-131e-9a5c-a24f1fa98fea@acm.org> (raw)
In-Reply-To: <20190620150337.7847-16-jinpuwang@gmail.com>

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.

  parent reply index

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190620150337.7847-1-jinpuwang@gmail.com>
2019-06-20 15:03 ` [PATCH v4 01/25] sysfs: export sysfs_remove_file_self() Jack Wang
2019-07-09  9:55 ` [PATCH v4 00/25] InfiniBand Transport (IBTRS) and Network Block Device (IBNBD) Danil Kipnis
2019-07-09 11:00   ` Leon Romanovsky
2019-07-09 11:17     ` Greg KH
2019-07-09 11:57       ` Jinpu Wang
2019-07-09 13:32       ` Leon Romanovsky
2019-07-09 15:39       ` Bart Van Assche
2019-07-09 11:37     ` Jinpu Wang
2019-07-09 12:06       ` Jason Gunthorpe
2019-07-09 13:15         ` Jinpu Wang
2019-07-09 13:19           ` Jason Gunthorpe
2019-07-09 14:17             ` Jinpu Wang
2019-07-09 21:27             ` Sagi Grimberg
2019-07-19 13:12               ` Danil Kipnis
2019-07-10 14:55     ` Danil Kipnis
2019-07-09 12:04   ` Jason Gunthorpe
2019-07-09 19:45   ` Sagi Grimberg
2019-07-10 13:55     ` Jason Gunthorpe
2019-07-10 16:25       ` Sagi Grimberg
2019-07-10 17:25         ` Jason Gunthorpe
2019-07-10 19:11           ` Sagi Grimberg
2019-07-11  7:27             ` Danil Kipnis
2019-07-11  8:54     ` Danil Kipnis
2019-07-12  0:22       ` Sagi Grimberg
2019-07-12  7:57         ` Jinpu Wang
2019-07-12 19:40           ` Sagi Grimberg
2019-07-15 11:21             ` Jinpu Wang
2019-07-12 10:58         ` Danil Kipnis
     [not found] ` <20190620150337.7847-26-jinpuwang@gmail.com>
2019-07-09 15:10   ` [PATCH v4 25/25] MAINTAINERS: Add maintainer for IBNBD/IBTRS modules Leon Romanovsky
2019-07-09 15:18     ` Jinpu Wang
2019-07-09 15:51       ` Leon Romanovsky
2019-09-13 23:56   ` Bart Van Assche
2019-09-19 10:30     ` Jinpu Wang
     [not found] ` <20190620150337.7847-16-jinpuwang@gmail.com>
2019-09-13 22:10   ` Bart Van Assche [this message]
2019-09-15 14:30     ` [PATCH v4 15/25] ibnbd: private headers with IBNBD protocol structs and helpers Jinpu Wang
2019-09-16  5:27       ` Leon Romanovsky
2019-09-16 13:45         ` Bart Van Assche
2019-09-17 15:41           ` Leon Romanovsky
2019-09-17 15:52             ` Jinpu Wang
2019-09-16  7:08       ` Danil Kipnis
2019-09-16 14:57       ` Jinpu Wang
2019-09-16 17:25         ` Bart Van Assche
2019-09-17 12:27           ` Jinpu Wang
2019-09-16 15:39       ` Jinpu Wang
2019-09-18 15:26         ` Bart Van Assche
2019-09-18 16:11           ` Jinpu Wang
     [not found] ` <20190620150337.7847-17-jinpuwang@gmail.com>
2019-09-13 22:25   ` [PATCH v4 16/25] ibnbd: client: private header with client structs and functions Bart Van Assche
2019-09-17 16:36     ` Jinpu Wang
     [not found] ` <20190620150337.7847-18-jinpuwang@gmail.com>
2019-09-13 23:46   ` [PATCH v4 17/25] ibnbd: client: main functionality Bart Van Assche
2019-09-16 14:17     ` Danil Kipnis
2019-09-16 16:46       ` Bart Van Assche
2019-09-17 11:39         ` Danil Kipnis
2019-09-18  7:14           ` Danil Kipnis
2019-09-18 15:47             ` Bart Van Assche
2019-09-20  8:29               ` Danil Kipnis
2019-09-17 13:09     ` Jinpu Wang
2019-09-17 16:46       ` Bart Van Assche
2019-09-18 12:02         ` Jinpu Wang
2019-09-18 16:05     ` Jinpu Wang
2019-09-14  0:00   ` Bart Van Assche
     [not found] ` <20190620150337.7847-25-jinpuwang@gmail.com>
2019-09-13 23:58   ` [PATCH v4 24/25] ibnbd: a bit of documentation Bart Van Assche
2019-09-18 12:22     ` Jinpu Wang
     [not found] ` <20190620150337.7847-19-jinpuwang@gmail.com>
2019-09-18 16:28   ` [PATCH v4 18/25] ibnbd: client: sysfs interface functions Bart Van Assche
2019-09-19 15:55     ` Jinpu Wang
     [not found] ` <20190620150337.7847-21-jinpuwang@gmail.com>
2019-09-18 17:41   ` [PATCH v4 20/25] ibnbd: server: main functionality Bart Van Assche
2019-09-20  7:36     ` Danil Kipnis
2019-09-20 15:42       ` Bart Van Assche
     [not found] ` <20190620150337.7847-22-jinpuwang@gmail.com>
2019-09-18 21:46   ` [PATCH v4 21/25] ibnbd: server: functionality for IO submission to file or block dev Bart Van Assche

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4fbad80b-f551-131e-9a5c-a24f1fa98fea@acm.org \
    --to=bvanassche@acm.org \
    --cc=axboe@kernel.dk \
    --cc=danil.kipnis@cloud.ionos.com \
    --cc=dledford@redhat.com \
    --cc=hch@infradead.org \
    --cc=jgg@mellanox.com \
    --cc=jinpu.wang@cloud.ionos.com \
    --cc=jinpuwang@gmail.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=roman.penyaev@profitbricks.com \
    --cc=rpenyaev@suse.de \
    --cc=sagi@grimberg.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org linux-rdma@archiver.kernel.org
	public-inbox-index linux-rdma


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/ public-inbox