diff --git a/.github/workflows/_build-and-test-locally.yml b/.github/workflows/_build-and-test-locally.yml index 8e280498883a..24f586714cba 100644 --- a/.github/workflows/_build-and-test-locally.yml +++ b/.github/workflows/_build-and-test-locally.yml @@ -222,7 +222,7 @@ jobs: # run pageserver tests with different settings for io_engine in std-fs tokio-epoll-uring ; do - NEON_PAGESERVER_UNIT_TEST_VIRTUAL_FILE_IOENGINE=$io_engine ${cov_prefix} cargo nextest run $CARGO_FLAGS $CARGO_FEATURES -E 'package(pageserver)' + NEON_PAGESERVER_UNIT_TEST_VIRTUAL_FILE_IOENGINE=$io_engine NEON_PAGESERVER_UNIT_TEST_VIRTUAL_FILE_IOMODE=direct ${cov_prefix} cargo nextest run $CARGO_FLAGS $CARGO_FEATURES -E 'package(pageserver)' done # Run separate tests for real S3 @@ -300,6 +300,7 @@ jobs: CHECK_ONDISK_DATA_COMPATIBILITY: nonempty BUILD_TAG: ${{ inputs.build-tag }} PAGESERVER_VIRTUAL_FILE_IO_ENGINE: tokio-epoll-uring + PAGESERVER_VIRTUAL_FILE_IO_MODE: direct # Temporary disable this step until we figure out why it's so flaky # Ref https://github.com/neondatabase/neon/issues/4540 diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index cc6f91d28e5e..5099801edfc2 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -334,6 +334,7 @@ jobs: PERF_TEST_RESULT_CONNSTR: "${{ secrets.PERF_TEST_RESULT_CONNSTR }}" TEST_RESULT_CONNSTR: "${{ secrets.REGRESS_TEST_RESULT_CONNSTR_NEW }}" PAGESERVER_VIRTUAL_FILE_IO_ENGINE: tokio-epoll-uring + PAGESERVER_VIRTUAL_FILE_IO_MODE: direct SYNC_BETWEEN_TESTS: true # XXX: no coverage data handling here, since benchmarks are run on release builds, # while coverage is currently collected for the debug ones diff --git a/libs/pageserver_api/src/config.rs b/libs/pageserver_api/src/config.rs index f48c1febb53c..bef06026012f 100644 --- a/libs/pageserver_api/src/config.rs +++ b/libs/pageserver_api/src/config.rs @@ -106,7 +106,7 @@ pub struct ConfigToml { pub timeline_offloading: bool, pub ephemeral_bytes_per_memory_kb: usize, pub l0_flush: Option, - pub virtual_file_io_mode: Option, + pub virtual_file_io_mode: Option, #[serde(skip_serializing_if = "Option::is_none")] pub no_sync: Option, } diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 0dfa1ba817ca..3b7ac1031950 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -976,8 +976,7 @@ pub mod virtual_file { Debug, )] #[strum(serialize_all = "kebab-case")] - #[repr(u8)] - pub enum IoMode { + pub enum IoModeKind { /// Uses buffered IO. Buffered, /// Uses direct IO, error out if the operation fails. @@ -985,22 +984,16 @@ pub mod virtual_file { Direct, } - impl IoMode { + impl IoModeKind { + // TODO(yuchen): change to [`Self::Direct`] once finish rollout. + #[cfg(target_os = "linux")] pub const fn preferred() -> Self { Self::Buffered } - } - - impl TryFrom for IoMode { - type Error = u8; - fn try_from(value: u8) -> Result { - Ok(match value { - v if v == (IoMode::Buffered as u8) => IoMode::Buffered, - #[cfg(target_os = "linux")] - v if v == (IoMode::Direct as u8) => IoMode::Direct, - x => return Err(x), - }) + #[cfg(target_os = "macos")] + pub const fn preferred() -> Self { + Self::Buffered } } } diff --git a/pageserver/client/src/mgmt_api.rs b/pageserver/client/src/mgmt_api.rs index 4d76c66905c4..eaa6b60ea48a 100644 --- a/pageserver/client/src/mgmt_api.rs +++ b/pageserver/client/src/mgmt_api.rs @@ -543,7 +543,7 @@ impl Client { /// Configs io mode at runtime. pub async fn put_io_mode( &self, - mode: &pageserver_api::models::virtual_file::IoMode, + mode: &pageserver_api::models::virtual_file::IoModeKind, ) -> Result<()> { let uri = format!("{}/v1/io_mode", self.mgmt_api_endpoint); self.request(Method::PUT, uri, mode) diff --git a/pageserver/ctl/src/layer_map_analyzer.rs b/pageserver/ctl/src/layer_map_analyzer.rs index 11b8e98f57d0..312979f9729b 100644 --- a/pageserver/ctl/src/layer_map_analyzer.rs +++ b/pageserver/ctl/src/layer_map_analyzer.rs @@ -7,7 +7,7 @@ use camino::{Utf8Path, Utf8PathBuf}; use pageserver::context::{DownloadBehavior, RequestContext}; use pageserver::task_mgr::TaskKind; use pageserver::tenant::{TENANTS_SEGMENT_NAME, TIMELINES_SEGMENT_NAME}; -use pageserver::virtual_file::api::IoMode; +use pageserver::virtual_file::api::IoModeKind; use std::cmp::Ordering; use std::collections::BinaryHeap; use std::ops::Range; @@ -137,7 +137,7 @@ pub(crate) async fn main(cmd: &AnalyzeLayerMapCmd) -> Result<()> { pageserver::virtual_file::init( 10, virtual_file::api::IoEngineKind::StdFs, - IoMode::preferred(), + IoModeKind::preferred(), ); pageserver::page_cache::init(100); diff --git a/pageserver/ctl/src/layers.rs b/pageserver/ctl/src/layers.rs index 6f543dcaa9ff..c6e14aac2e07 100644 --- a/pageserver/ctl/src/layers.rs +++ b/pageserver/ctl/src/layers.rs @@ -8,7 +8,7 @@ use pageserver::task_mgr::TaskKind; use pageserver::tenant::storage_layer::{delta_layer, image_layer}; use pageserver::tenant::storage_layer::{DeltaLayer, ImageLayer}; use pageserver::tenant::{TENANTS_SEGMENT_NAME, TIMELINES_SEGMENT_NAME}; -use pageserver::virtual_file::api::IoMode; +use pageserver::virtual_file::api::IoModeKind; use pageserver::{page_cache, virtual_file}; use std::fs::{self, File}; use utils::id::{TenantId, TimelineId}; @@ -50,7 +50,7 @@ async fn read_delta_file(path: impl AsRef, ctx: &RequestContext) -> Result virtual_file::init( 10, virtual_file::api::IoEngineKind::StdFs, - IoMode::preferred(), + IoModeKind::preferred(), ); page_cache::init(100); let path = Utf8Path::from_path(path.as_ref()).expect("non-Unicode path"); @@ -64,7 +64,7 @@ async fn read_image_file(path: impl AsRef, ctx: &RequestContext) -> Result virtual_file::init( 10, virtual_file::api::IoEngineKind::StdFs, - IoMode::preferred(), + IoModeKind::preferred(), ); page_cache::init(100); let path = Utf8Path::from_path(path.as_ref()).expect("non-Unicode path"); @@ -170,7 +170,7 @@ pub(crate) async fn main(cmd: &LayerCmd) -> Result<()> { pageserver::virtual_file::init( 10, virtual_file::api::IoEngineKind::StdFs, - IoMode::preferred(), + IoModeKind::preferred(), ); pageserver::page_cache::init(100); diff --git a/pageserver/ctl/src/main.rs b/pageserver/ctl/src/main.rs index f506caec5b06..671e24998f7c 100644 --- a/pageserver/ctl/src/main.rs +++ b/pageserver/ctl/src/main.rs @@ -24,7 +24,7 @@ use pageserver::{ page_cache, task_mgr::TaskKind, tenant::{dump_layerfile_from_path, metadata::TimelineMetadata}, - virtual_file::{self, api::IoMode}, + virtual_file::{self, api::IoModeKind}, }; use pageserver_api::shard::TenantShardId; use postgres_ffi::ControlFileData; @@ -208,7 +208,7 @@ async fn print_layerfile(path: &Utf8Path) -> anyhow::Result<()> { virtual_file::init( 10, virtual_file::api::IoEngineKind::StdFs, - IoMode::preferred(), + IoModeKind::preferred(), ); page_cache::init(100); let ctx = RequestContext::new(TaskKind::DebugTool, DownloadBehavior::Error); diff --git a/pageserver/pagebench/src/cmd/getpage_latest_lsn.rs b/pageserver/pagebench/src/cmd/getpage_latest_lsn.rs index b2df01714d31..9ad7cb95bbcf 100644 --- a/pageserver/pagebench/src/cmd/getpage_latest_lsn.rs +++ b/pageserver/pagebench/src/cmd/getpage_latest_lsn.rs @@ -61,7 +61,7 @@ pub(crate) struct Args { /// Before starting the benchmark, live-reconfigure the pageserver to use specified io mode (buffered vs. direct). #[clap(long)] - set_io_mode: Option, + set_io_mode: Option, targets: Option>, } diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index b694a435999f..e19bb8ef0d84 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -178,7 +178,7 @@ pub struct PageServerConf { pub l0_flush: crate::l0_flush::L0FlushConfig, /// Direct IO settings - pub virtual_file_io_mode: virtual_file::IoMode, + pub virtual_file_io_mode: virtual_file::IoModeKind, /// Optionally disable disk syncs (unsafe!) pub no_sync: bool, @@ -415,7 +415,8 @@ impl PageServerConf { l0_flush: l0_flush .map(crate::l0_flush::L0FlushConfig::from) .unwrap_or_default(), - virtual_file_io_mode: virtual_file_io_mode.unwrap_or(virtual_file::IoMode::preferred()), + virtual_file_io_mode: virtual_file_io_mode + .unwrap_or(virtual_file::IoModeKind::preferred()), no_sync: no_sync.unwrap_or(false), }; diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index dde9c5dd0b9a..d4af498ff390 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -17,7 +17,7 @@ use hyper::header; use hyper::StatusCode; use hyper::{Body, Request, Response, Uri}; use metrics::launch_timestamp::LaunchTimestamp; -use pageserver_api::models::virtual_file::IoMode; +use pageserver_api::models::virtual_file::IoModeKind; use pageserver_api::models::DownloadRemoteLayersTaskSpawnRequest; use pageserver_api::models::IngestAuxFilesRequest; use pageserver_api::models::ListAuxFilesRequest; @@ -2617,7 +2617,7 @@ async fn put_io_mode_handler( _cancel: CancellationToken, ) -> Result, ApiError> { check_permission(&r, None)?; - let mode: IoMode = json_request(&mut r).await?; + let mode: IoModeKind = json_request(&mut r).await?; crate::virtual_file::set_io_mode(mode); json_response(StatusCode::OK, ()) } diff --git a/pageserver/src/virtual_file.rs b/pageserver/src/virtual_file.rs index daa8b99ab0f1..79483c789c2a 100644 --- a/pageserver/src/virtual_file.rs +++ b/pageserver/src/virtual_file.rs @@ -43,7 +43,7 @@ pub use io_engine::FeatureTestResult as IoEngineFeatureTestResult; mod metadata; mod open_options; use self::owned_buffers_io::write::OwnedAsyncWriter; -pub(crate) use api::IoMode; +pub(crate) use api::IoModeKind; pub(crate) use io_engine::IoEngineKind; pub(crate) use metadata::Metadata; pub(crate) use open_options::*; @@ -138,6 +138,7 @@ impl VirtualFile { ctx: &RequestContext, /* TODO: carry a pointer to the metrics in the RequestContext instead of the parsing https://github.com/neondatabase/neon/issues/6107 */ ) -> Result { let file = match get_io_mode() { + IoMode::NotSet => panic!("not initialized"), IoMode::Buffered => { let inner = VirtualFileInner::open_with_options(path, open_options, ctx).await?; VirtualFile { @@ -1332,7 +1333,7 @@ impl OpenFiles { /// server startup. /// #[cfg(not(test))] -pub fn init(num_slots: usize, engine: IoEngineKind, mode: IoMode) { +pub fn init(num_slots: usize, engine: IoEngineKind, mode: IoModeKind) { if OPEN_FILES.set(OpenFiles::new(num_slots)).is_err() { panic!("virtual_file::init called twice"); } @@ -1370,14 +1371,72 @@ pub(crate) type IoBuffer = AlignedBuffer = AlignedSlice<'a, PAGE_SZ, ConstAlign<{ get_io_buffer_alignment() }>>; -static IO_MODE: AtomicU8 = AtomicU8::new(IoMode::preferred() as u8); +#[derive(Clone, Copy, Debug)] +#[repr(u8)] +pub(crate) enum IoMode { + NotSet, + Buffered, + #[cfg(target_os = "linux")] + Direct, +} + +impl TryFrom for IoMode { + type Error = u8; + + fn try_from(value: u8) -> Result { + Ok(match value { + v if v == (IoMode::NotSet as u8) => IoMode::NotSet, + v if v == (IoMode::Buffered as u8) => IoMode::Buffered, + #[cfg(target_os = "linux")] + v if v == (IoMode::Direct as u8) => IoMode::Direct, + x => return Err(x), + }) + } +} + +impl From for IoMode { + fn from(value: IoModeKind) -> Self { + match value { + IoModeKind::Buffered => IoMode::Buffered, + #[cfg(target_os = "linux")] + IoModeKind::Direct => IoMode::Direct, + } + } +} + +static IO_MODE: AtomicU8 = AtomicU8::new(IoMode::NotSet as u8); -pub(crate) fn set_io_mode(mode: IoMode) { +pub(crate) fn set_io_mode(mode_kind: IoModeKind) { + let mode: IoMode = mode_kind.into(); IO_MODE.store(mode as u8, std::sync::atomic::Ordering::Relaxed); } -pub(crate) fn get_io_mode() -> IoMode { - IoMode::try_from(IO_MODE.load(Ordering::Relaxed)).unwrap() +fn get_io_mode() -> IoMode { + let mode = IoMode::try_from(IO_MODE.load(Ordering::Relaxed)).unwrap(); + if cfg!(test) { + match mode { + IoMode::NotSet => { + let env_var_name = "NEON_PAGESERVER_UNIT_TEST_VIRTUAL_FILE_IOMODE"; + let kind = match std::env::var(env_var_name) { + Ok(v) => match v.parse::() { + Ok(mode_kind) => mode_kind, + Err(e) => { + panic!("invalid VirtualFile io mode for env var {env_var_name}: {e:#}: {v:?}") + } + }, + Err(std::env::VarError::NotPresent) => IoModeKind::preferred(), + Err(std::env::VarError::NotUnicode(_)) => { + panic!("env var {env_var_name} is not unicode"); + } + }; + set_io_mode(kind); + get_io_mode() + } + x => x, + } + } else { + mode + } } #[cfg(test)] mod tests { diff --git a/scripts/flaky_tests.py b/scripts/flaky_tests.py index 9312f8b3e7fa..992a32e0d867 100755 --- a/scripts/flaky_tests.py +++ b/scripts/flaky_tests.py @@ -64,6 +64,18 @@ def main(args: argparse.Namespace): else: pageserver_virtual_file_io_engine_parameter = "" + # If a test run has non-default PAGESERVER_VIRTUAL_FILE_IO_MODE (i.e. not empty, not direct), + # use it to parametrize test name along with build_type and pg_version + # + # See test_runner/fixtures/parametrize.py for details + if (io_mode := os.getenv("PAGESERVER_VIRTUAL_FILE_IO_MODE", "")) not in ( + "", + "direct", + ): + pageserver_virtual_file_io_mode_parameter = f"-{io_mode}" + else: + pageserver_virtual_file_io_mode_parameter = "" + # re-use existing records of flaky tests from before parametrization by compaction_algorithm def get_pageserver_default_tenant_config_compaction_algorithm() -> Optional[dict[str, Any]]: """Duplicated from parametrize.py""" @@ -90,10 +102,10 @@ def get_pageserver_default_tenant_config_compaction_algorithm() -> Optional[dict if row["name"].endswith("]"): parametrized_test = row["name"].replace( "[", - f"[{build_type}-pg{pg_version}{pageserver_virtual_file_io_engine_parameter}{pageserver_default_tenant_config_compaction_algorithm_parameter}-", + f"[{build_type}-pg{pg_version}{pageserver_virtual_file_io_engine_parameter}{pageserver_virtual_file_io_mode_parameter}{pageserver_default_tenant_config_compaction_algorithm_parameter}-", ) else: - parametrized_test = f"{row['name']}[{build_type}-pg{pg_version}{pageserver_virtual_file_io_engine_parameter}{pageserver_default_tenant_config_compaction_algorithm_parameter}]" + parametrized_test = f"{row['name']}[{build_type}-pg{pg_version}{pageserver_virtual_file_io_engine_parameter}{pageserver_virtual_file_io_mode_parameter}{pageserver_default_tenant_config_compaction_algorithm_parameter}]" res[row["parent_suite"]][row["suite"]][parametrized_test] = True diff --git a/test_runner/fixtures/parametrize.py b/test_runner/fixtures/parametrize.py index 1131bf090f08..82b93530e38e 100644 --- a/test_runner/fixtures/parametrize.py +++ b/test_runner/fixtures/parametrize.py @@ -81,6 +81,11 @@ def pytest_generate_tests(metafunc: Metafunc): ): metafunc.parametrize("pageserver_virtual_file_io_engine", [io_engine]) + # A hacky way to parametrize tests only for `pageserver_virtual_file_io_mode=buffered` + # And do not change test name for default `pageserver_virtual_file_io_mode=direct` to keep tests statistics + if (io_mode := os.getenv("PAGESERVER_VIRTUAL_FILE_IO_MODE", "")) not in ("", "direct"): + metafunc.parametrize("pageserver_virtual_file_io_mode", [io_mode]) + # Same hack for pageserver_default_tenant_config_compaction_algorithm if ( explicit_default := get_pageserver_default_tenant_config_compaction_algorithm()