From 593e97112185e3804d0a59241e77bf16c3632b72 Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Tue, 9 Apr 2019 10:52:01 +0200 Subject: [PATCH] block_session: SYNC and TRIM as async operations This patch removes the blocking Block::Session::sync RPC function and adds the asynchronous operations SYNC and TRIM to the block session's packet-stream interface. Even though the patch adjusts all block components to the interface change, the components keep the former blocking handling of sync internally for now because of the design of the 'Block::Driver' interface. This old interface is not worth changing. We should instead migrate the block servers step by step to the new 'Block::Request_stream' API. Fixes #3274 --- repos/dde_rump/src/lib/rump/io.cc | 13 +++- repos/libports/src/lib/fatfs/diskio_block.cc | 12 +++- repos/os/include/block/component.h | 20 +++++- repos/os/include/block/request.h | 5 +- repos/os/include/block/request_stream.h | 3 + .../os/include/block_session/block_session.h | 22 ++++--- repos/os/include/block_session/client.h | 2 - repos/os/src/server/part_block/component.h | 66 +++++++++++++------ repos/os/src/server/part_block/driver.h | 37 +++++++++-- .../os/src/test/block_request_stream/main.cc | 2 - 10 files changed, 135 insertions(+), 47 deletions(-) diff --git a/repos/dde_rump/src/lib/rump/io.cc b/repos/dde_rump/src/lib/rump/io.cc index 60b98581c..a652d67d4 100644 --- a/repos/dde_rump/src/lib/rump/io.cc +++ b/repos/dde_rump/src/lib/rump/io.cc @@ -34,6 +34,15 @@ class Backend Block::Session::Info _info { _session.info() }; Genode::Lock _session_lock; + void _sync() + { + using Block::Session; + + Session::Tag const tag { 0 }; + _session.tx()->submit_packet(Session::sync_all_packet_descriptor(_info, tag)); + _session.tx()->get_acked_packet(); + } + public: uint64_t block_count() const { return _info.block_count; } @@ -43,7 +52,7 @@ class Backend void sync() { Genode::Lock::Guard guard(_session_lock); - _session.sync(); + _sync(); } bool submit(int op, int64_t offset, size_t length, void *data) @@ -83,7 +92,7 @@ class Backend /* sync request */ if (op & RUMPUSER_BIO_SYNC) { - _session.sync(); + _sync(); } return succeeded; diff --git a/repos/libports/src/lib/fatfs/diskio_block.cc b/repos/libports/src/lib/fatfs/diskio_block.cc index 6b02f50e9..0502a12d4 100644 --- a/repos/libports/src/lib/fatfs/diskio_block.cc +++ b/repos/libports/src/lib/fatfs/diskio_block.cc @@ -61,9 +61,19 @@ extern "C" { Info const info = Block::Connection::info(); using Block::Connection::tx; - using Block::Connection::sync; using Block::Connection::alloc_packet; + void sync() + { + /* + * We don't need to distinguish tags because there can only be one + * outstanding request. + */ + Block::Session::Tag const tag { 0 }; + tx()->submit_packet(sync_all_packet_descriptor(info, tag)); + tx()->get_acked_packet(); + } + Drive(Platform &platform, char const *label) : Block::Connection(platform.env, &platform.tx_alloc, 128*1024, label) { } diff --git a/repos/os/include/block/component.h b/repos/os/include/block/component.h index ed3565917..718f849cc 100644 --- a/repos/os/include/block/component.h +++ b/repos/os/include/block/component.h @@ -148,6 +148,24 @@ class Block::Session_component : public Block::Session_component_base, _p_to_handle); break; + case Block::Packet_descriptor::SYNC: + + /* perform (blocking) sync */ + _driver.sync(); + + _p_to_handle.succeeded(true); + _ack_packet(_p_to_handle); + _p_to_handle = Packet_descriptor(); + break; + + case Block::Packet_descriptor::TRIM: + + /* trim is a nop */ + _p_to_handle.succeeded(true); + _ack_packet(_p_to_handle); + _p_to_handle = Packet_descriptor(); + break; + default: throw Driver::Io_error(); } @@ -244,8 +262,6 @@ class Block::Session_component : public Block::Session_component_base, *******************************/ Info info() const override { return _driver.info(); } - - void sync() override { _driver.sync(); } }; diff --git a/repos/os/include/block/request.h b/repos/os/include/block/request.h index 167ab15dc..75d2c2d3b 100644 --- a/repos/os/include/block/request.h +++ b/repos/os/include/block/request.h @@ -30,7 +30,7 @@ namespace Block { struct Block::Operation { - enum class Type { INVALID = 0, READ = 1, WRITE = 2, SYNC = 3 }; + enum class Type { INVALID = 0, READ = 1, WRITE = 2, SYNC = 3, TRIM = 4 }; Type type; block_number_t block_number; @@ -38,7 +38,8 @@ struct Block::Operation bool valid() const { - return type == Type::READ || type == Type::WRITE || type == Type::SYNC; + return type == Type::READ || type == Type::WRITE + || type == Type::SYNC || type == Type::TRIM; } }; diff --git a/repos/os/include/block/request_stream.h b/repos/os/include/block/request_stream.h index 0004ad1f5..6d8f873db 100644 --- a/repos/os/include/block/request_stream.h +++ b/repos/os/include/block/request_stream.h @@ -195,6 +195,8 @@ class Block::Request_stream : Genode::Noncopyable switch (op) { case Packet_descriptor::READ: return Operation::Type::READ; case Packet_descriptor::WRITE: return Operation::Type::WRITE; + case Packet_descriptor::SYNC: return Operation::Type::SYNC; + case Packet_descriptor::TRIM: return Operation::Type::TRIM; case Packet_descriptor::END: return Operation::Type::INVALID; }; return Operation::Type::INVALID; @@ -286,6 +288,7 @@ class Block::Request_stream : Genode::Noncopyable case Operation::Type::READ: return Packet_descriptor::READ; case Operation::Type::WRITE: return Packet_descriptor::WRITE; case Operation::Type::SYNC: return Packet_descriptor::END; + case Operation::Type::TRIM: return Packet_descriptor::TRIM; case Operation::Type::INVALID: return Packet_descriptor::END; }; return Packet_descriptor::END; diff --git a/repos/os/include/block_session/block_session.h b/repos/os/include/block_session/block_session.h index 5924fdd76..1d046518c 100644 --- a/repos/os/include/block_session/block_session.h +++ b/repos/os/include/block_session/block_session.h @@ -31,7 +31,7 @@ namespace Block { /** - * Representation of an block-operation request + * Representation of a block-operation request * * The data associated with the 'Packet_descriptor' is either * the data read from or written to the block indicated by @@ -41,7 +41,7 @@ class Block::Packet_descriptor : public Genode::Packet_descriptor { public: - enum Opcode { READ, WRITE, END }; + enum Opcode { READ, WRITE, SYNC, TRIM, END }; /* * Alignment used when allocating a packet directly via the 'tx' @@ -145,11 +145,6 @@ struct Block::Session : public Genode::Session */ virtual Info info() const = 0; - /** - * Synchronize with block device, like ensuring data to be written - */ - virtual void sync() = 0; - /** * Request packet-transmission channel */ @@ -165,6 +160,16 @@ struct Block::Session : public Genode::Session */ virtual Genode::Capability tx_cap() = 0; + /** + * Return packet descriptor for syncing the entire block session + */ + static Packet_descriptor sync_all_packet_descriptor(Info const &info, Tag tag) + { + return Packet_descriptor(Packet_descriptor(0UL, 0UL), + Packet_descriptor::SYNC, + 0UL, info.block_count, tag); + } + /******************* ** RPC interface ** @@ -172,8 +177,7 @@ struct Block::Session : public Genode::Session GENODE_RPC(Rpc_info, Info, info); GENODE_RPC(Rpc_tx_cap, Genode::Capability, tx_cap); - GENODE_RPC(Rpc_sync, void, sync); - GENODE_RPC_INTERFACE(Rpc_info, Rpc_tx_cap, Rpc_sync); + GENODE_RPC_INTERFACE(Rpc_info, Rpc_tx_cap); }; #endif /* _INCLUDE__BLOCK_SESSION__BLOCK_SESSION_H_ */ diff --git a/repos/os/include/block_session/client.h b/repos/os/include/block_session/client.h index e409e02de..a06a6a82d 100644 --- a/repos/os/include/block_session/client.h +++ b/repos/os/include/block_session/client.h @@ -57,8 +57,6 @@ class Block::Session_client : public Genode::Rpc_client Tx::Source *tx() override { return _tx.source(); } - void sync() override { call(); } - Genode::Capability tx_cap() override { return call(); } /** diff --git a/repos/os/src/server/part_block/component.h b/repos/os/src/server/part_block/component.h index 44a9aa8b5..391866dbc 100644 --- a/repos/os/src/server/part_block/component.h +++ b/repos/os/src/server/part_block/component.h @@ -89,27 +89,55 @@ class Block::Session_component : public Block::Session_rpc_object, return; } - bool write = _p_to_handle.operation() == Packet_descriptor::WRITE; - sector_t off = _p_to_handle.block_number() + _partition->lba; - size_t cnt = _p_to_handle.block_count(); + sector_t const off = _p_to_handle.block_number() + _partition->lba; + size_t const cnt = _p_to_handle.block_count(); - if (write && !_writeable) { - _ack_packet(_p_to_handle); - return; - } + auto perform_io = [&] () + { + bool const write = + (_p_to_handle.operation() == Packet_descriptor::WRITE); - try { - _driver.io(write, off, cnt, - tx_sink()->packet_content(_p_to_handle), - *this, _p_to_handle); - } catch (Block::Session::Tx::Source::Packet_alloc_failed) { - if (!_req_queue_full) { - _req_queue_full = true; - Session_component::wait_queue().insert(this); + try { + _driver.io(write, off, cnt, + tx_sink()->packet_content(_p_to_handle), + *this, _p_to_handle); + } catch (Block::Session::Tx::Source::Packet_alloc_failed) { + if (!_req_queue_full) { + _req_queue_full = true; + Session_component::wait_queue().insert(this); + } + } catch (Genode::Packet_descriptor::Invalid_packet) { + Genode::error("dropping invalid Block packet"); + _p_to_handle = Packet_descriptor(); } - } catch (Genode::Packet_descriptor::Invalid_packet) { - Genode::error("dropping invalid Block packet"); - _p_to_handle = Packet_descriptor(); + }; + + switch (_p_to_handle.operation()) { + + case Packet_descriptor::READ: + perform_io(); + break; + + case Packet_descriptor::WRITE: + + if (!_writeable) { + _ack_packet(_p_to_handle); + return; + } + + perform_io(); + break; + + case Packet_descriptor::SYNC: + _driver.sync_all(*this, _p_to_handle); + break; + + case Packet_descriptor::TRIM: + case Packet_descriptor::END: + + _p_to_handle.succeeded(true); + _ack_packet(_p_to_handle); + break; } } @@ -223,8 +251,6 @@ class Block::Session_component : public Block::Session_rpc_object, .align_log2 = Genode::log2(_driver.blk_size()), .writeable = _writeable && _driver.writeable() }; } - - void sync() override { _driver.session().sync(); } }; diff --git a/repos/os/src/server/part_block/driver.h b/repos/os/src/server/part_block/driver.h index 4b3dfe24e..53c2a6f82 100644 --- a/repos/os/src/server/part_block/driver.h +++ b/repos/os/src/server/part_block/driver.h @@ -37,9 +37,7 @@ struct Block::Block_dispatcher : Genode::Interface bool operator== (const Block::Packet_descriptor& p1, const Block::Packet_descriptor& p2) { - return p1.operation() == p2.operation() && - p1.block_number() == p2.block_number() && - p1.block_count() == p2.block_count(); + return p1.tag().value == p2.tag().value; } @@ -58,13 +56,13 @@ class Block::Driver public: Request(Block_dispatcher &d, - Packet_descriptor &cli, - Packet_descriptor &srv) + Packet_descriptor const &cli, + Packet_descriptor const &srv) : _dispatcher(d), _cli(cli), _srv(srv) {} bool handle(Packet_descriptor& reply) { - bool ret = reply == _srv; + bool ret = (reply == _srv); if (ret) _dispatcher.dispatch(_cli, reply); return ret; } @@ -84,6 +82,7 @@ class Block::Driver Block::Session::Info const _info { _session.info() }; Genode::Signal_handler _source_ack; Genode::Signal_handler _source_submit; + unsigned long _tag_cnt { 0 }; void _ready_to_submit(); @@ -105,6 +104,17 @@ class Block::Driver _ready_to_submit(); } + Block::Session::Tag _alloc_tag() + { + /* + * The wrapping of '_tag_cnt' is no problem because the number + * of consecutive outstanding requests is much lower than the + * value range of tags. + */ + _tag_cnt++; + return Block::Session::Tag { _tag_cnt }; + } + public: Driver(Genode::Env &env, Genode::Heap &heap) @@ -140,7 +150,7 @@ class Block::Driver : Block::Packet_descriptor::READ; Genode::size_t const size = _info.block_size * cnt; Packet_descriptor p(_session.alloc_packet(size), - op, nr, cnt); + op, nr, cnt, _alloc_tag()); Request *r = new (&_r_slab) Request(dispatcher, cli, p); _r_list.insert(r); @@ -151,6 +161,19 @@ class Block::Driver _session.tx()->submit_packet(p); } + void sync_all(Block_dispatcher &dispatcher, Packet_descriptor &cli) + { + if (!_session.tx()->ready_to_submit()) + throw Block::Session::Tx::Source::Packet_alloc_failed(); + + Packet_descriptor const p = + Block::Session::sync_all_packet_descriptor(_info, _alloc_tag()); + + _r_list.insert(new (&_r_slab) Request(dispatcher, cli, p)); + + _session.tx()->submit_packet(p); + } + void remove_dispatcher(Block_dispatcher &dispatcher) { for (Request *r = _r_list.first(); r;) { diff --git a/repos/os/src/test/block_request_stream/main.cc b/repos/os/src/test/block_request_stream/main.cc index d60aca07c..41027efd8 100644 --- a/repos/os/src/test/block_request_stream/main.cc +++ b/repos/os/src/test/block_request_stream/main.cc @@ -60,8 +60,6 @@ struct Test::Block_session_component : Rpc_object, Info info() const override { return Request_stream::info(); } - void sync() override { } - Capability tx_cap() override { return Request_stream::tx_cap(); } };