QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: Kevin Wolf <kwolf@redhat.com>,
	"nbd@other.debian.org" <nbd@other.debian.org>,
	"libguestfs@redhat.com" <libguestfs@redhat.com>,
	"open list:Network Block Dev..." <qemu-block@nongnu.org>,
	Max Reitz <mreitz@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 1/5] nbd: Improve per-export flag handling in server
Date: Fri, 30 Aug 2019 18:32:10 -0500
Message-ID: <55ebe9db-5a28-d844-89f7-2dc9a7d977d7@redhat.com> (raw)
In-Reply-To: <b1483a24-d524-169a-3440-5a9bf4440265@redhat.com>

[-- Attachment #1.1: Type: text/plain, Size: 3732 bytes --]

On 8/30/19 6:10 PM, Eric Blake wrote:
> On 8/30/19 1:00 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 23.08.2019 17:37, Eric Blake wrote:
>>> When creating a read-only image, we are still advertising support for
>>> TRIM and WRITE_ZEROES to the client, even though the client should not
>>> be issuing those commands.  But seeing this requires looking across
>>> multiple functions:
>>>
> 
>>> @@ -458,10 +458,13 @@ static int nbd_negotiate_handle_export_name(NBDClient *client,
>>>           return -EINVAL;
>>>       }
>>>
>>> -    trace_nbd_negotiate_new_style_size_flags(client->exp->size,
>>> -                                             client->exp->nbdflags | myflags);
>>> +    myflags = client->exp->nbdflags;
>>> +    if (client->structured_reply) {
>>> +        myflags |= NBD_FLAG_SEND_DF;
>>> +    }
>>
>>
>> why we cant do just
>> client->exp->nbdflags |= NBD_FLAG_SEND_DF ?
> 
> Because myflags is the runtime flags for _this_ client, while
> client->exp->nbdflags are the base flags shared by _all_ clients.  If
> client A requests structured reply, but client B does not, then we don't
> want to advertise DF to client B; but amending client->exp->nbdflags
> would have that effect.

I stand corrected - it looks like a fresh client->exp is created per
client, as evidenced by:

diff --git i/nbd/client.c w/nbd/client.c
index b9dc829175f9..9e05f1a0e2a3 100644
--- i/nbd/client.c
+++ w/nbd/client.c
@@ -1011,6 +1011,8 @@ int nbd_receive_negotiate(AioContext *aio_context,
QIOChannel *ioc,
     assert(info->name);
     trace_nbd_receive_negotiate_name(info->name);

+    if (getenv ("MY_HACK"))
+        info->structured_reply = false;
     result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname,
outioc,
                                  info->structured_reply, &zeroes, errp);

diff --git i/nbd/server.c w/nbd/server.c
index d5078f7468af..6f3a83704fb3 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -457,6 +457,7 @@ static int
nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
     myflags = client->exp->nbdflags;
     if (client->structured_reply) {
         myflags |= NBD_FLAG_SEND_DF;
+        client->exp->nbdflags |= NBD_FLAG_SEND_DF;
     }
     trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags);
     stq_be_p(buf, client->exp->size);

$ ./qemu-nbd -r -f raw file -t &

$  ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
nbd://localhost:10809 -c quit
32145@1567207628.519883:nbd_receive_negotiate_size_flags Size is
1049088, export flags 0x48f

$ MY_HACK=1 ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
nbd://localhost:10809 -c quit
32156@1567207630.417815:nbd_receive_negotiate_size_flags Size is
1049088, export flags 0x40f

$  ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
nbd://localhost:10809 -c quit
32167@1567207635.202940:nbd_receive_negotiate_size_flags Size is
1049088, export flags 0x48f

The export flags change per client, so I _can_ store into
client->exp->nbdflags.  Will do that for v2.

