QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Eric Blake <eblake@redhat.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:00:37 +0000
Message-ID: <92e9cfa2-fbaa-259d-7a04-91187cc809b6@virtuozzo.com> (raw)
In-Reply-To: <20190823143726.27062-2-eblake@redhat.com>

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:
> 
> All callers to nbd_export_new() passed a single flag based solely on
> whether the export allows writes.  Later, we then pass a constant set
> of flags to nbd_negotiate_options() (namely, the set of flags which we
> always support, at least for writable images), which is then further
> dynamically modified based on client requests for structured options.
> Finally, when processing NBD_OPT_EXPORT_NAME or NBD_OPT_EXPORT_GO we
> bitwise-or the original caller's flag with the runtime set of flags
> we've built up over several functions.
> 
> Let's refactor things to instead compute a baseline of flags as soon
> as possible, in nbd_export_new(), and changing the signature for the
> callers to pass in a simpler bool rather than having to figure out
> flags.  We can then get rid of the 'myflags' parameter to various
> functions, and instead refer to client for everything we need (we
> still have to perform a bitwise-OR during NBD_OPT_EXPORT_NAME and
> NBD_OPT_EXPORT_GO, but it's easier to see what is being computed).
> This lets us quit advertising senseless flags for read-only images, as
> well as making the next patch for exposing FAST_ZERO support easier to
> write.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   include/block/nbd.h |  2 +-
>   blockdev-nbd.c      |  3 +--
>   nbd/server.c        | 62 +++++++++++++++++++++++++--------------------
>   qemu-nbd.c          |  6 ++---
>   4 files changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 991fd52a5134..2c87b42dfd48 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -326,7 +326,7 @@ typedef struct NBDClient NBDClient;
> 
>   NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>                             uint64_t size, const char *name, const char *desc,
> -                          const char *bitmap, uint16_t nbdflags, bool shared,
> +                          const char *bitmap, bool readonly, bool shared,
>                             void (*close)(NBDExport *), bool writethrough,
>                             BlockBackend *on_eject_blk, Error **errp);
>   void nbd_export_close(NBDExport *exp);
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index ecfa2ef0adb5..1cdffab4fce1 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -187,8 +187,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>           writable = false;
>       }
> 
> -    exp = nbd_export_new(bs, 0, len, name, NULL, bitmap,
> -                         writable ? 0 : NBD_FLAG_READ_ONLY, !writable,
> +    exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
>                            NULL, false, on_eject_blk, errp);
>       if (!exp) {
>           return;
> diff --git a/nbd/server.c b/nbd/server.c
> index 0fb41c6c50ea..b5577828aa44 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -423,14 +423,14 @@ static void nbd_check_meta_export(NBDClient *client)
> 
>   /* Send a reply to NBD_OPT_EXPORT_NAME.
>    * Return -errno on error, 0 on success. */
> -static int nbd_negotiate_handle_export_name(NBDClient *client,
> -                                            uint16_t myflags, bool no_zeroes,
> +static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
>                                               Error **errp)
>   {
>       char name[NBD_MAX_NAME_SIZE + 1];
>       char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
>       size_t len;
>       int ret;
> +    uint16_t myflags;
> 
>       /* Client sends:
>           [20 ..  xx]   export name (length bytes)
> @@ -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 ?

Or even do it earlier, when client->structured_reply becomes true?

> +    trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags);
>       stq_be_p(buf, client->exp->size);
> -    stw_be_p(buf + 8, client->exp->nbdflags | myflags);
> +    stw_be_p(buf + 8, myflags);
>       len = no_zeroes ? 10 : sizeof(buf);
>       ret = nbd_write(client->ioc, buf, len, errp);
>       if (ret < 0) {
> @@ -526,8 +529,7 @@ static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp)
>   /* Handle NBD_OPT_INFO and NBD_OPT_GO.
>    * Return -errno on error, 0 if ready for next option, and 1 to move
>    * into transmission phase.  */
> -static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
> -                                     Error **errp)
> +static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
>   {
>       int rc;
>       char name[NBD_MAX_NAME_SIZE + 1];

[..]


-- 
Best regards,
Vladimir

  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 [this message]
2019-08-30 23:10       ` Eric Blake
2019-08-30 23:32         ` Eric Blake
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=92e9cfa2-fbaa-259d-7a04-91187cc809b6@virtuozzo.com \
    --to=vsementsov@virtuozzo.com \
    --cc=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 \
    /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
	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.git