[PATCH] um: ubd: perform the flush the block layer asks for

[Mykyta Bozhenko]

ubd sets BLK_FEAT_WRITE_CACHE, so the block layer sends it REQ_OP_FLUSH
requests and, as Documentation/block/writeback_cache_control.rst puts it,
the driver "needs to handle them". do_io() does implement that: for
REQ_OP_FLUSH it calls os_sync_file() on the backing file and maps the
result back into the request.

That branch has been unreachable since commit fc6b6a872dcd ("um: ubd:
Submit all data segments atomically"), which replaced the single
per-request do_io() call with a loop over the request's data
descriptors:

	-			do_io((*io_req_buffer)[count]);
	+			for (i = 0; !req->error && i < req->desc_cnt; i++)
	+				do_io(req, &(req->io_desc[i]));

A flush carries no data and ubd_submit_request() sets desc_cnt to 0 for
it, so the loop body never runs. The request is handed back to the block
layer with error 0, i.e. the flush is reported as completed without the
backing file ever being synced. Guest fsync(), fdatasync() and journal
commits return success while the data is only in the host's page cache,
and because no ordering is enforced either, a host crash can leave the
image in a state the guest never allowed. The error path is dead too:
a failing host fdatasync() cannot be reported.

Measured on a UML guest with ext4 on ubda, doing 20 writes of 4 KiB each
followed by fdatasync(), then one fsync() and one directory fsync():

  before: the guest sees 22 successful flushes, /sys/block/ubda/stat
          reports 16 completed flush requests, and the UML process issues
          no fdatasync() on the image at all
  after:  the same workload results in 43 fdatasync() calls on the image

os_pwrite_file() is entered 131 times either way, and e2fsck on the
resulting image is clean in both cases.

Honouring the flush costs what the flush costs. With the image on host
ext4, 400 iterations of write() plus fdatasync() in the guest take 467 ms
before and 2007 ms after (medians of five runs). uretprobes on
os_sync_file() attribute 1.597 s of that difference to time spent inside
the host's fdatasync(), the remaining data path being unchanged
(os_pwrite_file(): 19246 calls in both). A 64 MiB sequential write
followed by a single fsync() goes from 278 ms to 362 ms, due to the
periodic journal commits. With the image on tmpfs there is no measurable
difference.

Users who prefer the previous speed to durability can disable the cache
per device:

  echo "write through" > /sys/block/ubda/queue/write_cache

That is also cheaper than the unfixed driver, 326 ms for the same 400
iterations, because the block layer then completes empty flush requests
without entering the driver at all.

Fixes: fc6b6a872dcd ("um: ubd: Submit all data segments atomically")
Cc: stable@xxxxxxxxxxxxxxx
Assisted-by: LLM bpftrace
Signed-off-by: Mykyta Bozhenko <caudadragonis@xxxxxxxxx>
---
Not verified, stated explicitly as the process asks: the change was built and
exercised for ARCH=um only, on torvalds/master at cba2348ab and on v6.19; no
other architecture goes through this path. The flush error path, map_error() on
a failing host fdatasync(), is reachable again but I could not isolate it: the
block layer fault injection I used fails the data write too, so the EIO the
guest observes cannot be attributed to the flush alone.

The numbers above come from a single-CPU guest with 10 ms timer granularity,
which is why they are medians of five runs rather than percentages. Host side
counts are from uprobes on os_sync_file() and os_pwrite_file() and from the
sys_enter_fdatasync tracepoint filtered to the UML process; the guest side flush
count is field 11 of /sys/block/ubda/stat.
 arch/um/drivers/ubd_kern.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 20fc333..bb1d990 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -1516,6 +1516,18 @@ void *io_thread(void *arg)
 			int i;
 
 			io_count++;
+
+			/*
+			 * A flush request carries no data descriptors, so the
+			 * loop below would never call do_io() for it and the
+			 * flush would be reported as completed without the
+			 * backing file ever being synced.
+			 */
+			if (req_op(req->req) == REQ_OP_FLUSH) {
+				do_io(req, NULL);
+				continue;
+			}
+
 			for (i = 0; !req->error && i < req->desc_cnt; i++)
 				do_io(req, &(req->io_desc[i]));
 
-- 
2.43.0