Meanwhile, this points out a missing feature in libnbd - for testing
purposes, it would be really nice to be able to purposefully cripple the
client to NOT request structured replies automatically (default enabled,
but the ability to turn it off is useful for interop testing, as in this
thread).  I already recently added a --no-sr flag to nbdkit for a
similar reason (but that's creating a server which refuses to advertise,
where here I want a guest that refuses to ask).  Guess I'll be adding a
patch for that, too :)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-23 14:30 [Qemu-devel] cross-project patches: Add NBD Fast Zero support Eric Blake
2019-08-23 14:34 ` [Qemu-devel] [PATCH 0/1] NBD protocol change to add fast zero support Eric Blake
2019-08-23 14:34   ` [Qemu-devel] [PATCH 1/1] protocol: Add NBD_CMD_FLAG_FAST_ZERO Eric Blake
2019-08-23 18:48     ` Wouter Verhelst
2019-08-23 18:58       ` Eric Blake
2019-08-24  6:44         ` Wouter Verhelst
2019-08-28  9:57     ` Vladimir Sementsov-Ogievskiy
2019-08-28 13:04       ` Eric Blake
2019-08-28 13:45         ` Vladimir Sementsov-Ogievskiy
2019-09-03 20:53   ` [Qemu-devel] [Libguestfs] [PATCH 0/1] NBD protocol change to add fast zero support Eric Blake
2019-08-23 14:37 ` [Qemu-devel] [PATCH 0/5] Add NBD fast zero support to qemu client and server Eric Blake
2019-08-23 14:37   ` [Qemu-devel] [PATCH 1/5] nbd: Improve per-export flag handling in server Eric Blake
2019-08-30 18:00     ` Vladimir Sementsov-Ogievskiy
2019-08-30 23:10       ` Eric Blake
2019-08-30 23:32         ` Eric Blake [this message]
2019-09-03 16:39           ` Eric Blake
2019-09-04 17:08     ` Vladimir Sementsov-Ogievskiy
2019-08-23 14:37   ` [Qemu-devel] [PATCH 2/5] nbd: Prepare for NBD_CMD_FLAG_FAST_ZERO Eric Blake
2019-08-30 18:07     ` Vladimir Sementsov-Ogievskiy
2019-08-30 23:37       ` Eric Blake
2019-08-31  8:11         ` Vladimir Sementsov-Ogievskiy
2019-09-03 18:49       ` Eric Blake
2019-08-31  8:20     ` Vladimir Sementsov-Ogievskiy
2019-08-23 14:37   ` [Qemu-devel] [PATCH 3/5] nbd: Implement client use of NBD FAST_ZERO Eric Blake
2019-08-30 18:11     ` Vladimir Sementsov-Ogievskiy
2019-08-23 14:37   ` [Qemu-devel] [PATCH 4/5] nbd: Implement server " Eric Blake
2019-08-30 18:40     ` Vladimir Sementsov-Ogievskiy
2019-08-23 14:37   ` [Qemu-devel] [PATCH 5/5] nbd: Tolerate more errors to structured reply request Eric Blake
2019-08-23 16:41     ` Eric Blake
2019-08-28 13:55   ` [Qemu-devel] [PATCH 0/5] Add NBD fast zero support to qemu client and server Vladimir Sementsov-Ogievskiy
2019-08-28 14:05     ` Eric Blake
2019-08-23 14:38 ` [Qemu-devel] [libnbd PATCH 0/1] libnbd support for new fast zero Eric Blake
2019-08-23 14:38   ` [Qemu-devel] [libnbd PATCH 1/1] api: Add support for FAST_ZERO flag Eric Blake
2019-08-27 12:25     ` [Qemu-devel] [Libguestfs] " Richard W.M. Jones
2019-08-23 14:40 ` [Qemu-devel] [nbdkit PATCH 0/3] nbdkit support for new NBD fast zero Eric Blake
2019-08-23 14:40   ` [Qemu-devel] [nbdkit PATCH 1/3] server: Add internal support for NBDKIT_FLAG_FAST_ZERO Eric Blake
2019-08-23 14:40   ` [Qemu-devel] [nbdkit PATCH 2/3] filters: Add .can_fast_zero hook Eric Blake
2019-08-23 14:40   ` [Qemu-devel] [nbdkit PATCH 3/3] plugins: " Eric Blake
2019-08-23 21:16     ` [Qemu-devel] [Libguestfs] " Eric Blake
2019-08-27 15:43     ` Richard W.M. Jones
2019-08-23 15:05 ` [Qemu-devel] cross-project patches: Add NBD Fast Zero support Vladimir Sementsov-Ogievskiy
2019-08-27 12:14 ` [Qemu-devel] [Libguestfs] " Richard W.M. Jones
2019-08-27 13:23   ` Eric Blake

Reply instructions:

You may reply publically 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=55ebe9db-5a28-d844-89f7-2dc9a7d977d7@redhat.com \
    --to=eblake@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=libguestfs@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=nbd@other.debian.org \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=vsementsov@virtuozzo.com \
    /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

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox