[libvirt] [PATCH v2 3/3] virstoragefile: Refactor virStorageFileResize

[John Ferlan]

On 08/21/2014 11:53 AM, Ján Tomko wrote:
> On 08/11/2014 10:30 PM, John Ferlan wrote:
>> Currently virStorageFileResize() function uses build conditionals to
>> choose either the posix_fallocate() or mmap() with no fallback in order
>> to preallocate the space in the newly resized file.
>>
>> This patch will modify the logic in order to allow fallbacks in the event
>> that posix_fallocate() or the sys_fallocate syscall() doesn't work properly.
>> The fallback will be to use the slow safewrite of zero filled buffers to
>> the file.
>>
>> Use the virFileFdPosixFallocate() rather than a local function.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/util/virstoragefile.c | 52 +++++++++++++++++++++++++++++++----------------
>>  1 file changed, 34 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 5b6b2f5..4d37de1 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -1086,6 +1086,39 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain,
>>      return 0;
>>  }
>>  
>> +static int
>> +resizeSysFallocate(const char *path,
>> +                   int fd,
>> +                   off_t offset,
>> +                   off_t len)
>> +{
>> +    int rc = -1;
>> +#if HAVE_SYS_SYSCALL_H && defined(SYS_fallocate)
>> +    if ((rc = syscall(SYS_fallocate, fd, 0, offset, len)) != 0) {
>> +        virReportSystemError(errno,
>> +                             _("Failed to pre-allocate space for "
>> +                             "file '%s'"), path);
> 
> I think this should not log an error, since we have a fallback.
> VIR_DEBUG maybe?
> 

Yep - right.  Although to match the other path, VIR_WARN...

>> +    }
>> +#endif
>> +    return rc;
>> +}
>> +
>> +static int
>> +doResize(const char *path,
>> +         int fd,
>> +         off_t offset,
>> +         off_t len)
>> +{
>> +    if (virFileFdPosixFallocate(fd, offset, len) == 0 ||
>> +        resizeSysFallocate(path, fd, offset, len) == 0 ||
>> +        safezero(fd, offset, len) == 0)
>> +        return 0;
>> +
>> +    virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>> +                   _("preallocate is not supported on this platform"));
>> +    return -1;
> 
> safezero is always supported. And this function should do:
> return safezero(fd, offset, len);
> 
> 

Hmm.. Perhaps a better way to do this would be to modify safezero() to
add a 4th boolean parameter "resize" and make the "safezero_mmap()" be
the false side and the check/call to SYS_fallocate() be the true side.
That way all the logic resides in virfile.c

Thus doResize() goes away and resizeSysFallocate() moves to virfile.c.
All virStorageFileResize() does for the "if (preAllocate)" condition "if
(safezero(...,true) < 0)"

See the attached patch file that "should" be able to be "git am"'d on
top of the existing patches (although I did rebase to top to tree this
morning).

Does that seem better?

John



>> +}
>> +
>>  
>>  /**
>>   * virStorageFileResize:
>> @@ -1113,25 +1146,8 @@ virStorageFileResize(const char *path,
>>      }
>>  
>>      if (pre_allocate) {
>> -#if HAVE_POSIX_FALLOCATE
>> -        if ((rc = posix_fallocate(fd, offset, len)) != 0) {
>> -            virReportSystemError(rc,
>> -                                 _("Failed to pre-allocate space for "
>> -                                   "file '%s'"), path);
>> -            goto cleanup;
>> -        }
>> -#elif HAVE_SYS_SYSCALL_H && defined(SYS_fallocate)
>> -        if (syscall(SYS_fallocate, fd, 0, offset, len) != 0) {
>> -            virReportSystemError(errno,
>> -                                 _("Failed to pre-allocate space for "
>> -                                   "file '%s'"), path);
>> +        if (doResize(path, fd, offset, len) < 0)
>>              goto cleanup;
>> -        }
>> -#else
>> -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>> -                       _("preallocate is not supported on this platform"));
>> -        goto cleanup;
>> -#endif
>>      } else {
>>          if (ftruncate(fd, capacity) < 0) {
>>              virReportSystemError(errno,
>>
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Rework-virStorageFileResize.patch
Type: text/x-patch
Size: 5918 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140821/55367dea/attachment-0001.bin>