[libvirt] [PATCH 3/4] vol-info: Check for NFS FS type

[John Ferlan]

<...snip...>

>> Why all this happens I'm not sure.  Bug in posix_fallocate()? Bug in
>> configuration?  I have to assume that when this code was first added NFS
>> probably was still using smaller block sizes.
> 
> The code was introduced in 2013. Maybe it wasn't tested on NFS at all?
> It looks like a posix_fallocate bug to me.
> 

Technically it was before that - the safezero function and it's
supporting cast were moved into virfile.c in 2013; however, they were in
[vir]util.c before that. It was first introduced in 2009 in commit id
'c29d0929' and beyond some minor changes/reword has stayed relatively
unchanged.

The one downside I see with it is that it's a build related either/or
setup. That is the "HAVE_POSIX_FALLOCATE" dictates the usage and
provides no fallback in the event that perhaps the result isn't expected
or if for some reason posix_fallocate fails.

"Theoretically" the safewrite option should/could be the fallback in the
event it's determined that the posix_fallocate() didn't generate the
right allocation size.

> The other method we have (syscall(SYS_fallocate)) gives me EOPNOTSUPP -
> Operation not supported on transport endpoint.
> 

Right - I saw that too.... The resize code followed the safezero() code
at least with respect to the either/or with the HAVE_POSIX_FALLOCATE
build conditional. It was introduced in commit id 'aa2a4cff'.  It
assumes that the "SYS_fallocate" syscall is present "and" will work. It
is missing an option to safewrite() from current allocation to the new
end of the file (eg, a call to safezero passing the the current offset).

>> Whether anyone has
>> noticed or not beyond the virt-test which discovered the issue - I'm not
>> sure.  In any case, does anyone have feedback/thoughts for next steps?
>> I can put together something that avoids posix_fallocate() for the
>> create-as and resize paths.
>>
> 
> I think reporting an error when preallocate is requested for a NFS pool makes
> sense, but that might be pretty annoying if something supplies the preallocate
> flag by default.
> 

My comment was more geared towards my findings of the posix_fallocate()
introduction in 2009.

While I agree there's something flaky with posix_fallocate, I don't
think an error is necessarily the best path. Maybe better "fall back
code" - we could check the posix_fallocate() results (using stat)
perhaps VIR_WARN that something is wrong before falling back to the
mmap/safewrite options.

John