9:56 a.m.
On 06/29/22 13:03, 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 | 9 ++++++++-
copy/main.c | 9 ++++++---
copy/nbd-ops.c | 9 +++++++++
copy/nbdcopy.h | 4 +++-
copy/null-ops.c | 1 +
copy/pipe-ops.c | 1 +
6 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/copy/file-ops.c b/copy/file-ops.c
index ab3787545c..3b901bcdb8 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -241,7 +241,8 @@ seek_hole_supported (int fd)
struct rw *
file_create (const char *name, int fd,
- off_t st_size, bool is_block, direction d)
+ off_t st_size, blksize_t st_blksize,
+ bool is_block, direction d)
{
struct rw_file *rwf = calloc (1, sizeof *rwf);
if (rwf == NULL) { perror ("calloc"); exit (EXIT_FAILURE); }
@@ -262,6 +263,11 @@ file_create (const char *name, int fd,
perror ("lseek");
exit (EXIT_FAILURE);
}
+ rwf->rw.preferred = 4096;
+#ifdef BLKIOOPT
+ if (ioctl (fd, BLKIOOPT, &rwf->rw.preferred))
+ fprintf (stderr, "warning: cannot get optimal I/O size: %s: %m", name);
+#endif
From my reading of the kernel, this ioctl stores an "unsigned
int",
which (in practice) will not set all bytes in the "preferred"
uint64_t
field.
Additionally, if the ioctl fails, I'm not sure if we have a guarantee
that the destination will be left unchanged. I suggest:
#ifdef BLKIOOPT
unsigned int blkioopt;
if (ioctl (fd, BLKIOOPT, &blkioopt))
rwf->rw.preferred = blkioopt;
else {
fprintf (stderr, "warning: cannot get optimal I/O size: %s: %m",
name);
rwf->rw.preferred = 4096;
}
#else
rwf->rw.preferred = 4096;
#endif
... however, I also suggest doing this *elsewhere* (not here). I think
this function should take a plain uint64_t "preferred" param, and simply
assign that to "rwf->rw.preferred", independently of "is_block".
(There
is one call site only, so moving the logic out there does not lead to
code duplication.) I'll explain the reason below.
rwf->seek_hole_supported = seek_hole_supported (fd);
rwf->sector_size = 4096;
#ifdef BLKSSZGET
@@ -282,6 +288,7 @@ file_create (const char *name, int fd,
else {
/* Regular file. */
rwf->rw.size = st_size;
+ rwf->rw.preferred = st_blksize;
rwf->seek_hole_supported = seek_hole_supported (fd);
/* Possible efficient zero methods for regular file. */
#ifdef FALLOC_FL_PUNCH_HOLE
diff --git a/copy/main.c b/copy/main.c
index cc379e98bc..9b551de664 100644
--- a/copy/main.c
+++ b/copy/main.c
@@ -513,7 +513,9 @@ open_local (const char *filename, direction d)
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);
+ return file_create (filename, fd,
+ stat.st_size, stat.st_blksize, S_ISBLK (stat.st_mode),
+ d);
else {
/* Probably stdin/stdout, a pipe or a socket. */
synchronous = true; /* Force synchronous mode for pipes. */
I couldn't find any info on whether fstat() fills in "stat.st_blksize"
at all for block devices. I understand that with "is_block" set, the
potential garbage will be ignored in file_create() anyway, but passing
garbage *itself* is not great.
So I suggest to split
S_ISBLK (stat.st_mode) || S_ISREG (stat.st_mode)
to two separate branche. Pass stat.st_blksize (as "uint64_t preferred")
on the S_ISREG branch. Do the following on the S_ISBLK branch:
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, blkioopt, false, d);
@@ -528,8 +530,9 @@ print_rw (struct rw *rw, const char *prefix, FILE
*fp)
char buf[HUMAN_SIZE_LONGEST];
fprintf (fp, "%s: %s \"%s\"\n", prefix, rw->ops->ops_name,
rw->name);
- fprintf (fp, "%s: size=%" PRIi64 " (%s)\n",
- prefix, rw->size, human_size (buf, rw->size, NULL));
+ fprintf (fp, "%s: size=%" PRIi64 " (%s), preferred block size=%"
PRIu64 "\n",
+ prefix, rw->size, human_size (buf, rw->size, NULL),
+ rw->preferred);
}
/* Default implementation of rw->ops->get_extents for backends which
diff --git a/copy/nbd-ops.c b/copy/nbd-ops.c
index 3bc26ba613..6c5d59c578 100644
--- a/copy/nbd-ops.c
+++ b/copy/nbd-ops.c
@@ -113,11 +113,20 @@ open_one_nbd_handle (struct rw_nbd *rwn)
*/
if (rwn->handles.len == 0) {
rwn->can_zero = nbd_can_zero (nbd) > 0;
+
rwn->rw.size = nbd_get_size (nbd);
if (rwn->rw.size == -1) {
This existent code is fine, as nbd_get_size returns int64_t, and
"rwn->rw.size" also has type int64_t...
fprintf (stderr, "%s: %s: %s\n", prog,
rwn->rw.name, nbd_get_error ());
exit (EXIT_FAILURE);
}
+
+ rwn->rw.preferred = nbd_get_block_size (nbd, LIBNBD_SIZE_PREFERRED);
+ if (rwn->rw.preferred == -1) {
... but this is hard to read (albeit correct in practice).
In case nbd_get_block_size() fails, it will return (int64_t)-1, and we
end up assigning "rwn->rw.preferred" UINT64_MAX. In the comparison on
the next line, the (int)-1 will in practice be converted to uint64_t, so
we do the comparison against UINT64_MAX. Correct, but hard to read.
I'd use a separate "int64_t block_size" local variable for this:
block_size = nbd_get_block_size (...);
if (block_size == -1) {
fprintf (...);
exit (...);
}
rwn->rw.preferred = block_size == 0 ? 4096 : block_size;
But, replacing -1 with UINT64_MAX, or else (uint64_t)-1, would be
informative too.
+ fprintf (stderr, "%s: %s: %s\n", prog,
rwn->rw.name, nbd_get_error ());
+ exit (EXIT_FAILURE);
+ }
+ if (rwn->rw.preferred == 0)
+ rwn->rw.preferred = 4096;
}
if (handles_append (&rwn->handles, nbd) == -1) {
diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
index 19797dfd66..9b02ddf270 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. */
/* Followed by private data for the particular subtype. */
};
@@ -53,7 +54,8 @@ typedef enum { READING, WRITING } direction;
/* Create subtypes. */
extern struct rw *file_create (const char *name, int fd,
- off_t st_size, bool is_block, direction d);
+ off_t st_size, blksize_t st_blksize,
+ bool is_block, direction d);
extern struct rw *nbd_rw_create_uri (const char *name,
const char *uri, direction d);
extern struct rw *nbd_rw_create_subprocess (const char **argv, size_t argc,
diff --git a/copy/null-ops.c b/copy/null-ops.c
index 1218a623e2..854e291613 100644
--- a/copy/null-ops.c
+++ b/copy/null-ops.c
@@ -45,6 +45,7 @@ null_create (const char *name)
rw->rw.ops = &null_ops;
rw->rw.name = name;
rw->rw.size = INT64_MAX;
+ rw->rw.preferred = 4 * 1024 * 1024;
return &rw->rw;
}
diff --git a/copy/pipe-ops.c b/copy/pipe-ops.c
index 3c8b6c2b39..3815f824fc 100644
--- a/copy/pipe-ops.c
+++ 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;
rwp->fd = fd;
return &rwp->rw;
}
Thanks,
Laszlo