[Libguestfs] [PATCH 1/3] file: Avoid unsupported fallocate() calls

[Eric Blake]

On 08/02/2018 03:00 PM, Nir Soffer wrote:
> Sure disabling trim does not help the client. The only advantage is not
> calling
> trim when it does nothing.
> 
> Do you think it is better to leave the file_can_trim as is, to avoid
> confusion?

I'm not yet sold that making it dynamic helps anything, so leaving it 
hard-coded to the presence of the macros (whether fallocate() works) is 
certainly the most conservative approach, regardless of whether it is 
the most sensible in the long run.

>>> @@ -118,6 +119,8 @@ file_config_complete (void)
>>>    /* The per-connection handle. */
>>>    struct handle {
>>>      int fd;
>>> +  bool can_punch_hole;
>>> +  bool can_zero_range;
>>
>> Would it be better to make these tri-state rather than merely bool?
>> (Indeterminate, supported, known to fail)
>>
> 
> What is the advantage of having tri-state?

If you know it has worked in the past (supported state), but the current 
fallocate() fails, then you can immediately report failure instead of 
triggering the fallback path. I'm not sure whether there are enough 
advantages to having tri-state, only pointing it out as a possible 
implementation (as I have seen code in gnulib where tri-state does save 
effort).


>>>    static int
>>>    file_can_trim (void *handle)
>>>    {
>>> -  /* Trim is advisory, but we prefer to advertise it only when we can
>>> -   * actually (attempt to) punch holes.  Since not all filesystems
>>> -   * support all fallocate modes, it would be nice if we had a way
>>> -   * from fpathconf() to definitively learn what will work on a given
>>> -   * fd for a more precise answer; oh well.  */
>>
>> The comment about fpathconf() would still be nice to keep in some form.
>>
> 
> But fpathconf does not tell anything abut this, no?

The point of the comment is that it SHOULD, but doesn't - ie. we have a 
kernel RFE, and if the kernel folks ever realize that they could help us 
out by adding such a conf query, we could refactor this code to be more 
efficient as a result.  Keeping the comment is thus an arsenal in 
proposing a kernel enhancement.

>> I'm a bit worried about whether changing the return value within the
>> context of a single connection is wise, or if we need to further
>> maintain a per-connection state that is initialized according to the
>> overall plugin state.
>>
> 
> It seems like we should keep the current behavior.
> 
> Richard, what do you think?
> 

>>> +  if (h->can_punch_hole) {
>>> +    r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
>>> +                      offset, count);
>>
>> Should we also use FALLOC_FL_NO_HIDE_STALE here, if it is available?
>>
> 
> We can add h->can_leave_stale, and use it if available. But I don't think
> it will give much with typical request size.

The difference is that (at least for modern kernel and block devices), 
NO_HIDE_STALE means to 'discard no matter what, even if I read back 
stale data instead of zeroes' while omitting it means 'discard, but only 
if you can guarantee reading back as zeros'. And some devices may 
implement both modes but with a faster behavior for pure discard than 
for read-back-as-zeroes.  Since the trim operation is documented as not 
guaranteeing what we read back, we might as well try the potentially 
faster operation first.

> 
> Do you think it worth the effort?

I'm not sure. Benchmarks and/or feedback from a kernel developer may 
prove useful.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org