Re: [Qemu-devel] [PATCH 1/5] nbd: Improve per-export flag handling in server

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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