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.
next prev parent reply index Thread overview: 123+ 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-09-23 17:21 ` Bart Van Assche 2019-09-25 9:30 ` Danil Kipnis 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 2019-09-25 23:43 ` Danil Kipnis 2019-09-26 10:00 ` 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-25 22:26 ` Danil Kipnis 2019-09-26 9:55 ` Roman Penyaev 2019-09-26 15:01 ` Bart Van Assche 2019-09-27 8:52 ` Roman Penyaev 2019-09-27 9:32 ` Danil Kipnis 2019-09-27 12:18 ` Danil Kipnis 2019-09-27 16:37 ` Bart Van Assche 2019-09-27 16:50 ` Roman Penyaev 2019-09-27 17:16 ` Bart Van Assche 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 2019-09-23 15:19 ` Danil Kipnis [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 2019-09-26 14:04 ` Jinpu Wang 2019-09-26 15:11 ` Bart Van Assche 2019-09-26 15:25 ` Danil Kipnis 2019-09-26 15:29 ` Bart Van Assche 2019-09-26 15:38 ` Danil Kipnis 2019-09-26 15:42 ` Jinpu Wang [not found] ` <20190620150337.7847-3-jinpuwang@gmail.com> 2019-09-23 17:44 ` [PATCH v4 02/25] ibtrs: public interface header to establish RDMA connections Bart Van Assche 2019-09-25 10:20 ` Danil Kipnis 2019-09-25 15:38 ` Bart Van Assche [not found] ` <20190620150337.7847-7-jinpuwang@gmail.com> 2019-09-23 21:51 ` [PATCH v4 06/25] ibtrs: client: main functionality Bart Van Assche 2019-09-25 17:36 ` Danil Kipnis 2019-09-25 18:55 ` Bart Van Assche 2019-09-25 20:50 ` Danil Kipnis 2019-09-25 21:08 ` Bart Van Assche 2019-09-25 21:16 ` Bart Van Assche 2019-09-25 22:53 ` Danil Kipnis 2019-09-25 23:21 ` Bart Van Assche 2019-09-26 9:16 ` Danil Kipnis [not found] ` <20190620150337.7847-4-jinpuwang@gmail.com> 2019-09-23 22:50 ` [PATCH v4 03/25] ibtrs: private headers with IBTRS protocol structs and helpers Bart Van Assche 2019-09-25 21:45 ` Danil Kipnis 2019-09-25 21:57 ` Bart Van Assche 2019-09-27 8:56 ` Jinpu Wang [not found] ` <20190620150337.7847-5-jinpuwang@gmail.com> 2019-09-23 23:03 ` [PATCH v4 04/25] ibtrs: core: lib functions shared between client and server modules Bart Van Assche 2019-09-27 10:13 ` Jinpu Wang [not found] ` <20190620150337.7847-6-jinpuwang@gmail.com> 2019-09-23 23:05 ` [PATCH v4 05/25] ibtrs: client: private header with client structs and functions Bart Van Assche 2019-09-27 10:18 ` Jinpu Wang [not found] ` <20190620150337.7847-8-jinpuwang@gmail.com> 2019-09-23 23:15 ` [PATCH v4 07/25] ibtrs: client: statistics functions Bart Van Assche 2019-09-27 12:00 ` Jinpu Wang [not found] ` <20190620150337.7847-10-jinpuwang@gmail.com> 2019-09-23 23:21 ` [PATCH v4 09/25] ibtrs: server: private header with server structs and functions Bart Van Assche 2019-09-27 12:04 ` Jinpu Wang [not found] ` <20190620150337.7847-11-jinpuwang@gmail.com> 2019-09-23 23:49 ` [PATCH v4 10/25] ibtrs: server: main functionality Bart Van Assche 2019-09-27 15:03 ` Jinpu Wang 2019-09-27 15:11 ` Bart Van Assche 2019-09-27 15:19 ` Jinpu Wang [not found] ` <20190620150337.7847-12-jinpuwang@gmail.com> 2019-09-23 23:56 ` [PATCH v4 11/25] ibtrs: server: statistics functions Bart Van Assche 2019-10-02 15:15 ` Jinpu Wang 2019-10-02 15:42 ` Leon Romanovsky 2019-10-02 15:45 ` Jinpu Wang 2019-10-02 16:00 ` Leon Romanovsky [not found] ` <20190620150337.7847-13-jinpuwang@gmail.com> 2019-09-24 0:00 ` [PATCH v4 12/25] ibtrs: server: sysfs interface functions Bart Van Assche 2019-10-02 15:11 ` Jinpu Wang
Reply instructions: You may reply publicly 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 public-inbox-index linux-rdma Example config snippet for mirrors 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.git