[Libguestfs] [PATCH libnbd v4 1/2] copy: Store the preferred block size in the operations struct

[Eric Blake]

On Thu, Jun 30, 2022 at 05:43:56PM +0100, Richard W.M. Jones wrote:
> This will be used in a subsequent commit.  At the moment the preferred
> block size for all sources / destinations is simply calculated and
> stored.
> ---
>  copy/file-ops.c |  4 +++-
>  copy/main.c     | 29 +++++++++++++++++++++++------
>  copy/nbd-ops.c  | 10 ++++++++++
>  copy/nbdcopy.h  |  4 +++-
>  copy/null-ops.c |  1 +
>  copy/pipe-ops.c |  1 +
>  6 files changed, 41 insertions(+), 8 deletions(-)
> 

> +++ b/copy/main.c
> @@ -512,10 +512,26 @@ open_local (const char *filename, direction d)
>      fprintf (stderr, "%s: %s: %m\n", prog, filename);
>      exit (EXIT_FAILURE);
>    }
> -  if (S_ISBLK (stat.st_mode) || S_ISREG (stat.st_mode))
> -    return file_create (filename, fd, stat.st_size, S_ISBLK (stat.st_mode), d);
> -  else {
> -    /* Probably stdin/stdout, a pipe or a socket. */
> +  if (S_ISREG (stat.st_mode))   /* Regular file. */
> +    return file_create (filename, fd,
> +                        stat.st_size, (uint64_t) stat.st_blksize, false, d);

Is the cast to uint64_t actually needed?

> +  else if (S_ISBLK (stat.st_mode)) { /* Block device. */
> +    unsigned int blkioopt;
> +
> +#ifdef BLKIOOPT
> +    if (ioctl (fd, BLKIOOPT, &blkioopt) == -1) {
> +      fprintf (stderr, "warning: cannot get optimal I/O size: %s: %m",
> +               filename);
> +      blkioopt = 4096;
> +    }
> +#else
> +    blkioopt = 4096;
> +#endif
> +
> +    return file_create (filename, fd,
> +                        stat.st_size, (uint64_t) blkioopt, true, d);

Ditto.

> +++ b/copy/nbd-ops.c
> @@ -112,12 +112,22 @@ open_one_nbd_handle (struct rw_nbd *rwn)
>     * the same way.
>     */
>    if (rwn->handles.len == 0) {
> +    int64_t block_size;
> +
>      rwn->can_zero = nbd_can_zero (nbd) > 0;
> +
>      rwn->rw.size = nbd_get_size (nbd);
>      if (rwn->rw.size == -1) {
>        fprintf (stderr, "%s: %s: %s\n", prog, rwn->rw.name, nbd_get_error ());
>        exit (EXIT_FAILURE);
>      }
> +
> +    block_size = nbd_get_block_size (nbd, LIBNBD_SIZE_PREFERRED);
> +    if (block_size == -1) {
> +      fprintf (stderr, "%s: %s: %s\n", prog, rwn->rw.name, nbd_get_error ());
> +      exit (EXIT_FAILURE);
> +    }
> +    rwn->rw.preferred = block_size == 0 ? 4096 : block_size;

Looks good for NBD.

>    }
>  
>    if (handles_append (&rwn->handles, nbd) == -1) {
> diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
> index 19797dfd66..9438cce417 100644
> --- a/copy/nbdcopy.h
> +++ b/copy/nbdcopy.h
> @@ -43,6 +43,7 @@ struct rw {
>    struct rw_ops *ops;           /* Operations. */
>    const char *name;             /* Printable name, for error messages etc. */
>    int64_t size;                 /* May be -1 for streams. */
> +  uint64_t preferred;           /* Preferred block size. */

Will we ever need to worry about preferred block sizes being larger
than signed int?  Using uint64_t feels a bit oversized, but is not
technically wrong even if we never exceed 2^31-bit blocks in practice.

> +++ b/copy/pipe-ops.c
> @@ -43,6 +43,7 @@ pipe_create (const char *name, int fd)
>    rwp->rw.ops = &pipe_ops;
>    rwp->rw.name = name;
>    rwp->rw.size = -1;
> +  rwp->rw.preferred = 4096;

Should we try to use PIPE_BUF instead of 4096?  Or on Linux,
fcntl(F_GETPIPE_SZ)?  Then again, 4096 is probably a safe default,
even if we aren't pushing the pipe to its full atomic capacity.

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