Date:   Tue, 14 Mar 2023 14:56:57 -0500
From:   Eric Blake <eblake@redhat.com>
To:     Nir Soffer <nsoffer@redhat.com>
Cc:     josef@toxicpanda.com, linux-block@vger.kernel.org,
        nbd@other.debian.org, philipp.reisner@linbit.com,
        lars.ellenberg@linbit.com, christoph.boehmwalder@linbit.com,
        corbet@lwn.net, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] block nbd: use req.cookie instead of req.handle
Message-ID: <20230314195657.vpazxest7rvm5frp@redhat.com>
References: <20230310201525.2615385-1-eblake@redhat.com>
 <20230310201525.2615385-4-eblake@redhat.com>
 <CAMRbyyv59L9GiLr5tJvnNdwnBNdNGw+xveG7S63WC9ycOuJYrA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMRbyyv59L9GiLr5tJvnNdwnBNdNGw+xveG7S63WC9ycOuJYrA@mail.gmail.com>
User-Agent: NeoMutt/20220429
Precedence: bulk

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