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

From: Jinpu Wang <jinpu.wang@cloud.ionos.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Jack Wang <jinpuwang@gmail.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	Sagi Grimberg <sagi@grimberg.me>,
	Jason Gunthorpe <jgg@mellanox.com>,
	Doug Ledford <dledford@redhat.com>,
	Danil Kipnis <danil.kipnis@cloud.ionos.com>,
	rpenyaev@suse.de, Roman Pen <roman.penyaev@profitbricks.com>
Subject: Re: [PATCH v4 15/25] ibnbd: private headers with IBNBD protocol structs and helpers
Date: Mon, 16 Sep 2019 16:57:15 +0200	[thread overview]
Message-ID: <CAMGffEmnTG4ixN1Hfy7oY93TgG3qQtF9TkpGzi=BxWm5a2i3Eg@mail.gmail.com> (raw)
In-Reply-To: <CAMGffEnVFHpmDCiazHFX1jwi4=p401T9goSkes3j1AttV0t1Ng@mail.gmail.com>

> > > +#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?
> Seems we can remove _IBNBD_*.
Sorry, checked again,  we defined _IBNBD_* constants to show the right
value for def_io_mode description.
If we remove the _IBNBD_*, then the modinfo shows:
def_io_mode:By default, export devices in blockio(IBNBD_BLOCKIO) or
fileio(IBNBD_FILEIO) mode. (default: IBNBD_BLOCKIO (blockio))
instead of:
parm:           def_io_mode:By default, export devices in blockio(1)
or fileio(0) mode. (default: 1 (blockio))


> > > +/**
> > > + * 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.
After discuss with Danil, we will remove the ibnbd_msg_io_old next round.

Regards,

--
Jack Wang
Linux Kernel Developer
Platform Engineering Compute (IONOS Cloud)