* [PATCH 0/3] nbd: s/handle/cookie/ @ 2023-03-10 20:15 Eric Blake 2023-03-10 20:15 ` [PATCH 1/3] uapi nbd: improve doc links to userspace spec Eric Blake ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Eric Blake @ 2023-03-10 20:15 UTC (permalink / raw) To: josef, linux-block, nbd Cc: philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] uapi nbd: improve doc links to userspace spec 2023-03-10 20:15 [PATCH 0/3] nbd: s/handle/cookie/ Eric Blake @ 2023-03-10 20:15 ` Eric Blake 2023-03-13 3:04 ` Ming Lei 2023-03-10 20:15 ` [PATCH 2/3] uapi nbd: add cookie alias to handle Eric Blake 2023-03-10 20:15 ` [PATCH 3/3] block nbd: use req.cookie instead of req.handle Eric Blake 2 siblings, 1 reply; 11+ messages in thread From: Eric Blake @ 2023-03-10 20:15 UTC (permalink / raw) To: josef, linux-block, nbd Cc: philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] uapi nbd: improve doc links to userspace spec 2023-03-10 20:15 ` [PATCH 1/3] uapi nbd: improve doc links to userspace spec Eric Blake @ 2023-03-13 3:04 ` Ming Lei 0 siblings, 0 replies; 11+ messages in thread From: Ming Lei @ 2023-03-13 3:04 UTC (permalink / raw) To: Eric Blake Cc: josef, linux-block, nbd, philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] uapi nbd: add cookie alias to handle 2023-03-10 20:15 [PATCH 0/3] nbd: s/handle/cookie/ Eric Blake 2023-03-10 20:15 ` [PATCH 1/3] uapi nbd: improve doc links to userspace spec Eric Blake @ 2023-03-10 20:15 ` Eric Blake 2023-03-11 12:30 ` Nir Soffer 2023-03-10 20:15 ` [PATCH 3/3] block nbd: use req.cookie instead of req.handle Eric Blake 2 siblings, 1 reply; 11+ messages in thread From: Eric Blake @ 2023-03-10 20:15 UTC (permalink / raw) To: josef, linux-block, nbd Cc: philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] uapi nbd: add cookie alias to handle 2023-03-10 20:15 ` [PATCH 2/3] uapi nbd: add cookie alias to handle Eric Blake @ 2023-03-11 12:30 ` Nir Soffer 2023-03-14 19:50 ` Eric Blake 0 siblings, 1 reply; 11+ messages in thread From: Nir Soffer @ 2023-03-11 12:30 UTC (permalink / raw) To: Eric Blake Cc: josef, linux-block, nbd, philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] uapi nbd: add cookie alias to handle 2023-03-11 12:30 ` Nir Soffer @ 2023-03-14 19:50 ` Eric Blake 2023-03-15 3:33 ` Ming Lei 0 siblings, 1 reply; 11+ messages in thread From: Eric Blake @ 2023-03-14 19:50 UTC (permalink / raw) To: Nir Soffer Cc: josef, linux-block, nbd, philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] uapi nbd: add cookie alias to handle 2023-03-14 19:50 ` Eric Blake @ 2023-03-15 3:33 ` Ming Lei 0 siblings, 0 replies; 11+ messages in thread From: Ming Lei @ 2023-03-15 3:33 UTC (permalink / raw) To: Eric Blake Cc: Nir Soffer, josef, linux-block, nbd, philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] block nbd: use req.cookie instead of req.handle 2023-03-10 20:15 [PATCH 0/3] nbd: s/handle/cookie/ Eric Blake 2023-03-10 20:15 ` [PATCH 1/3] uapi nbd: improve doc links to userspace spec Eric Blake 2023-03-10 20:15 ` [PATCH 2/3] uapi nbd: add cookie alias to handle Eric Blake @ 2023-03-10 20:15 ` Eric Blake 2023-03-11 12:22 ` Nir Soffer 2023-03-12 5:12 ` kernel test robot 2 siblings, 2 replies; 11+ messages in thread From: Eric Blake @ 2023-03-10 20:15 UTC (permalink / raw) To: josef, linux-block, nbd Cc: philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] block nbd: use req.cookie instead of req.handle 2023-03-10 20:15 ` [PATCH 3/3] block nbd: use req.cookie instead of req.handle Eric Blake @ 2023-03-11 12:22 ` Nir Soffer 2023-03-14 19:56 ` Eric Blake 2023-03-12 5:12 ` kernel test robot 1 sibling, 1 reply; 11+ messages in thread From: Nir Soffer @ 2023-03-11 12:22 UTC (permalink / raw) To: Eric Blake Cc: josef, linux-block, nbd, philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] block nbd: use req.cookie instead of req.handle 2023-03-11 12:22 ` Nir Soffer @ 2023-03-14 19:56 ` Eric Blake 0 siblings, 0 replies; 11+ messages in thread From: Eric Blake @ 2023-03-14 19:56 UTC (permalink / raw) To: Nir Soffer Cc: josef, linux-block, nbd, philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] block nbd: use req.cookie instead of req.handle 2023-03-10 20:15 ` [PATCH 3/3] block nbd: use req.cookie instead of req.handle Eric Blake 2023-03-11 12:22 ` Nir Soffer @ 2023-03-12 5:12 ` kernel test robot 1 sibling, 0 replies; 11+ messages in thread From: kernel test robot @ 2023-03-12 5:12 UTC (permalink / raw) To: Eric Blake, josef, linux-block, nbd Cc: oe-kbuild-all, philipp.reisner, lars.ellenberg, christoph.boehmwalder, corbet, linux-doc, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-03-15 3:34 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-10 20:15 [PATCH 0/3] nbd: s/handle/cookie/ Eric Blake 2023-03-10 20:15 ` [PATCH 1/3] uapi nbd: improve doc links to userspace spec Eric Blake 2023-03-13 3:04 ` Ming Lei 2023-03-10 20:15 ` [PATCH 2/3] uapi nbd: add cookie alias to handle Eric Blake 2023-03-11 12:30 ` Nir Soffer 2023-03-14 19:50 ` Eric Blake 2023-03-15 3:33 ` Ming Lei 2023-03-10 20:15 ` [PATCH 3/3] block nbd: use req.cookie instead of req.handle Eric Blake 2023-03-11 12:22 ` Nir Soffer 2023-03-14 19:56 ` Eric Blake 2023-03-12 5:12 ` kernel test robot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).