Re: [Libguestfs] [PATCH libnbd v4 1/2] copy: Store the preferred block size in the operations struct
On Thu, Jul 07, 2022 at 01:54:55PM -0500, Eric Blake wrote:
> + 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?
No it's not, because the parameter is declared as uint64_t. I guess
it's my spidey sense about some imagined platform where blksize_t is a
strange type that can't be implicitly converted, but I don't know if
that's possible.
> 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.
It seems very unlikely. The protocol limits these block sizes to
uint32_t (ie. unsigned int) right now, and preferred size to much
smaller. I'm not sure what you anticipate with your upcoming 64 bit
patches.
As this is preferred (not max) block size, it seems unlikely that we'd
ever want anything larger than 31 bits in future.
I think I used uint64_t here because it reduced the need to worry
about overflow.
> +++ 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.
I'm going to say we shouldn't do that, for a few reasons:
- If the platform had a randomly large PIPE_BUF then it could cause
nbdcopy to automatically adjust request size upwards. We saw this
in an earlier iteration of this series where I set the null_ops
preferred size to 4M, which meant nbdcopy silently made all
requests huge, which had strange knock-on effects.
- As you say, atomic pipe size is not the same as preferred size.
It's hard to say what the preferred size for a kernel pipe is (it's
not a problem handling smallish writes).
- Despite the name, this file applies to sockets and anything else
that isn't S_ISREG / S_ISBLK.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit