Re: [Libguestfs] [PATCH 1/3] file: Avoid unsupported fallocate() calls
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