First part of VirtualFile async conversions #5180

arpad-m · 2023-09-01T12:30:31Z

Problem

VirtualFile does both reading and writing, and it would be nice if both could be converted to async, so that it doesn't have to support an async read path and a blocking write path (especially for the locks this is annoying as none of the lock implementations in std, tokio or parking_lot have support for both async and blocking access).

Summary of changes

This PR is some initial work on making the VirtualFile APIs async. It can be reviewed commit-by-commit.

Introduce the MaybeVirtualFile enum to be generic in a test that compares real files with virtual files.
Make various APIs of VirtualFile async, including write_all_at, read_at, read_exact_at
Prepare for making the write plus fsync pattern we have in two locations async, but don't do it yet due to data being held that is not Send.

Part of #4743

Checklist before requesting a review

I have performed a self-review of my code.
If it is a core feature, I have added thorough tests.
Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

Do not forget to reformat commit message to not include the above checklist

…o async API

github-actions · 2023-09-01T12:36:26Z

No tests were run or test report is not available

_{The comment gets automatically updated with the latest test results
d2314af at 2023-09-01T12:36:25.604Z :recycle:}

problame · 2023-09-01T13:21:07Z

pageserver/src/tenant.rs

+        let do_persist = |target_config_path: PathBuf| -> _ {
+            async move {
+                let target_config_parent = target_config_path.parent().with_context(|| {


Can probably save ourselves one level of indent here?

Suggested change

let do_persist = |target_config_path: PathBuf| -> _ {

async move {

let target_config_parent = target_config_path.parent().with_context(|| {

let do_persist = |target_config_path: PathBuf| async move {

let target_config_parent = target_config_path.parent().with_context(|| {

problame · 2023-09-01T13:34:30Z

pageserver/src/tenant/block_io.rs

-    F: FileExt,
-{
-    pub fn new(file: F) -> Self {
+impl FileBlockReader<VirtualFile> {


Extracted this stuff into #5181

problame · 2023-09-01T13:38:13Z

pageserver/src/virtual_file.rs

+    /// Write the given buffer (which has to be below the kernel's internal page size) and fsync
+    ///
+    /// This ensures some level of atomicity (not a good one, but it's the best we have).
+    pub fn write_and_fsync(&mut self, buf: &[u8]) -> Result<(), Error> {
+        if self.write(buf)? != buf.len() {
+            return Err(Error::new(
+                std::io::ErrorKind::Other,
+                "Could not write all the bytes in a single call",
+            ));
+        }
+        self.sync_all()?;
+        Ok(())
+    }


Would prefer to back out the addition of this function. It's not what I had in mind with

special-purpose api for write-once files, to be used in save_metadata and persist_tenant_conf

#4743 (comment)

Will push a commit that removes this API, then create a PR that adds the API I had in mind.

problame

See my inline comments

part of #4743 preliminary to #5180

@arpad-m

(part of #4743) (preliminary to #5180) This PR adds a special-purpose API to `VirtualFile` for write-once files. It adopts it for `save_metadata` and `persist_tenant_conf`. This is helpful for the asyncification efforts (#4743) and specifically asyncification of `VirtualFile` because above two functions were the only ones that needed the VirtualFile to be an `std::io::Write`. (There was also `manifest.rs` that needed the `std::io::Write`, but, it isn't used right now, and likely won't be used because we're taking a different route for crash consistency, see #5172. So, let's remove it. It'll be in Git history if we need to re-introduce it when picking up the compaction work again; that's why it was introduced in the first place). We can't remove the `impl std::io::Write for VirtualFile` just yet because of the `BufWriter` in ```rust struct DeltaLayerWriterInner { ... blob_writer: WriteBlobWriter<BufWriter<VirtualFile>>, } ``` But, @arpad-m and I have a plan to get rid of that by extracting the append-only-ness-on-top-of-VirtualFile that #4994 added to `EphemeralFile` into an abstraction that can be re-used in the `DeltaLayerWriterInner` and `ImageLayerWriterInner`. That'll be another PR. ### Performance Impact This PR adds more fsyncs compared to before because we fsync the parent directory every time. 1. For `save_metadata`, the additional fsyncs are unnecessary because we know that `metadata` fits into a kernel page, and hence the write won't be torn on the way into the kernel. However, the `metadata` file in general is going to lose signficance very soon (=> see #5172), and the NVMes in prod can definitely handle the additional fsync. So, let's not worry about it. 2. For `persist_tenant_conf`, which we don't check to fit into a single kernel page, this PR makes it actually crash-consistent. Before, we could crash while writing out the tenant conf, leaving a prefix of the tenant conf on disk.

arpad-m · 2023-09-02T09:00:40Z

This has been split up into #5181, #5186, #5189, and #5190.

part of #4743 preliminary to #5180

## Problem `VirtualFile` does both reading and writing, and it would be nice if both could be converted to async, so that it doesn't have to support an async read path and a blocking write path (especially for the locks this is annoying as none of the lock implementations in std, tokio or parking_lot have support for both async and blocking access). ## Summary of changes This PR is some initial work on making the `VirtualFile` APIs async. It can be reviewed commit-by-commit. * Introduce the `MaybeVirtualFile` enum to be generic in a test that compares real files with virtual files. * Make various APIs of `VirtualFile` async, including `write_all_at`, `read_at`, `read_exact_at`. Part of #4743 , successor of #5180. Co-authored-by: Christian Schwarz <me@cschwarz.com>

arpad-m and others added 16 commits September 1, 2023 13:51

Move VirtualFile::seek to inherent function

17b3f4b

FileBlockReader<File> is never used

353ebef

Move used FileExt functions to inherent impls

69d046f

Remove Read impl that was only used in one place

67f7e6f

FileBlockReader::fill_buffer make it obvious that we need to switch t…

3413937

…o async API

Make write_all_at async

486a7f6

Remove bounds

afbebce

Add write_and_fsync function

8ef27a3

Make save_metadata async fn

9cffba2

Use write_and_fsync in save_metadata

df28e7a

Add MaybeVirtualFile and use it in tests

dc6668d

Make read_exact_at async fn

167342d

Make read_at async fn

a13b0cc

Use write_and_fsync in persist_tenant_config

aa4763b

Make tenant_map_insert and its closure async fn

0aed765

Make persist_tenant_config async fn

d2314af

arpad-m requested review from a team as code owners September 1, 2023 12:30

arpad-m requested review from awestover, koivunej and problame and removed request for a team September 1, 2023 12:30

problame reviewed Sep 1, 2023

View reviewed changes

problame mentioned this pull request Sep 1, 2023

FileBlockReader<File> is never used #5181

Merged

problame reviewed Sep 1, 2023

View reviewed changes

arpad-m mentioned this pull request Sep 1, 2023

Epic: convert remaining IO stack for Timeline::get to async fn #4743

Closed

problame added a commit that referenced this pull request Sep 1, 2023

FileBlockReader<File> is never used (#5181)

aa22000

part of #4743 preliminary to #5180

problame mentioned this pull request Sep 1, 2023

VirtualFile: add crash-safe overwrite abstraction & use it #5186

Merged

arpad-m mentioned this pull request Sep 2, 2023

Convert many VirtualFile APIs to async #5190

Merged

5 tasks

arpad-m closed this Sep 2, 2023

jcsp pushed a commit that referenced this pull request Sep 4, 2023

FileBlockReader<File> is never used (#5181)

6455f0d

part of #4743 preliminary to #5180

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First part of VirtualFile async conversions #5180

First part of VirtualFile async conversions #5180

arpad-m commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023

problame Sep 1, 2023

problame Sep 1, 2023

problame Sep 1, 2023

problame left a comment

arpad-m commented Sep 2, 2023

First part of VirtualFile async conversions #5180

First part of VirtualFile async conversions #5180

Conversation

arpad-m commented Sep 1, 2023 • edited Loading

Problem

Summary of changes

Checklist before requesting a review

Checklist before merging

github-actions bot commented Sep 1, 2023

No tests were run or test report is not available

problame Sep 1, 2023

Choose a reason for hiding this comment

problame Sep 1, 2023

Choose a reason for hiding this comment

problame Sep 1, 2023

Choose a reason for hiding this comment

problame left a comment

Choose a reason for hiding this comment

arpad-m commented Sep 2, 2023

arpad-m commented Sep 1, 2023 •

edited

Loading