Re: [PATCH 4/6] qemu-nbd: add compression flag support

[Eric Blake]

On 10/1/19 2:27 PM, Andrey Shinkevich wrote:
> Added possibility to write compressed data by using the
> blk_write_compressed. This action has the limitations of the format
> driver. For example we can't write compressed data over other.

> +++ b/blockdev-nbd.c
> @@ -191,7 +191,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, 
> const char *name,
>       }
>   
>       exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable,
> -                         NULL, false, on_eject_blk, errp);
> +                         0, NULL, false, on_eject_blk, errp);

This is a lot of parameters.  Should we be combining some of them into a 
struct, or even at least the booleans into a flags parameter?


> +++ b/include/block/nbd.h
> @@ -25,6 +25,10 @@
>   #include "crypto/tlscreds.h"
>   #include "qapi/error.h"
>   
> +enum {
> +  NBD_INTERNAL_FLAG_COMPRESS = 1 << 1, /* Use write compressed */
> +};

What happened to flag 1 << 0?  What other flags do you anticipate adding?


> +++ b/nbd/server.c
> @@ -91,6 +91,7 @@ struct NBDExport {
>       uint16_t nbdflags;
>       QTAILQ_HEAD(, NBDClient) clients;
>       QTAILQ_ENTRY(NBDExport) next;
> +    uint32_t iflags;
>   
>       AioContext *ctx;
>   
> @@ -1471,7 +1472,8 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>   
>   NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>                             uint64_t size, const char *name, const char *desc,
> -                          const char *bitmap, bool readonly, bool shared,
> +                          const char *bitmap, bool readonly,
> +                          bool shared, uint32_t iflags,
>                             void (*close)(NBDExport *), bool writethrough,
>                             BlockBackend *on_eject_blk, Error **errp)

Again, this feels like a lot of parameters, combining more through 
iflags may make sense.

>   {
> @@ -1525,6 +1527,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t 
> dev_offset,
>           exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
>                             NBD_FLAG_SEND_FAST_ZERO);
>       }
> +    exp->iflags = iflags;
>       assert(size <= INT64_MAX - dev_offset);
>       exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
>   
> @@ -2312,6 +2315,9 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>           if (request->flags & NBD_CMD_FLAG_FUA) {
>               flags |= BDRV_REQ_FUA;
>           }
> +        if (exp->iflags & NBD_INTERNAL_FLAG_COMPRESS) {
> +            flags |= BDRV_REQ_WRITE_COMPRESSED;
> +        }

This unconditionally tries to make all writes compressed if the option 
was selected when starting qemu-nbd.  Should we at least sanity check 
that it will work during nbd_export_new, rather than waiting to find out 
on the first (failed) write, whether it actually works?

/me looks ahead [1]

>           ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
>                            data, request->len, flags);
>           return nbd_send_generic_reply(client, request->handle, ret,
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 9032b6d..3765c4b 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -139,6 +139,7 @@ static void usage(const char *name)
>   "      --discard=MODE        set discard mode (ignore, unmap)\n"
>   "      --detect-zeroes=MODE  set detect-zeroes mode (off, on, unmap)\n"
>   "      --image-opts          treat FILE as a full set of image options\n"
> +"  -C, --compress            use data compression (if the target format supports 
> it)\n"

I'm not necessarily opposed to burning a short option.  But it's a shame 
that we can't use -c to be similar to 'qemu-img convert -c'.  Requiring 
the use of a long option is also okay (short options have to be for the 
more likely uses, although it does seem like this use case might qualify).

> @@ -610,7 +612,7 @@ int main(int argc, char **argv)
>       int64_t fd_size;
>       QemuOpts *sn_opts = NULL;
>       const char *sn_id_or_name = NULL;
> -    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:L";
> +    const char *sopt = "hVb:o:p:CrsnP:c:dvk:e:f:tl:x:T:D:B:L";

Pre-existing, but we don't sort this very well.

>       struct option lopt[] = {
>           { "help", no_argument, NULL, 'h' },
>           { "version", no_argument, NULL, 'V' },
> @@ -619,6 +621,7 @@ int main(int argc, char **argv)
>           { "socket", required_argument, NULL, 'k' },
>           { "offset", required_argument, NULL, 'o' },
>           { "read-only", no_argument, NULL, 'r' },
> +        { "compress", no_argument, NULL, 'C'},
>           { "partition", required_argument, NULL, 'P' },

Above you put 'C' between 'p' and 'r', but here between 'r' and 'P'.  We 
really don't sort very well :)

>           { "bitmap", required_argument, NULL, 'B' },
>           { "connect", required_argument, NULL, 'c' },
> @@ -786,6 +789,9 @@ int main(int argc, char **argv)
>               readonly = true;
>               flags &= ~BDRV_O_RDWR;
>               break;
> +        case 'C':
> +            iflags |= NBD_INTERNAL_FLAG_COMPRESS;
> +            break;
>           case 'P':

At least this matches your lopt[] ordering.

>               warn_report("The '-P' option is deprecated; use --image-opts with 
> "
>                           "a raw device wrapper for subset exports instead");
> @@ -1117,6 +1123,14 @@ int main(int argc, char **argv)
>   
>       blk_set_enable_write_cache(blk, !writethrough);
>   
> +    if ((iflags & NBD_INTERNAL_FLAG_COMPRESS) &&
> +        !(bs->drv && bs->drv->bdrv_co_pwritev_compressed_part))
> +    {
> +        error_report("Compression not supported for this file format %s",
> +                     argv[optind]);
> +        exit(EXIT_FAILURE);
> +    }
> +

[1] ah, you DO make sure that compression is supported before passing 
the option through.

The idea seems reasonable.

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