[PATCH 0/3] nbd: s/handle/cookie/

[PATCH 0/3] nbd: s/handle/cookie/

[Eric Blake]

A 64-bit integer is easier to deal with than char[8], and has no
difference over the wire.  This series stems from a review of a patch
I first submitted to the userspace NBD documentation:

https://lists.debian.org/nbd/2023/03/msg00031.html

Eric Blake (3):
  uapi nbd: improve doc links to userspace spec
  uapi nbd: add cookie alias to handle
  block nbd: use req.cookie instead of req.handle

 Documentation/admin-guide/blockdev/nbd.rst |  2 +-
 drivers/block/nbd.c                        |  4 ++--
 include/uapi/linux/nbd.h                   | 25 +++++++++++++++++-----
 3 files changed, 23 insertions(+), 8 deletions(-)

-- 
2.39.2

[PATCH 1/3] uapi nbd: improve doc links to userspace spec

[Eric Blake]

The uapi <linux/nbd.h> header intentionally documents only the NBD
server features that the kernel module will utilize as a client.  But
while it already had one mention of skipped bits due to userspace
extensions, it did not actually direct the reader to the canonical
source to learn about those extensions.

While touching comments, fix an outdated reference that listed only
READ and WRITE as commands.

The documentation file also had a stale link to sourceforge; nbd
ditched that several years ago in favor of github.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 Documentation/admin-guide/blockdev/nbd.rst |  2 +-
 include/uapi/linux/nbd.h                   | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/blockdev/nbd.rst b/Documentation/admin-guide/blockdev/nbd.rst
index d78dfe559dcf..faf2ac4b1509 100644
--- a/Documentation/admin-guide/blockdev/nbd.rst
+++ b/Documentation/admin-guide/blockdev/nbd.rst
@@ -14,7 +14,7 @@ to borrow disk space from another computer.
 Unlike NFS, it is possible to put any filesystem on it, etc.

 For more information, or to download the nbd-client and nbd-server
-tools, go to http://nbd.sf.net/.
+tools, go to https://github.com/NetworkBlockDevice/nbd.

 The nbd kernel module need only be installed on the client
 system, as the nbd-server is completely in userspace. In fact,
diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
index 20d6cc91435d..8797387caaf7 100644
--- a/include/uapi/linux/nbd.h
+++ b/include/uapi/linux/nbd.h
@@ -11,6 +11,8 @@
  *            Cleanup PARANOIA usage & code.
  * 2004/02/19 Paul Clements
  *            Removed PARANOIA, plus various cleanup and comments
+ * 2023 Copyright Red Hat
+ *            Link to userspace extensions.
  */

 #ifndef _UAPILINUX_NBD_H
@@ -30,12 +32,18 @@
 #define NBD_SET_TIMEOUT _IO( 0xab, 9 )
 #define NBD_SET_FLAGS   _IO( 0xab, 10)

+/*
+ * See also https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md
+ * for additional userspace extensions not yet utilized in the kernel module.
+ */
+
 enum {
 	NBD_CMD_READ = 0,
 	NBD_CMD_WRITE = 1,
 	NBD_CMD_DISC = 2,
 	NBD_CMD_FLUSH = 3,
 	NBD_CMD_TRIM = 4
+	/* userspace defines additional extension commands */
 };

 /* values for flags field, these are server interaction specific. */
