[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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
trim when it does nothing.

Do you think it is better to leave the file_can_trim as is, to avoid

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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]