[nbdkit PATCH] readahead: Fix multi-conn inconsistency
The readahead filter was blindly passing through the plugin's .can_multi_conn, but does not kill the readahead buffer on a flush. This is a bug in the following scenario, when the plugin advertised multi-conn: connA connB write wipes our buffer plugin caches the write read reads stale data into buffer flush plugin finally flushes the write read data in buffer is still stale In order to preserve the plugin's multi-conn status, we MUST drop our cache any time we flush, so that our next read picks up whatever got flushed from another connection. But when the server lacks multi-conn, we don't have to kill our readahead buffer on flush, because the client already has no guarantee that a flush from one connection will affect the read seen by another (killing on write was already good enough to maintain semantics). --- filters/readahead/readahead.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/filters/readahead/readahead.c b/filters/readahead/readahead.c index 8163c4fb..a97c5da5 100644 --- a/filters/readahead/readahead.c +++ b/filters/readahead/readahead.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2019 Red Hat Inc. + * Copyright (C) 2019-2021 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -244,6 +244,15 @@ readahead_zero (struct nbdkit_next_ops *next_ops, void *nxdata, return next_ops->zero (nxdata, count, offset, flags, err); } +static int +readahead_flush (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, uint32_t flags, int *err) +{ + if (next_ops->can_multi_conn(nxdata) != 1) + kill_readahead (); + return next_ops->flush (nxdata, flags, err); +} + static struct nbdkit_filter filter = { .name = "readahead", .longname = "nbdkit readahead filter", @@ -255,6 +264,7 @@ static struct nbdkit_filter filter = { .pwrite = readahead_pwrite, .trim = readahead_trim, .zero = readahead_zero, + .flush = readahead_flush, }; NBDKIT_REGISTER_FILTER(filter) -- 2.30.1
On 2/19/21 3:03 PM, Eric Blake wrote:
The readahead filter was blindly passing through the plugin's .can_multi_conn, but does not kill the readahead buffer on a flush. This is a bug in the following scenario, when the plugin advertised multi-conn:
In order to preserve the plugin's multi-conn status, we MUST drop our cache any time we flush, so that our next read picks up whatever got flushed from another connection.
But when the server lacks multi-conn, we don't have to kill our readahead buffer on flush, because the client already has no guarantee that a flush from one connection will affect the read seen by another (killing on write was already good enough to maintain semantics). ---
+static int +readahead_flush (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, uint32_t flags, int *err) +{ + if (next_ops->can_multi_conn(nxdata) != 1)
That should be ==, to match my commit message.
+ kill_readahead (); + return next_ops->flush (nxdata, flags, err); +} +
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 2/19/21 3:18 PM, Eric Blake wrote:
On 2/19/21 3:03 PM, Eric Blake wrote:
The readahead filter was blindly passing through the plugin's .can_multi_conn, but does not kill the readahead buffer on a flush. This is a bug in the following scenario, when the plugin advertised multi-conn:
In order to preserve the plugin's multi-conn status, we MUST drop our cache any time we flush, so that our next read picks up whatever got flushed from another connection.
But when the server lacks multi-conn, we don't have to kill our readahead buffer on flush, because the client already has no guarantee that a flush from one connection will affect the read seen by another (killing on write was already good enough to maintain semantics). ---
+static int +readahead_flush (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, uint32_t flags, int *err) +{ + if (next_ops->can_multi_conn(nxdata) != 1)
That should be ==, to match my commit message.
+ kill_readahead (); + return next_ops->flush (nxdata, flags, err); +} +
In fact, the more I think about it, the more I want it to be unconditional, killing readahead even when not multi-conn. Why? Because when multi-conn is not advertised, it seems odd that there is no way at all to force the NBD server to flush its caches; it seems like clients should be able to expect a flush on connA (to write pending data to cache), then a flush on connB (to drop any readahead data), prior to a read on connB, should let connB see what connA wrote. All that multi-conn buys you is that you can skip one of the two flush calls. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Feb 19, 2021 at 04:02:48PM -0600, Eric Blake wrote:
On 2/19/21 3:18 PM, Eric Blake wrote:
On 2/19/21 3:03 PM, Eric Blake wrote:
The readahead filter was blindly passing through the plugin's .can_multi_conn, but does not kill the readahead buffer on a flush. This is a bug in the following scenario, when the plugin advertised multi-conn:
In order to preserve the plugin's multi-conn status, we MUST drop our cache any time we flush, so that our next read picks up whatever got flushed from another connection.
But when the server lacks multi-conn, we don't have to kill our readahead buffer on flush, because the client already has no guarantee that a flush from one connection will affect the read seen by another (killing on write was already good enough to maintain semantics). ---
+static int +readahead_flush (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle, uint32_t flags, int *err) +{ + if (next_ops->can_multi_conn(nxdata) != 1)
That should be ==, to match my commit message.
+ kill_readahead (); + return next_ops->flush (nxdata, flags, err); +} +
In fact, the more I think about it, the more I want it to be unconditional, killing readahead even when not multi-conn. Why? Because when multi-conn is not advertised, it seems odd that there is no way at all to force the NBD server to flush its caches; it seems like clients should be able to expect a flush on connA (to write pending data to cache), then a flush on connB (to drop any readahead data), prior to a read on connB, should let connB see what connA wrote. All that multi-conn buys you is that you can skip one of the two flush calls.
I see the modified version of this patch went upstream - thanks! The readahead filter is fairly broken at the moment, and we stopped using it in virt-v2v. There are several causes of the brokenness: - Not having the ability of using a background thread. (See TODO) - Readahead happens synchronously with the request which makes the requests run slower. - Readahead causes too large plugin requests which breaks VDDK. Even though strictly this is yet another VDDK-is-crap bug, it was the final straw that made us drop the readahead filter. - Very naive/stupid readahead policy. - Related to previous point and to your message: The naive policy is practically incompatible with multi-conn, because if two threads are reading different parts of the disk then the readahead buffer will be dropped between the requests. So I believe we should also set can_multi_conn = 0 in this filter. It might also be a good idea to drop the filter until such time as we have the background threads feature in the server and can do it properly. But at the moment this filter has a very narrow and limited usefulness if you know what you're doing. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
participants (2)
-
Eric Blake -
Richard W.M. Jones