@@ -64,14 +72,15 @@ enum {
 #define NBD_REQUEST_MAGIC 0x25609513
 #define NBD_REPLY_MAGIC 0x67446698
 /* Do *not* use magics: 0x12560953 0x96744668. */
+/* magic 0x668e33ef for structured reply not supported by kernel yet */

 /*
  * This is the packet used for communication between client and
  * server. All data are in network byte order.
  */
 struct nbd_request {
-	__be32 magic;
-	__be32 type;	/* == READ || == WRITE 	*/
+	__be32 magic;	/* NBD_REQUEST_MAGIC	*/
+	__be32 type;	/* See NBD_CMD_*	*/
 	char handle[8];
 	__be64 from;
 	__be32 len;
@@ -82,7 +91,7 @@ struct nbd_request {
  * it has completed an I/O request (or an error occurs).
  */
 struct nbd_reply {
-	__be32 magic;
+	__be32 magic;		/* NBD_REPLY_MAGIC	*/
 	__be32 error;		/* 0 = ok, else error	*/
 	char handle[8];		/* handle you got from request	*/
 };
-- 
2.39.2

[PATCH 2/3] uapi nbd: add cookie alias to handle

[Eric Blake]

The uapi <linux/nbd.h> header declares a 'char handle[8]' per request;
which is overloaded in English (are you referring to "handle" the
verb, such as handling a signal or writing a callback handler, or
"handle" the noun, the value used in a lookup table to correlate a
response back to the request).  Many client-side NBD implementations
(both servers and clients) have instead used 'u64 cookie' or similar,
as it is easier to directly assign an integer than to futz around with
memcpy.  In fact, upstream documentation is now encouraging this shift
in terminology: https://lists.debian.org/nbd/2023/03/msg00031.html

Accomplish this by use of an anonymous union to provide the alias for
anyone getting the definition from the uapi; this does not break
existing clients, while exposing the nicer name for those who prefer
it.  Note that block/nbd.c still uses the term handle (in fact, it
actually combines a 32-bit cookie and a 32-bit tag into the 64-bit
handle), but that internal usage is not changed the public uapi, since
no compliant NBD server has any reason to inspect or alter the 64
bits sent over the socket.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/uapi/linux/nbd.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
index 8797387caaf7..f58f2043f62e 100644
--- a/include/uapi/linux/nbd.h
+++ b/include/uapi/linux/nbd.h
@@ -81,7 +81,10 @@ enum {
 struct nbd_request {
 	__be32 magic;	/* NBD_REQUEST_MAGIC	*/
 	__be32 type;	/* See NBD_CMD_*	*/
-	char handle[8];
+	union {
+		char handle[8];
+		__be64 cookie;
+	};
 	__be64 from;
 	__be32 len;
 } __attribute__((packed));
@@ -93,6 +96,9 @@ struct nbd_request {
 struct nbd_reply {
 	__be32 magic;		/* NBD_REPLY_MAGIC	*/
 	__be32 error;		/* 0 = ok, else error	*/
-	char handle[8];		/* handle you got from request	*/
+	union {
+		char handle[8];	/* handle you got from request	*/
+		__be64 cookie;
+	};
 };
 #endif /* _UAPILINUX_NBD_H */
-- 
2.39.2

[PATCH 3/3] block nbd: use req.cookie instead of req.handle

[Eric Blake]

A good compiler should not compile this any differently, but it seems
nicer to avoid memcpy() when integer assignment will work.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 drivers/block/nbd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 592cfa8b765a..672fb8d1ce67 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -606,7 +606,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 		request.len = htonl(size);
 	}
 	handle = nbd_cmd_handle(cmd);
-	memcpy(request.handle, &handle, sizeof(handle));
+	request.cookie = handle;

 	trace_nbd_send_request(&request, nbd->index, blk_mq_rq_from_pdu(cmd));

@@ -732,7 +732,7 @@ static struct nbd_cmd *nbd_handle_reply(struct nbd_device *nbd, int index,
 	u32 tag;
 	int ret = 0;

-	memcpy(&handle, reply->handle, sizeof(handle));
+	handle = reply->cookie;
 	tag = nbd_handle_to_tag(handle);
 	hwq = blk_mq_unique_tag_to_hwq(tag);
 	if (hwq < nbd->tag_set.nr_hw_queues)
-- 
2.39.2

Re: [PATCH 3/3] block nbd: use req.cookie instead of req.handle

[Nir Soffer]

On Fri, Mar 10, 2023 at 10:16 PM Eric Blake <eblake@redhat.com> wrote:
>
> A good compiler should not compile this any differently, but it seems
> nicer to avoid memcpy() when integer assignment will work.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  drivers/block/nbd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 592cfa8b765a..672fb8d1ce67 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -606,7 +606,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
>                 request.len = htonl(size);
>         }
>         handle = nbd_cmd_handle(cmd);

This returns native u64 (likely little endian) but the new interface
specifies __be64. Should we swap the bytes if needed?

This will help tools like the wireshark plugin to display the right value
when checking traces from machines with different endianness. Or help
the nbd server to show the same *cooike* value in the logs. The value
is opaque but reasonable code can assume that __be64 can be safely
parsed as an integer.

> -       memcpy(request.handle, &handle, sizeof(handle));
> +       request.cookie = handle;
>
>         trace_nbd_send_request(&request, nbd->index, blk_mq_rq_from_pdu(cmd));
>
> @@ -732,7 +732,7 @@ static struct nbd_cmd *nbd_handle_reply(struct nbd_device *nbd, int index,
>         u32 tag;
>         int ret = 0;
>
> -       memcpy(&handle, reply->handle, sizeof(handle));
> +       handle = reply->cookie;
>         tag = nbd_handle_to_tag(handle);
>         hwq = blk_mq_unique_tag_to_hwq(tag);
>         if (hwq < nbd->tag_set.nr_hw_queues)
> --
> 2.39.2
>

Also the same file has references to *handle* like:

static u64 nbd_cmd_handle(struct nbd_cmd *cmd)
{
    struct request *req = blk_mq_rq_from_pdu(cmd);
    u32 tag = blk_mq_unique_tag(req);
    u64 cookie = cmd->cmd_cookie;

    return (cookie << NBD_COOKIE_BITS) | tag;
}

static u32 nbd_handle_to_tag(u64 handle)
{
    return (u32)handle;
}

static u32 nbd_handle_to_cookie(u64 handle)
{
    return (u32)(handle >> NBD_COOKIE_BITS);
}

So this change is a little bit confusing.

I think we need to use a term like *nbd_cookie* instead of
*handle* to make this more clear.

Nir

Re: [PATCH 2/3] uapi nbd: add cookie alias to handle

[Nir Soffer]

On Fri, Mar 10, 2023 at 10:16 PM Eric Blake <eblake@redhat.com> wrote:
>
> The uapi <linux/nbd.h> header declares a 'char handle[8]' per request;
> which is overloaded in English (are you referring to "handle" the
> verb, such as handling a signal or writing a callback handler, or
> "handle" the noun, the value used in a lookup table to correlate a
> response back to the request).  Many client-side NBD implementations
> (both servers and clients) have instead used 'u64 cookie' or similar,
> as it is easier to directly assign an integer than to futz around with
> memcpy.  In fact, upstream documentation is now encouraging this shift
> in terminology: https://lists.debian.org/nbd/2023/03/msg00031.html
>
> Accomplish this by use of an anonymous union to provide the alias for
> anyone getting the definition from the uapi; this does not break
> existing clients, while exposing the nicer name for those who prefer
> it.  Note that block/nbd.c still uses the term handle (in fact, it
> actually combines a 32-bit cookie and a 32-bit tag into the 64-bit
> handle), but that internal usage is not changed the public uapi, since
> no compliant NBD server has any reason to inspect or alter the 64
> bits sent over the socket.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/uapi/linux/nbd.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
> index 8797387caaf7..f58f2043f62e 100644
> --- a/include/uapi/linux/nbd.h
> +++ b/include/uapi/linux/nbd.h
> @@ -81,7 +81,10 @@ enum {
>  struct nbd_request {
>         __be32 magic;   /* NBD_REQUEST_MAGIC    */
>         __be32 type;    /* See NBD_CMD_*        */
> -       char handle[8];
> +       union {
> +               char handle[8];
> +               __be64 cookie;
> +       };
>         __be64 from;
>         __be32 len;
>  } __attribute__((packed));
> @@ -93,6 +96,9 @@ struct nbd_request {
>  struct nbd_reply {
>         __be32 magic;           /* NBD_REPLY_MAGIC      */
>         __be32 error;           /* 0 = ok, else error   */
> -       char handle[8];         /* handle you got from request  */
> +       union {
> +               char handle[8]; /* handle you got from request  */
> +               __be64 cookie;

Should we document like this?

    union {
        __be64 cookie; /* cookie you got from request */
        char handle[8]; /* older name */

I think we want future code to use the new term.

> +       };
>  };
>  #endif /* _UAPILINUX_NBD_H */
> --
> 2.39.2
>

Nir

Re: [PATCH 3/3] block nbd: use req.cookie instead of req.handle

[kernel test robot]

Hi Eric,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on linus/master v6.3-rc1 next-20230310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Eric-Blake/uapi-nbd-improve-doc-links-to-userspace-spec/20230311-041759
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link:    https://lore.kernel.org/r/20230310201525.2615385-4-eblake%40redhat.com
patch subject: [PATCH 3/3] block nbd: use req.cookie instead of req.handle
config: loongarch-randconfig-s052-20230310 (https://download.01.org/0day-ci/archive/20230312/202303121323.Jd35Q1Au-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/ee3462cd07240f936d4a304b8b7da8c1d610e2af
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Eric-Blake/uapi-nbd-improve-doc-links-to-userspace-spec/20230311-041759
        git checkout ee3462cd07240f936d4a304b8b7da8c1d610e2af
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch SHELL=/bin/bash drivers/block/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303121323.Jd35Q1Au-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/block/nbd.c:609:24: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __be64 [addressable] [assigned] [usertype] cookie @@     got unsigned long long [assigned] [usertype] handle @@
   drivers/block/nbd.c:609:24: sparse:     expected restricted __be64 [addressable] [assigned] [usertype] cookie
   drivers/block/nbd.c:609:24: sparse:     got unsigned long long [assigned] [usertype] handle
   drivers/block/nbd.c:631:32: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted blk_status_t [usertype] @@
   drivers/block/nbd.c:631:32: sparse:     expected int
   drivers/block/nbd.c:631:32: sparse:     got restricted blk_status_t [usertype]
   drivers/block/nbd.c:672:48: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted blk_status_t [usertype] @@
   drivers/block/nbd.c:672:48: sparse:     expected int
   drivers/block/nbd.c:672:48: sparse:     got restricted blk_status_t [usertype]
>> drivers/block/nbd.c:735:16: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned long long [usertype] handle @@     got restricted __be64 [usertype] cookie @@
   drivers/block/nbd.c:735:16: sparse:     expected unsigned long long [usertype] handle
   drivers/block/nbd.c:735:16: sparse:     got restricted __be64 [usertype] cookie
   drivers/block/nbd.c:1077:21: sparse: sparse: incorrect type in assignment (different base types) @@     expected int [assigned] ret @@     got restricted blk_status_t [usertype] @@
   drivers/block/nbd.c:1077:21: sparse:     expected int [assigned] ret
   drivers/block/nbd.c:1077:21: sparse:     got restricted blk_status_t [usertype]
   drivers/block/nbd.c:1082:16: sparse: sparse: incorrect type in return expression (different base types) @@     expected restricted blk_status_t @@     got int [assigned] ret @@
   drivers/block/nbd.c:1082:16: sparse:     expected restricted blk_status_t
   drivers/block/nbd.c:1082:16: sparse:     got int [assigned] ret

vim +609 drivers/block/nbd.c

   549	
   550	/* always call with the tx_lock held */
   551	static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
   552	{
   553		struct request *req = blk_mq_rq_from_pdu(cmd);
   554		struct nbd_config *config = nbd->config;
   555		struct nbd_sock *nsock = config->socks[index];
   556		int result;
   557		struct nbd_request request = {.magic = htonl(NBD_REQUEST_MAGIC)};
   558		struct kvec iov = {.iov_base = &request, .iov_len = sizeof(request)};
   559		struct iov_iter from;
   560		unsigned long size = blk_rq_bytes(req);
   561		struct bio *bio;
   562		u64 handle;
   563		u32 type;
   564		u32 nbd_cmd_flags = 0;
   565		int sent = nsock->sent, skip = 0;
   566	
   567		iov_iter_kvec(&from, ITER_SOURCE, &iov, 1, sizeof(request));
   568	
   569		type = req_to_nbd_cmd_type(req);
   570		if (type == U32_MAX)
   571			return -EIO;
   572	
   573		if (rq_data_dir(req) == WRITE &&
   574		    (config->flags & NBD_FLAG_READ_ONLY)) {
   575			dev_err_ratelimited(disk_to_dev(nbd->disk),
   576					    "Write on read-only\n");
   577			return -EIO;
   578		}
   579	
   580		if (req->cmd_flags & REQ_FUA)
   581			nbd_cmd_flags |= NBD_CMD_FLAG_FUA;
   582	
   583		/* We did a partial send previously, and we at least sent the whole
   584		 * request struct, so just go and send the rest of the pages in the
   585		 * request.
   586		 */
   587		if (sent) {
   588			if (sent >= sizeof(request)) {
   589				skip = sent - sizeof(request);
   590	
   591				/* initialize handle for tracing purposes */
   592				handle = nbd_cmd_handle(cmd);
   593	
   594				goto send_pages;
   595			}
   596			iov_iter_advance(&from, sent);
   597		} else {
   598			cmd->cmd_cookie++;
   599		}
   600		cmd->index = index;
   601		cmd->cookie = nsock->cookie;
   602		cmd->retries = 0;
   603		request.type = htonl(type | nbd_cmd_flags);
   604		if (type != NBD_CMD_FLUSH) {
   605			request.from = cpu_to_be64((u64)blk_rq_pos(req) << 9);
   606			request.len = htonl(size);
   607		}
   608		handle = nbd_cmd_handle(cmd);
 > 609		request.cookie = handle;
   610	
   611		trace_nbd_send_request(&request, nbd->index, blk_mq_rq_from_pdu(cmd));
   612	
   613		dev_dbg(nbd_to_dev(nbd), "request %p: sending control (%s@%llu,%uB)\n",
   614			req, nbdcmd_to_ascii(type),
   615			(unsigned long long)blk_rq_pos(req) << 9, blk_rq_bytes(req));
   616		result = sock_xmit(nbd, index, 1, &from,
   617				(type == NBD_CMD_WRITE) ? MSG_MORE : 0, &sent);
   618		trace_nbd_header_sent(req, handle);
   619		if (result < 0) {
   620			if (was_interrupted(result)) {
   621				/* If we havne't sent anything we can just return BUSY,
   622				 * however if we have sent something we need to make
   623				 * sure we only allow this req to be sent until we are
   624				 * completely done.
   625				 */
   626				if (sent) {
   627					nsock->pending = req;
   628					nsock->sent = sent;
   629				}
   630				set_bit(NBD_CMD_REQUEUED, &cmd->flags);
   631				return BLK_STS_RESOURCE;
   632			}
   633			dev_err_ratelimited(disk_to_dev(nbd->disk),
   634				"Send control failed (result %d)\n", result);
   635			return -EAGAIN;
   636		}
   637	send_pages:
   638		if (type != NBD_CMD_WRITE)
   639			goto out;
   640	
   641		bio = req->bio;
   642		while (bio) {
   643			struct bio *next = bio->bi_next;
   644			struct bvec_iter iter;
   645			struct bio_vec bvec;
   646	
   647			bio_for_each_segment(bvec, bio, iter) {
   648				bool is_last = !next && bio_iter_last(bvec, iter);
   649				int flags = is_last ? 0 : MSG_MORE;
   650	
   651				dev_dbg(nbd_to_dev(nbd), "request %p: sending %d bytes data\n",
   652					req, bvec.bv_len);
   653				iov_iter_bvec(&from, ITER_SOURCE, &bvec, 1, bvec.bv_len);
   654				if (skip) {
   655					if (skip >= iov_iter_count(&from)) {
   656						skip -= iov_iter_count(&from);
   657						continue;
   658					}
   659					iov_iter_advance(&from, skip);
   660					skip = 0;
   661				}
   662				result = sock_xmit(nbd, index, 1, &from, flags, &sent);
   663				if (result < 0) {
   664					if (was_interrupted(result)) {
   665						/* We've already sent the header, we
   666						 * have no choice but to set pending and
   667						 * return BUSY.
   668						 */
   669						nsock->pending = req;
   670						nsock->sent = sent;
   671						set_bit(NBD_CMD_REQUEUED, &cmd->flags);
   672						return BLK_STS_RESOURCE;
   673					}
   674					dev_err(disk_to_dev(nbd->disk),
   675						"Send data failed (result %d)\n",
   676						result);
   677					return -EAGAIN;
   678				}
   679				/*
   680				 * The completion might already have come in,
   681				 * so break for the last one instead of letting
   682				 * the iterator do it. This prevents use-after-free
   683				 * of the bio.
   684				 */
   685				if (is_last)
   686					break;
   687			}
   688			bio = next;
   689		}
   690	out:
   691		trace_nbd_payload_sent(req, handle);
   692		nsock->pending = NULL;
   693		nsock->sent = 0;
   694		return 0;
   695	}
   696	
   697	static int nbd_read_reply(struct nbd_device *nbd, int index,
   698				  struct nbd_reply *reply)
   699	{
   700		struct kvec iov = {.iov_base = reply, .iov_len = sizeof(*reply)};
   701		struct iov_iter to;
   702		int result;
   703	
   704		reply->magic = 0;
   705		iov_iter_kvec(&to, ITER_DEST, &iov, 1, sizeof(*reply));
   706		result = sock_xmit(nbd, index, 0, &to, MSG_WAITALL, NULL);
   707		if (result < 0) {
   708			if (!nbd_disconnected(nbd->config))
   709				dev_err(disk_to_dev(nbd->disk),
   710					"Receive control failed (result %d)\n", result);
   711			return result;
   712		}
   713	
   714		if (ntohl(reply->magic) != NBD_REPLY_MAGIC) {
   715			dev_err(disk_to_dev(nbd->disk), "Wrong magic (0x%lx)\n",
   716					(unsigned long)ntohl(reply->magic));
   717			return -EPROTO;
   718		}
   719	
   720		return 0;
   721	}
   722	
   723	/* NULL returned = something went wrong, inform userspace */
   724	static struct nbd_cmd *nbd_handle_reply(struct nbd_device *nbd, int index,
   725						struct nbd_reply *reply)
   726	{
   727		int result;
   728		struct nbd_cmd *cmd;
   729		struct request *req = NULL;
   730		u64 handle;
   731		u16 hwq;
   732		u32 tag;
   733		int ret = 0;
   734	
 > 735		handle = reply->cookie;
   736		tag = nbd_handle_to_tag(handle);
   737		hwq = blk_mq_unique_tag_to_hwq(tag);
   738		if (hwq < nbd->tag_set.nr_hw_queues)
   739			req = blk_mq_tag_to_rq(nbd->tag_set.tags[hwq],
   740					       blk_mq_unique_tag_to_tag(tag));
   741		if (!req || !blk_mq_request_started(req)) {
   742			dev_err(disk_to_dev(nbd->disk), "Unexpected reply (%d) %p\n",
   743				tag, req);
   744			return ERR_PTR(-ENOENT);
   745		}
   746		trace_nbd_header_received(req, handle);
   747		cmd = blk_mq_rq_to_pdu(req);
   748	
   749		mutex_lock(&cmd->lock);
   750		if (!test_bit(NBD_CMD_INFLIGHT, &cmd->flags)) {
   751			dev_err(disk_to_dev(nbd->disk), "Suspicious reply %d (status %u flags %lu)",
   752				tag, cmd->status, cmd->flags);
   753			ret = -ENOENT;
   754			goto out;
   755		}
   756		if (cmd->index != index) {
   757			dev_err(disk_to_dev(nbd->disk), "Unexpected reply %d from different sock %d (expected %d)",
   758				tag, index, cmd->index);
   759			ret = -ENOENT;
   760			goto out;
   761		}
   762		if (cmd->cmd_cookie != nbd_handle_to_cookie(handle)) {
   763			dev_err(disk_to_dev(nbd->disk), "Double reply on req %p, cmd_cookie %u, handle cookie %u\n",
   764				req, cmd->cmd_cookie, nbd_handle_to_cookie(handle));
   765			ret = -ENOENT;
   766			goto out;
   767		}
   768		if (cmd->status != BLK_STS_OK) {
   769			dev_err(disk_to_dev(nbd->disk), "Command already handled %p\n",
   770				req);
   771			ret = -ENOENT;
   772			goto out;
   773		}
   774		if (test_bit(NBD_CMD_REQUEUED, &cmd->flags)) {
   775			dev_err(disk_to_dev(nbd->disk), "Raced with timeout on req %p\n",
   776				req);
   777			ret = -ENOENT;
   778			goto out;
   779		}
   780		if (ntohl(reply->error)) {
   781			dev_err(disk_to_dev(nbd->disk), "Other side returned error (%d)\n",
   782				ntohl(reply->error));
   783			cmd->status = BLK_STS_IOERR;
   784			goto out;
   785		}
   786	
   787		dev_dbg(nbd_to_dev(nbd), "request %p: got reply\n", req);
   788		if (rq_data_dir(req) != WRITE) {
   789			struct req_iterator iter;
   790			struct bio_vec bvec;
   791			struct iov_iter to;
   792	
   793			rq_for_each_segment(bvec, req, iter) {
   794				iov_iter_bvec(&to, ITER_DEST, &bvec, 1, bvec.bv_len);
   795				result = sock_xmit(nbd, index, 0, &to, MSG_WAITALL, NULL);
   796				if (result < 0) {
   797					dev_err(disk_to_dev(nbd->disk), "Receive data failed (result %d)\n",
   798						result);
   799					/*
   800					 * If we've disconnected, we need to make sure we
   801					 * complete this request, otherwise error out
   802					 * and let the timeout stuff handle resubmitting
   803					 * this request onto another connection.
   804					 */
   805					if (nbd_disconnected(nbd->config)) {
   806						cmd->status = BLK_STS_IOERR;
   807						goto out;
   808					}
   809					ret = -EIO;
   810					goto out;
   811				}
   812				dev_dbg(nbd_to_dev(nbd), "request %p: got %d bytes data\n",
   813					req, bvec.bv_len);
   814			}
   815		}
   816	out:
   817		trace_nbd_payload_received(req, handle);
   818		mutex_unlock(&cmd->lock);
   819		return ret ? ERR_PTR(ret) : cmd;
   820	}
   821	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 1/3] uapi nbd: improve doc links to userspace spec

[Ming Lei]

On Sat, Mar 11, 2023 at 4:17 AM Eric Blake <eblake@redhat.com> wrote:
>
> The uapi <linux/nbd.h> header intentionally documents only the NBD
> server features that the kernel module will utilize as a client.  But
> while it already had one mention of skipped bits due to userspace
> extensions, it did not actually direct the reader to the canonical
> source to learn about those extensions.
>
> While touching comments, fix an outdated reference that listed only
> READ and WRITE as commands.
>
> The documentation file also had a stale link to sourceforge; nbd
> ditched that several years ago in favor of github.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Re: [PATCH 2/3] uapi nbd: add cookie alias to handle

[Eric Blake]

On Sat, Mar 11, 2023 at 02:30:39PM +0200, Nir Soffer wrote:
> On Fri, Mar 10, 2023 at 10:16 PM Eric Blake <eblake@redhat.com> wrote:
> >
> > The uapi <linux/nbd.h> header declares a 'char handle[8]' per request;
> > which is overloaded in English (are you referring to "handle" the
> > verb, such as handling a signal or writing a callback handler, or
> > "handle" the noun, the value used in a lookup table to correlate a
> > response back to the request).  Many client-side NBD implementations
> > (both servers and clients) have instead used 'u64 cookie' or similar,
> > as it is easier to directly assign an integer than to futz around with
> > memcpy.  In fact, upstream documentation is now encouraging this shift
> > in terminology: https://lists.debian.org/nbd/2023/03/msg00031.html
> >
> > Accomplish this by use of an anonymous union to provide the alias for
> > anyone getting the definition from the uapi; this does not break
> > existing clients, while exposing the nicer name for those who prefer
> > it.  Note that block/nbd.c still uses the term handle (in fact, it
> > actually combines a 32-bit cookie and a 32-bit tag into the 64-bit
> > handle), but that internal usage is not changed the public uapi, since
> > no compliant NBD server has any reason to inspect or alter the 64
> > bits sent over the socket.
> >
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  include/uapi/linux/nbd.h | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
> > index 8797387caaf7..f58f2043f62e 100644
> > --- a/include/uapi/linux/nbd.h
> > +++ b/include/uapi/linux/nbd.h
> > @@ -81,7 +81,10 @@ enum {
> >  struct nbd_request {
> >         __be32 magic;   /* NBD_REQUEST_MAGIC    */
> >         __be32 type;    /* See NBD_CMD_*        */
> > -       char handle[8];
> > +       union {
> > +               char handle[8];
> > +               __be64 cookie;
> > +       };
> >         __be64 from;
> >         __be32 len;
> >  } __attribute__((packed));
> > @@ -93,6 +96,9 @@ struct nbd_request {
> >  struct nbd_reply {
> >         __be32 magic;           /* NBD_REPLY_MAGIC      */
> >         __be32 error;           /* 0 = ok, else error   */
> > -       char handle[8];         /* handle you got from request  */
> > +       union {
> > +               char handle[8]; /* handle you got from request  */
> > +               __be64 cookie;
> 
> Should we document like this?
> 
>     union {
>         __be64 cookie; /* cookie you got from request */
>         char handle[8]; /* older name */
> 
> I think we want future code to use the new term.

Sure, swapping the order to favor the preferred name first makes sense.

I'm still not sure on whether cookie should be u64 or __be64 (it's
opaque, so endianness over the wire doesn't matter; and previous code
was using memcpy() onto char[8] which may behave differently depending
on machine endianness).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/3] block nbd: use req.cookie instead of req.handle

[Eric Blake]

On Sat, Mar 11, 2023 at 02:22:51PM +0200, Nir Soffer wrote:
> On Fri, Mar 10, 2023 at 10:16 PM Eric Blake <eblake@redhat.com> wrote:
> >
> > A good compiler should not compile this any differently, but it seems
> > nicer to avoid memcpy() when integer assignment will work.
> >
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  drivers/block/nbd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 592cfa8b765a..672fb8d1ce67 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -606,7 +606,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
> >                 request.len = htonl(size);
> >         }
> >         handle = nbd_cmd_handle(cmd);
> 
> This returns native u64 (likely little endian) but the new interface
> specifies __be64. Should we swap the bytes if needed?

Or document the field as u64 in the .h file.  I'm not sure which is
better, but the mismatch here is definitely why the test robot
complained about new warnings with my v1 patch.  I'm new enough to
kernel development that I will welcome a hint about which way I should
lean in writing v2.

> 
> This will help tools like the wireshark plugin to display the right value
> when checking traces from machines with different endianness. Or help
> the nbd server to show the same *cooike* value in the logs. The value
> is opaque but reasonable code can assume that __be64 can be safely
> parsed as an integer.

The fact that the old code was memcpy()ing a u64 into char[8] over the
wire means that wireshark was already seeing endian-dependant values
in that portion of the struct (a big-endian and little-endian client
that happen to use the same cookie/handle will show up differently).
I don't mind adding byteswapping and using __be64 (instead of direct
assignment and u64) if that's what we want, but I don't think anyone
should be relying on wireshark to have stable output for those bytes,
since they are opaque to the server regardless of endianness.

> 
> Also the same file has references to *handle* like:
> 
> static u64 nbd_cmd_handle(struct nbd_cmd *cmd)
> {
>     struct request *req = blk_mq_rq_from_pdu(cmd);
>     u32 tag = blk_mq_unique_tag(req);
>     u64 cookie = cmd->cmd_cookie;
> 
>     return (cookie << NBD_COOKIE_BITS) | tag;
> }
> 
> static u32 nbd_handle_to_tag(u64 handle)
> {
>     return (u32)handle;
> }
> 
> static u32 nbd_handle_to_cookie(u64 handle)
> {
>     return (u32)(handle >> NBD_COOKIE_BITS);
> }
> 
> So this change is a little bit confusing.
> 
> I think we need to use a term like *nbd_cookie* instead of
> *handle* to make this more clear.

I can additionally rename these helper functions in v2 if that would
be helpful.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/3] uapi nbd: add cookie alias to handle

[Ming Lei]

On Tue, Mar 14, 2023 at 02:50:23PM -0500, Eric Blake wrote:
> On Sat, Mar 11, 2023 at 02:30:39PM +0200, Nir Soffer wrote:
> > On Fri, Mar 10, 2023 at 10:16 PM Eric Blake <eblake@redhat.com> wrote:
> > >
> > > The uapi <linux/nbd.h> header declares a 'char handle[8]' per request;
> > > which is overloaded in English (are you referring to "handle" the
> > > verb, such as handling a signal or writing a callback handler, or
> > > "handle" the noun, the value used in a lookup table to correlate a
> > > response back to the request).  Many client-side NBD implementations
> > > (both servers and clients) have instead used 'u64 cookie' or similar,
> > > as it is easier to directly assign an integer than to futz around with
> > > memcpy.  In fact, upstream documentation is now encouraging this shift
> > > in terminology: https://lists.debian.org/nbd/2023/03/msg00031.html
> > >
> > > Accomplish this by use of an anonymous union to provide the alias for
> > > anyone getting the definition from the uapi; this does not break
> > > existing clients, while exposing the nicer name for those who prefer
> > > it.  Note that block/nbd.c still uses the term handle (in fact, it
> > > actually combines a 32-bit cookie and a 32-bit tag into the 64-bit
> > > handle), but that internal usage is not changed the public uapi, since
> > > no compliant NBD server has any reason to inspect or alter the 64
> > > bits sent over the socket.
> > >
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > ---
> > >  include/uapi/linux/nbd.h | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
> > > index 8797387caaf7..f58f2043f62e 100644
> > > --- a/include/uapi/linux/nbd.h
> > > +++ b/include/uapi/linux/nbd.h
> > > @@ -81,7 +81,10 @@ enum {
> > >  struct nbd_request {
> > >         __be32 magic;   /* NBD_REQUEST_MAGIC    */
> > >         __be32 type;    /* See NBD_CMD_*        */
> > > -       char handle[8];
> > > +       union {
> > > +               char handle[8];
> > > +               __be64 cookie;
> > > +       };
> > >         __be64 from;
> > >         __be32 len;
> > >  } __attribute__((packed));
> > > @@ -93,6 +96,9 @@ struct nbd_request {
> > >  struct nbd_reply {
> > >         __be32 magic;           /* NBD_REPLY_MAGIC      */
> > >         __be32 error;           /* 0 = ok, else error   */
> > > -       char handle[8];         /* handle you got from request  */
> > > +       union {
> > > +               char handle[8]; /* handle you got from request  */
> > > +               __be64 cookie;
> > 
> > Should we document like this?
> > 
> >     union {
> >         __be64 cookie; /* cookie you got from request */
> >         char handle[8]; /* older name */
> > 
> > I think we want future code to use the new term.
> 
> Sure, swapping the order to favor the preferred name first makes sense.
> 
> I'm still not sure on whether cookie should be u64 or __be64 (it's
> opaque, so endianness over the wire doesn't matter;

I guess it is 'u64', given ->handle is always copied to nbd_reply from
nbd_request in nbd server side, so native endian is always applied for
building and parsing ->handle in nbd client side.

But it looks odd to mark it as u64.

> and previous code
> was using memcpy() onto char[8] which may behave differently depending
> on machine endianness).




Thanks,
Ming