<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jul 30, 2018 at 6:01 PM Eric Blake <<a href="mailto:eblake@redhat.com">eblake@redhat.com</a>> wrote:<br></div><div>... </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> @@ -42,6 +43,8 @@<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
>   #include <sys/stat.h><br>
>   #include <errno.h><br>
>   #include <linux/falloc.h>   /* For FALLOC_FL_* on RHEL, glibc < 2.18 */<br>
> +#include <sys/ioctl.h><br>
> +#include <linux/fs.h><br>
<br>
Does this need a configure-time probe to see if it exists, since it will <br>
break compilation on BSD systems?  Same question to linux/falloc.h. <br>
Actually, linux/falloc.h doesn't see any use in the current nbdkit.git; <br>
does this email depend on another thread being applied first?<br></blockquote><div><br></div><div>Yes, this depends on </div><div><a href="https://www.redhat.com/archives/libguestfs/2018-July/msg00083.html">https://www.redhat.com/archives/libguestfs/2018-July/msg00083.html</a> </div><div><br></div><div>I plan to protect both imports with #if defined(__linux__). Any reason to</div><div>use configure instead?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> +#ifdef FALLOC_FL_PUNCH_HOLE<br>
> +  /* If we can punch hole but may not trim, we can combine punching hole and<br>
> +     fallocate to zero a range. This is much more efficient than writing zeros<br>
> +     manually. */<br>
<br>
s/is/can be/ (it's two syscalls instead of one, and may not be as <br>
efficient as we'd like - but does indeed stand a chance of being more <br>
efficient than manual efforts)<br></blockquote><div><br></div><div>"can be" is better, but I really mean "is typically", or "is expected to be".</div><div><br></div><div>For example in imageio same change improved upload throughout by</div><div>450% with my poor NFS server, see:</div><div><a href="https://gerrit.ovirt.org/c/92871/11//COMMIT_MSG">https://gerrit.ovirt.org/c/92871/11//COMMIT_MSG</a></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +  if (h->can_punch_hole && h->can_fallocate) {<br>
> +    r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,<br>
> +                      offset, count);<br>
> +    if (r == 0) {<br>
> +      r = do_fallocate(h->fd, 0, offset, count);<br>
> +      if (r == 0)<br>
> +        return 0;<br>
> +<br>
> +      if (errno != EOPNOTSUPP) {<br>
> +        nbdkit_error ("zero: %m");<br>
> +        return r;<br>
> +      }<br>
> +<br>
> +      h->can_fallocate = false;<br>
> +    } else {<br>
> +      if (errno != EOPNOTSUPP) {<br>
> +        nbdkit_error ("zero: %m");<br>
> +        return r;<br>
> +      }<br>
> +<br>
> +      h->can_punch_hole = false;<br>
> +    }<br>
> +  }<br>
> +#endif<br>
> +<br>
> +  /* For block devices, we can use BLKZEROOUT.<br>
> +     NOTE: count and offset must be aligned to logical block size. */<br>
> +  if (h->is_block_device) {<br>
> +    uint64_t range[2] = {offset, count};<br>
<br>
Is it worth attempting the ioctl only when you have aligned values?<br></blockquote><div><br></div><div>I think it does, but this requires getting the logical sector size</div><div>and keeping it in the handle.</div><div><br></div><div>Looking in plugin_zero, if we find that the offset and count are not</div><div>aligned and return EOPNOTSUPP, we will fallback to manual</div><div>zeroing for this call.</div><div><br></div><div>But this worries me:</div><div><br></div><div>549   int can_zero = 1; /* TODO cache this per-connection? */</div><div><br></div><div>Once can_zero is cached per connection, failing once because of</div><div>single unaligned call will prevent efficient zero for the rest of the image.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> +    r = ioctl(h->fd, BLKZEROOUT, &range);<br>
<br>
This portion of the code be conditional on whether BLKZEROOUT is defined.<br></blockquote><div><br></div><div>Right.</div><div>...</div><div><br></div><div>Nir</div></div></div>