[libvirt] [PATCH v2 0/3] Wrong allocation size displayed for NFS volumes

[John Ferlan]

ping?

Tks -

John

On 08/11/2014 04:30 PM, John Ferlan wrote:
> Changes over v1 - different tact as more research discovers that the
> posix_fallocate() does not perform the correct pre-allocation of space
> for an NFS backed file/disk, some details of the findings can be found:
> 
> http://www.redhat.com/archives/libvir-list/2014-August/msg00367.html
> 
> The first patch in this series will refactor the safezero to allow for
> more fallback than the current code. Initially implemented as a series
> of three 'safezero()' functions in commit id 'c29d0929' using build
> conditionals to determine which of the 3 is being used.
> 
> The refactored code will have one function that will make a series of
> calls rather the just failing on whatever is built into the system.
> The first is a global virFileFdPosixFallocate() with the build conditional 
> controlling only whether the function runs or will just return -1.  It
> will also be re-used by the Volume Resize code in a future patch.
> 
> If on creation the virFileFdPosixFallocate() fails, attempts will be made
> to then try the mmap() method (if it exists), and then fall back to the
> slow, old, safewrite of zero filled buffers. The mmap function will follow
> the format of the posix_fallocate insomuch as if HAVE_MMAP is defined,
> then that will be attempted or a -1 will be returned immediately.
> 
> The major change of this patch over the prior code is the fallback. One
> other minor change is if the virFileFdPosixFallocate() tries to call
> posix_fallocate() and fails, then a VIR_WARN will be used to indicate
> something went wrong. Previously, the code would return errno and eventually
> that would filter back to a caller which would use the errno in a sys
> error message. This I think only is odd when it comes to the resize path.
> 
> The second patch looks to resolve the first issue discovered by:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1077068
> 
> It does this by checking the returned size/len of the file in the
> posix_fallocate() call and if it does not match a VIR_WARN will be
> posted and a failure returned allowing either the mmap or write of
> zero filled buffers to the file.
> 
> The third patch changes the virStorageFileResize() logic to follow
> the safezero() logic with respect to build conditionals. Also, rather
> than reinvent the wheel, it will reuse the virFileFdPosixFallocate().
> The new static API resizeSysFallocate() will return -1 if the build
> conditionals are not met.
> 
> A current "downside" is by calling virFileFdPosixFallocate() twice
> (once directly and the other indirectly through safezero), the
> VIR_WARN will be displayed twice, but that should only affect someone
> trying to debug things.
> 
> 
> NOTE:
> Investigation and testing while creating this series also showed it's
> possible to modify the NFS Pool Start code to add a "-o wsize=4096" to
> the MOUNT command in order to force smaller write's.  While that did fix
> the symptoms, the fix just didn't "feel right". The block size of the
> "source" export was 4096 bytes, while the block size of the "target"
> file was 1048576 bytes (current NFSv4 wsize default value when not
> specified). Whether those play into what posix_fallocate() does I
> am not sure, but if it does and that's fixed, then the way these
> patches are coded up - it won't matter. Compared to perhaps needing
> to revisit or document heavily how to set some new field in the pool
> XML source - this just seems more right.
> 
> John Ferlan (3):
>   virfile: Refactor safezero, introduce virFileFdPosixFallocate
>   virfile: Check returned size from virFileFdPosixFallocate
>   virstoragefile: Refactor virStorageFileResize
> 
>  src/libvirt_private.syms  |  1 +
>  src/util/virfile.c        | 58 ++++++++++++++++++++++++++++++++++++++---------
>  src/util/virfile.h        |  2 ++
>  src/util/virstoragefile.c | 52 +++++++++++++++++++++++++++---------------
>  4 files changed, 84 insertions(+), 29 deletions(-)
>