<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Aug 2, 2018 at 10:39 PM Eric Blake <<a href="mailto:eblake@redhat.com">eblake@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 08/02/2018 02:05 PM, Nir Soffer wrote:<br>
> fallocate(FALLOC_FL_ZERO_RANGE) is supportd for block devices with<br>
> modern kernel, but when it is not, we fall back to manual zeroing.<br>
> <br>
> Check if the underlying file is a block device when opening the file,<br>
> and fall back to ioctl(BLKZEROOUT) for aligned zero requests for a<br>
> block device.<br>
> <br>
> +++ b/plugins/file/file.c<br>
> @@ -45,6 +45,7 @@<br>
>   <br>
>   #if defined(__linux__) && !defined(FALLOC_FL_PUNCH_HOLE)<br>
>   #include <linux/falloc.h>   /* For FALLOC_FL_*, glibc < 2.18 */<br>
> +#include <linux/fs.h>       /* For BLKZEROOUT */<br>
<br>
Will this pick up BLKZEROOUT in all cases where it is needed?  Or do we <br>
need to relax the !defined(FALLOC_FL_PUNCH_HOLE), and just blindly <br>
include both of these headers for all Linux compilations?<br></blockquote><div><br></div><div>It works on RHEL 7.5, but it should not depend on FALLOC_FL_*.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +static bool<br>
> +is_aligned(struct handle *h, uint64_t n)<br>
> +{<br>
> +  return n % h->sector_size == 0;<br>
<br>
Since we know (but the compiler doesn't) that sector_size is a power of <br>
2, it is slightly faster to use bitwise math:<br>
  return !(n & (h->sector_size - 1))<br></blockquote><div><br></div><div>Right</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +#ifdef BLKSSZGET<br>
> +  if (h->is_block_device) {<br>
> +    if (ioctl (h->fd, BLKSSZGET, &h->sector_size)) {<br>
> +      nbdkit_error ("ioctl(BLKSSZGET): %s: %m", filename);<br>
> +      free (h);<br>
> +      return NULL;<br>
<br>
If the ioctl() fails, would it be better to just fall back...<br>
<br>
> +    }<br>
> +  }<br>
> +#else<br>
> +  h->sector_size = 4096;  /* Safe guess */<br>
<br>
...to the safe guess, instead of giving up entirely?  (Might matter on a <br>
system with newer headers that have the macro, but where the kernel does <br>
not support the ioctl).<br></blockquote><div><br></div><div>Good idea.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> @@ -329,6 +361,20 @@ file_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)<br>
>     }<br>
>   #endif<br>
>   <br>
> +#ifdef BLKZEROOUT<br>
> +  /* For aligned range and block devices, we can use BLKZEROOUT. */<br>
> +  if (h->is_block_device && is_aligned (h, offset) && is_aligned (h, count)) {<br>
<br>
Since alignment is a power of 2, you can compress this as:<br>
<br>
if (h->is_block_device && is_aligned (h, offset | count)) {<br></blockquote><div><br></div><div>Clever!</div><div><br></div><div>Richard, do you like to maintain bitwise tricks like this?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +    uint64_t range[2] = {offset, count};<br>
> +<br>
> +    r = ioctl (h->fd, BLKZEROOUT, &range);<br>
> +    if (r == 0)<br>
> +      return r;<br>
> +<br>
> +    nbdkit_error ("zero: %m");<br>
> +    return r;<br>
<br>
Are we sure that treating ALL errors as fatal is worthwhile, or should <br>
we still attempt to trigger a fall back to writing?<br></blockquote><div><br></div><div>I think we can assume that all block devices support BLKZEROOUT.</div><div>This is what oVirt does.</div><div><br></div><div>Since nbdkit is more general purpose, maybe it is better to fallback to </div><div>manual zero.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +  }<br>
> +#endif<br>
> +<br>
>     /* Trigger a fall back to writing */<br>
>     errno = EOPNOTSUPP;<br>
>     return r;<br>
> <br>
<br>
-- <br>
Eric Blake, Principal Software Engineer<br>
Red Hat, Inc.           <a href="tel:(919)%20301-3266" value="+19193013266" target="_blank">+1-919-301-3266</a><br>
Virtualization:  <a href="http://qemu.org" rel="noreferrer" target="_blank">qemu.org</a> | <a href="http://libvirt.org" rel="noreferrer" target="_blank">libvirt.org</a><br>
</blockquote></div></div>