Richard W.M. Jones
2021-Feb-22 11:39 UTC
[Libguestfs] [PATCH libnbd 1/2] nbdcopy: Do not use trim for zeroing
The patch doesn't compile for me:
pipe-ops.c:121:17: error: initialization of ?_Bool (*)(struct rw *, uint64_t,
uint64_t, _Bool)? {aka ?_Bool (*)(struct rw *, long unsigned int, long
unsigned int, _Bool)?} from incompatible pointer type ?_Bool (*)(struct rw *,
uint64_t, uint64_t)? {aka ?_Bool (*)(struct rw *, long unsigned int, long
unsigned int)?} [-Werror=incompatible-pointer-types]
121 | .synch_zero = pipe_synch_trim_zero,
| ^~~~~~~~~~~~~~~~~~~~
pipe-ops.c:121:17: note: (near initialization for ?pipe_ops.synch_zero?)
pipe-ops.c:133:18: error: initialization of ?_Bool (*)(struct rw *, struct
command *, nbd_completion_callback, _Bool)? from incompatible pointer type
?_Bool (*)(struct rw *, struct command *, nbd_completion_callback)?
[-Werror=incompatible-pointer-types]
133 | .asynch_zero = pipe_asynch_trim_zero,
| ^~~~~~~~~~~~~~~~~~~~~
pipe-ops.c:133:18: note: (near initialization for ?pipe_ops.asynch_zero?)
More comments below.
On Sun, Feb 21, 2021 at 03:09:05AM +0200, Nir Soffer
wrote:> diff --git a/copy/file-ops.c b/copy/file-ops.c
> index 2a239d0..d0b9447 100644
> --- a/copy/file-ops.c
> +++ b/copy/file-ops.c
> @@ -100,10 +100,9 @@ file_synch_write (struct rw *rw,
> }
>
> static bool
> -file_synch_trim (struct rw *rw, uint64_t offset, uint64_t count)
> +file_punch_hole(int fd, uint64_t offset, uint64_t count)
(Style) missing space before '(' ...
> {
> #ifdef FALLOC_FL_PUNCH_HOLE
> - int fd = rw->u.local.fd;
> int r;
>
> r = fallocate (fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> @@ -113,17 +112,14 @@ file_synch_trim (struct rw *rw, uint64_t offset,
uint64_t count)
> exit (EXIT_FAILURE);
> }
> return true;
> -#else /* !FALLOC_FL_PUNCH_HOLE */
> - return false;
> #endif
> + return false;
> }
>
> static bool
> -file_synch_zero (struct rw *rw, uint64_t offset, uint64_t count)
> +file_zero_range(int fd, uint64_t offset, uint64_t count)
... and here
> {
> - if (S_ISREG (rw->u.local.stat.st_mode)) {
> #ifdef FALLOC_FL_ZERO_RANGE
> - int fd = rw->u.local.fd;
> int r;
>
> r = fallocate (fd, FALLOC_FL_ZERO_RANGE, offset, count);
> @@ -133,11 +129,13 @@ file_synch_zero (struct rw *rw, uint64_t offset,
uint64_t count)
> }
> return true;
> #endif
> - }
> - else if (S_ISBLK (rw->u.local.stat.st_mode) &&
> - IS_ALIGNED (offset | count, rw->u.local.sector_size)) {
> + return false;
> +}
> +
> +static bool
> +file_zeroout(int fd, uint64_t offset, uint64_t count)
... and here
> +{
> #ifdef BLKZEROOUT
> - int fd = rw->u.local.fd;
> int r;
> uint64_t range[2] = {offset, count};
>
> @@ -148,6 +146,31 @@ file_synch_zero (struct rw *rw, uint64_t offset,
uint64_t count)
> }
> return true;
> #endif
> + return false;
> +}
> +
> +static bool
> +file_synch_trim (struct rw *rw, uint64_t offset, uint64_t count)
> +{
> + return file_punch_hole(rw->u.local.fd, offset, count);
> +}
> +
> +static bool
> +file_synch_zero (struct rw *rw, uint64_t offset, uint64_t count, bool
allocate)
... and overlong line.
> +{
> + int fd = rw->u.local.fd;
> +
> + if (S_ISREG (rw->u.local.stat.st_mode)) {
> + if (allocate) {
> + return file_zero_range (fd, offset, count);
> + } else {
> + return file_punch_hole (fd, offset, count);
> + }
> + }
> + else if (S_ISBLK (rw->u.local.stat.st_mode) &&
> + IS_ALIGNED (offset | count, rw->u.local.sector_size)) {
> + /* Always allocate, discard and gurantee zeroing. */
> + return file_zeroout (fd, offset, count);
> }
> return false;
In the S_ISBLK && not aligned case, is giving up OK?
> diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
> index 69fac2a..21d09bf 100644
> --- a/copy/nbdcopy.h
> +++ b/copy/nbdcopy.h
> @@ -134,7 +134,8 @@ struct rw_ops {
> bool (*synch_trim) (struct rw *rw, uint64_t offset, uint64_t count);
>
> /* Synchronously zero. If not possible, returns false. */
> - bool (*synch_zero) (struct rw *rw, uint64_t offset, uint64_t count);
> + bool (*synch_zero) (struct rw *rw, uint64_t offset, uint64_t count,
> + bool allocate);
After this change ops->synch_trim and ops->asynch_trim are no longer
used, so I guess they should be removed completely from the code?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
Nir Soffer
2021-Feb-22 12:12 UTC
[Libguestfs] [PATCH libnbd 1/2] nbdcopy: Do not use trim for zeroing
On Mon, Feb 22, 2021, 13:39 Richard W.M. Jones <rjones at redhat.com> wrote:> The patch doesn't compile for me: > > pipe-ops.c:121:17: error: initialization of ?_Bool (*)(struct rw *, > uint64_t, uint64_t, _Bool)? {aka ?_Bool (*)(struct rw *, long unsigned > int, long unsigned int, _Bool)?} from incompatible pointer type ?_Bool > (*)(struct rw *, uint64_t, uint64_t)? {aka ?_Bool (*)(struct rw *, long > unsigned int, long unsigned int)?} [-Werror=incompatible-pointer-types] > 121 | .synch_zero = pipe_synch_trim_zero, > | ^~~~~~~~~~~~~~~~~~~~ > pipe-ops.c:121:17: note: (near initialization for ?pipe_ops.synch_zero?) > pipe-ops.c:133:18: error: initialization of ?_Bool (*)(struct rw *, struct > command *, nbd_completion_callback, _Bool)? from incompatible pointer type > ?_Bool (*)(struct rw *, struct command *, nbd_completion_callback)? > [-Werror=incompatible-pointer-types] > 133 | .asynch_zero = pipe_asynch_trim_zero, > | ^~~~~~~~~~~~~~~~~~~~~ > pipe-ops.c:133:18: note: (near initialization for ?pipe_ops.asynch_zero?) >It compiled here on top of master, will check again.> More comments below. > > On Sun, Feb 21, 2021 at 03:09:05AM +0200, Nir Soffer wrote: > > diff --git a/copy/file-ops.c b/copy/file-ops.c > > index 2a239d0..d0b9447 100644 > > --- a/copy/file-ops.c > > +++ b/copy/file-ops.c > > @@ -100,10 +100,9 @@ file_synch_write (struct rw *rw, > > } > > > > static bool > > -file_synch_trim (struct rw *rw, uint64_t offset, uint64_t count) > > +file_punch_hole(int fd, uint64_t offset, uint64_t count) > > (Style) missing space before '(' ... >Sorry, will fix in next version.> > { > > #ifdef FALLOC_FL_PUNCH_HOLE > > - int fd = rw->u.local.fd; > > int r; > > > > r = fallocate (fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, > > @@ -113,17 +112,14 @@ file_synch_trim (struct rw *rw, uint64_t offset, > uint64_t count) > > exit (EXIT_FAILURE); > > } > > return true; > > -#else /* !FALLOC_FL_PUNCH_HOLE */ > > - return false; > > #endif > > + return false; > > } > > > > static bool > > -file_synch_zero (struct rw *rw, uint64_t offset, uint64_t count) > > +file_zero_range(int fd, uint64_t offset, uint64_t count) > > ... and here > > > { > > - if (S_ISREG (rw->u.local.stat.st_mode)) { > > #ifdef FALLOC_FL_ZERO_RANGE > > - int fd = rw->u.local.fd; > > int r; > > > > r = fallocate (fd, FALLOC_FL_ZERO_RANGE, offset, count); > > @@ -133,11 +129,13 @@ file_synch_zero (struct rw *rw, uint64_t offset, > uint64_t count) > > } > > return true; > > #endif > > - } > > - else if (S_ISBLK (rw->u.local.stat.st_mode) && > > - IS_ALIGNED (offset | count, rw->u.local.sector_size)) { > > + return false; > > +} > > + > > +static bool > > +file_zeroout(int fd, uint64_t offset, uint64_t count) > > ... and here > > > +{ > > #ifdef BLKZEROOUT > > - int fd = rw->u.local.fd; > > int r; > > uint64_t range[2] = {offset, count}; > > > > @@ -148,6 +146,31 @@ file_synch_zero (struct rw *rw, uint64_t offset, > uint64_t count) > > } > > return true; > > #endif > > + return false; > > +} > > + > > +static bool > > +file_synch_trim (struct rw *rw, uint64_t offset, uint64_t count) > > +{ > > + return file_punch_hole(rw->u.local.fd, offset, count); > > +} > > + > > +static bool > > +file_synch_zero (struct rw *rw, uint64_t offset, uint64_t count, bool > allocate) > > ... and overlong line. > > > +{ > > + int fd = rw->u.local.fd; > > + > > + if (S_ISREG (rw->u.local.stat.st_mode)) { > > + if (allocate) { > > + return file_zero_range (fd, offset, count); > > + } else { > > + return file_punch_hole (fd, offset, count); > > + } > > + } > > + else if (S_ISBLK (rw->u.local.stat.st_mode) && > > + IS_ALIGNED (offset | count, rw->u.local.sector_size)) { > > + /* Always allocate, discard and gurantee zeroing. */ > > + return file_zeroout (fd, offset, count); > > } > > return false; > > In the S_ISBLK && not aligned case, is giving up OK? >Not sure what do you mean by this, but it returns false love ke the previous version, right?> > diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h > > index 69fac2a..21d09bf 100644 > > --- a/copy/nbdcopy.h > > +++ b/copy/nbdcopy.h > > @@ -134,7 +134,8 @@ struct rw_ops { > > bool (*synch_trim) (struct rw *rw, uint64_t offset, uint64_t count); > > > > /* Synchronously zero. If not possible, returns false. */ > > - bool (*synch_zero) (struct rw *rw, uint64_t offset, uint64_t count); > > + bool (*synch_zero) (struct rw *rw, uint64_t offset, uint64_t count, > > + bool allocate); > > After this change ops->synch_trim and ops->asynch_trim are no longer > used, so I guess they should be removed completely from the code? >Yes, unless we have a reason to keep the trim option.> Rich. > > -- > Richard Jones, Virtualization Group, Red Hat > http://people.redhat.com/~rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > virt-builder quickly builds VMs from scratch > http://libguestfs.org/virt-builder.1.html > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://listman.redhat.com/archives/libguestfs/attachments/20210222/b13e44dd/attachment.htm>