From b90c02f20ddc7d3c523ef7e2762b4bf013f35c98 Mon Sep 17 00:00:00 2001 From: Vlad Khorsun Date: Thu, 21 Dec 2023 13:47:58 +0200 Subject: [PATCH] Fixed bug #7873 : Wrong memory buffer alignment and IO buffer size when working in direct IO mode (#7890) --- src/common/classes/array.h | 10 +++ src/jrd/CryptoManager.cpp | 4 +- src/jrd/CryptoManager.h | 6 +- src/jrd/Database.cpp | 8 ++ src/jrd/Database.h | 3 + src/jrd/jrd.cpp | 12 ++- src/jrd/nbak.cpp | 9 +- src/jrd/nbak.h | 2 +- src/jrd/ods.h | 5 ++ src/jrd/pag.cpp | 14 +-- src/jrd/sdw.cpp | 142 +++++++++++++----------------- src/utilities/gstat/dba.epp | 15 ++-- src/utilities/nbackup/nbackup.cpp | 28 +++--- 13 files changed, 137 insertions(+), 121 deletions(-) diff --git a/src/common/classes/array.h b/src/common/classes/array.h index ba2a1fc9e7..a91d111b55 100644 --- a/src/common/classes/array.h +++ b/src/common/classes/array.h @@ -409,6 +409,16 @@ public: return data; } + // prepare array to be used as a buffer of capacity bytes aligned on given alignment + T* getAlignedBuffer(const size_type capacityL, const size_type alignL) + { + static_assert(sizeof(T) == 1, "sizeof(T) != 1"); + + ensureCapacity(capacityL + alignL, false); + count = capacityL + alignL; + return FB_ALIGN(data, alignL); + } + // clear array and release dinamically allocated memory void free() { diff --git a/src/jrd/CryptoManager.cpp b/src/jrd/CryptoManager.cpp index 1bce8f1805..012c69c72f 100644 --- a/src/jrd/CryptoManager.cpp +++ b/src/jrd/CryptoManager.cpp @@ -201,9 +201,9 @@ namespace Jrd { Jrd::BufferDesc bdb(bcb); bdb.bdb_page = Jrd::HEADER_PAGE_NUMBER; - UCHAR* h = FB_NEW_POOL(*Firebird::MemoryPool::getContextPool()) UCHAR[dbb->dbb_page_size + PAGE_ALIGNMENT]; + UCHAR* h = FB_NEW_POOL(*MemoryPool::getContextPool()) UCHAR[dbb->dbb_page_size + dbb->getIOBlockSize()]; buffer.reset(h); - h = FB_ALIGN(h, PAGE_ALIGNMENT); + h = FB_ALIGN(h, dbb->getIOBlockSize()); bdb.bdb_buffer = (Ods::pag*) h; Jrd::FbStatusVector* const status = tdbb->tdbb_status_vector; diff --git a/src/jrd/CryptoManager.h b/src/jrd/CryptoManager.h index 44f315567a..c75f928c67 100644 --- a/src/jrd/CryptoManager.h +++ b/src/jrd/CryptoManager.h @@ -312,16 +312,16 @@ private: public: operator Ods::pag*() { - return reinterpret_cast(FB_ALIGN(buf, PAGE_ALIGNMENT)); + return reinterpret_cast(buf); } Ods::pag* operator->() { - return reinterpret_cast(FB_ALIGN(buf, PAGE_ALIGNMENT)); + return reinterpret_cast(buf); } private: - char buf[MAX_PAGE_SIZE + PAGE_ALIGNMENT - 1]; + alignas(DIRECT_IO_BLOCK_SIZE) char buf[MAX_PAGE_SIZE]; }; class DbInfo; diff --git a/src/jrd/Database.cpp b/src/jrd/Database.cpp index b2c2411c21..57b84ffc96 100644 --- a/src/jrd/Database.cpp +++ b/src/jrd/Database.cpp @@ -55,6 +55,14 @@ namespace Jrd return pageSpace->onRawDevice(); } + ULONG Database::getIOBlockSize() const + { + if ((dbb_flags & DBB_no_fs_cache) || onRawDevice()) + return DIRECT_IO_BLOCK_SIZE; + + return PAGE_ALIGNMENT; + } + AttNumber Database::generateAttachmentId() { fb_assert(dbb_tip_cache); diff --git a/src/jrd/Database.h b/src/jrd/Database.h index 81dae9e0c8..6c3fb86e51 100644 --- a/src/jrd/Database.h +++ b/src/jrd/Database.h @@ -551,6 +551,9 @@ public: // returns an unique ID string for a database file const Firebird::string& getUniqueFileId(); + // returns the minimum IO block size + ULONG getIOBlockSize() const; + #ifdef DEV_BUILD // returns true if main lock is in exclusive state bool locked() const diff --git a/src/jrd/jrd.cpp b/src/jrd/jrd.cpp index d43aba3c54..bbcba1492b 100644 --- a/src/jrd/jrd.cpp +++ b/src/jrd/jrd.cpp @@ -7562,11 +7562,17 @@ static JAttachment* create_attachment(const PathName& alias_name, static void check_single_maintenance(thread_db* tdbb) { - UCHAR spare_memory[RAW_HEADER_SIZE + PAGE_ALIGNMENT]; - UCHAR* header_page_buffer = FB_ALIGN(spare_memory, PAGE_ALIGNMENT); + Database* const dbb = tdbb->getDatabase(); + + const ULONG ioBlockSize = dbb->getIOBlockSize(); + const ULONG headerSize = MAX(RAW_HEADER_SIZE, ioBlockSize); + + HalfStaticArray temp; + UCHAR* header_page_buffer = temp.getAlignedBuffer(headerSize, ioBlockSize); + Ods::header_page* const header_page = reinterpret_cast(header_page_buffer); - PIO_header(tdbb, header_page_buffer, RAW_HEADER_SIZE); + PIO_header(tdbb, header_page_buffer, headerSize); if ((header_page->hdr_flags & Ods::hdr_shutdown_mask) == Ods::hdr_shutdown_single) { diff --git a/src/jrd/nbak.cpp b/src/jrd/nbak.cpp index 727c080bcb..3e5f86cd96 100644 --- a/src/jrd/nbak.cpp +++ b/src/jrd/nbak.cpp @@ -899,17 +899,17 @@ void BackupManager::setForcedWrites(const bool forceWrite, const bool notUseFSCa BackupManager::BackupManager(thread_db* tdbb, Database* _database, int ini_state) : dbCreating(false), database(_database), diff_file(NULL), alloc_table(NULL), - last_allocated_page(0), current_scn(0), diff_name(*_database->dbb_permanent), + last_allocated_page(0), temp_buffers_space(*database->dbb_permanent), + current_scn(0), diff_name(*database->dbb_permanent), explicit_diff_name(false), flushInProgress(false), shutDown(false), allocIsValid(false), master(false), stateBlocking(false), stateLock(FB_NEW_POOL(*database->dbb_permanent) NBackupStateLock(tdbb, *database->dbb_permanent, this)), allocLock(FB_NEW_POOL(*database->dbb_permanent) NBackupAllocLock(tdbb, *database->dbb_permanent, this)) { // Allocate various database page buffers needed for operation - temp_buffers_space = FB_NEW_POOL(*database->dbb_permanent) BYTE[database->dbb_page_size * 3 + PAGE_ALIGNMENT]; // Align it at sector boundary for faster IO (also guarantees correct alignment for ULONG later) - BYTE* temp_buffers = reinterpret_cast(FB_ALIGN(temp_buffers_space, PAGE_ALIGNMENT)); - memset(temp_buffers, 0, database->dbb_page_size * 3); + UCHAR* temp_buffers = reinterpret_cast + (temp_buffers_space.getAlignedBuffer(database->dbb_page_size * 3, database->getIOBlockSize())); backup_state = ini_state; @@ -925,7 +925,6 @@ BackupManager::~BackupManager() delete stateLock; delete allocLock; delete alloc_table; - delete[] temp_buffers_space; } void BackupManager::setDifference(thread_db* tdbb, const char* filename) diff --git a/src/jrd/nbak.h b/src/jrd/nbak.h index e9f58c6dd8..b3df9ea515 100644 --- a/src/jrd/nbak.h +++ b/src/jrd/nbak.h @@ -493,7 +493,7 @@ private: AllocItemTree* alloc_table; // Cached allocation table of pages in difference file USHORT backup_state; ULONG last_allocated_page; // Last physical page allocated in the difference file - BYTE *temp_buffers_space; + Firebird::Array temp_buffers_space; ULONG *alloc_buffer, *empty_buffer, *spare_buffer; ULONG current_scn; Firebird::PathName diff_name; diff --git a/src/jrd/ods.h b/src/jrd/ods.h index 5ede9100dc..2e2784aa3b 100644 --- a/src/jrd/ods.h +++ b/src/jrd/ods.h @@ -918,6 +918,11 @@ Firebird::string pagtype(UCHAR type); // alignment for raw page access const USHORT PAGE_ALIGNMENT = 1024; +// alignment and IO block size/offset multiplier for non-buffered file access +const ULONG DIRECT_IO_BLOCK_SIZE = 4096; + +static_assert(MIN_PAGE_SIZE >= DIRECT_IO_BLOCK_SIZE, "check DIRECT_IO_BLOCK_SIZE"); + // size of raw I/O operation for header page const USHORT RAW_HEADER_SIZE = 1024; // ROUNDUP(HDR_SIZE, PAGE_ALIGNMENT); //static_assert(RAW_HEADER_SIZE >= HDR_SIZE, "RAW_HEADER_SIZE is less than HDR_SIZE"); diff --git a/src/jrd/pag.cpp b/src/jrd/pag.cpp index a64ca651ae..d40df23d59 100644 --- a/src/jrd/pag.cpp +++ b/src/jrd/pag.cpp @@ -1261,10 +1261,13 @@ void PAG_header_init(thread_db* tdbb) // and unit of transfer is a multiple of physical disk // sector for raw disk access. - UCHAR temp_buffer[RAW_HEADER_SIZE + PAGE_ALIGNMENT]; - UCHAR* const temp_page = FB_ALIGN(temp_buffer, PAGE_ALIGNMENT); + const ULONG ioBlockSize = dbb->getIOBlockSize(); + const ULONG headerSize = MAX(RAW_HEADER_SIZE, ioBlockSize); - PIO_header(tdbb, temp_page, RAW_HEADER_SIZE); + HalfStaticArray temp; + UCHAR* const temp_page = temp.getAlignedBuffer(headerSize, ioBlockSize); + + PIO_header(tdbb, temp_page, headerSize); const header_page* header = (header_page*) temp_page; if (header->hdr_header.pag_type != pag_header || header->hdr_sequence) @@ -1377,8 +1380,7 @@ void PAG_init2(thread_db* tdbb, USHORT shadow_number) // the temporary page buffer for raw disk access. Array temp; - UCHAR* const temp_page = - FB_ALIGN(temp.getBuffer(dbb->dbb_page_size + PAGE_ALIGNMENT), PAGE_ALIGNMENT); + UCHAR* const temp_page = temp.getAlignedBuffer(dbb->dbb_page_size, dbb->getIOBlockSize()); PageSpace* pageSpace = dbb->dbb_page_manager.findPageSpace(DB_PAGE_SPACE); jrd_file* file = pageSpace->file; @@ -2603,7 +2605,7 @@ ULONG PAG_page_count(thread_db* tdbb) Database* const dbb = tdbb->getDatabase(); Array temp; page_inv_page* pip = reinterpret_cast - (FB_ALIGN(temp.getBuffer(dbb->dbb_page_size + PAGE_ALIGNMENT), PAGE_ALIGNMENT)); + (temp.getAlignedBuffer(dbb->dbb_page_size, dbb->getIOBlockSize())); PageSpace* pageSpace = dbb->dbb_page_manager.findPageSpace(DB_PAGE_SPACE); fb_assert(pageSpace); diff --git a/src/jrd/sdw.cpp b/src/jrd/sdw.cpp index 016df82320..f833a79468 100644 --- a/src/jrd/sdw.cpp +++ b/src/jrd/sdw.cpp @@ -186,98 +186,80 @@ int SDW_add_file(thread_db* tdbb, const TEXT* file_name, SLONG start, USHORT sha // and set up to release it in case of error. Align // the spare page buffer for raw disk access. - SCHAR* const spare_buffer = - FB_NEW_POOL(*tdbb->getDefaultPool()) char[dbb->dbb_page_size + PAGE_ALIGNMENT]; - // And why doesn't the code check that the allocation succeeds? + Array temp; + UCHAR* const spare_page = temp.getAlignedBuffer(dbb->dbb_page_size, dbb->getIOBlockSize()); - SCHAR* spare_page = FB_ALIGN(spare_buffer, PAGE_ALIGNMENT); + // create the header using the spare_buffer - try { + header_page* header = (header_page*) spare_page; + header->hdr_header.pag_type = pag_header; + header->hdr_sequence = sequence; + header->hdr_page_size = dbb->dbb_page_size; + header->hdr_data[0] = HDR_end; + header->hdr_end = HDR_SIZE; + header->hdr_next_page = 0; - // create the header using the spare_buffer + // fool PIO_write into writing the scratch page into the correct place + BufferDesc temp_bdb(dbb->dbb_bcb); + temp_bdb.bdb_page = next->fil_min_page; + temp_bdb.bdb_buffer = (PAG) header; + header->hdr_header.pag_pageno = temp_bdb.bdb_page.getPageNum(); + // It's header, never encrypted + if (!PIO_write(tdbb, shadow_file, &temp_bdb, reinterpret_cast(header), 0)) + return 0; - header_page* header = (header_page*) spare_page; - header->hdr_header.pag_type = pag_header; - header->hdr_sequence = sequence; - header->hdr_page_size = dbb->dbb_page_size; + next->fil_fudge = 1; + + // Update the previous header page to point to new file -- + // we can use the same header page, suitably modified, + // because they all look pretty much the same at this point + + /******************* + Fix for bug 7925. drop_gdb wan not dropping secondary file in + multi-shadow files. The structure was not being filled with the + info. Commented some code so that the structure will always be filled. + + -Sudesh 07/06/95 + + The original code : + === + if (shadow_file == file) + copy_header(tdbb); + else + === + ************************/ + + // Temporarly reverting the change ------- Sudesh 07/07/95 ******* + + if (shadow_file == file) + { + copy_header(tdbb); + } + else + { + --start; header->hdr_data[0] = HDR_end; header->hdr_end = HDR_SIZE; header->hdr_next_page = 0; - // fool PIO_write into writing the scratch page into the correct place - BufferDesc temp_bdb(dbb->dbb_bcb); - temp_bdb.bdb_page = next->fil_min_page; - temp_bdb.bdb_buffer = (PAG) header; + PAG_add_header_entry(tdbb, header, HDR_file, static_cast(strlen(file_name)), + reinterpret_cast(file_name)); + PAG_add_header_entry(tdbb, header, HDR_last_page, sizeof(start), + reinterpret_cast(&start)); + file->fil_fudge = 0; + temp_bdb.bdb_page = file->fil_min_page; header->hdr_header.pag_pageno = temp_bdb.bdb_page.getPageNum(); // It's header, never encrypted if (!PIO_write(tdbb, shadow_file, &temp_bdb, reinterpret_cast(header), 0)) - { - delete[] spare_buffer; return 0; - } - next->fil_fudge = 1; - - // Update the previous header page to point to new file -- - // we can use the same header page, suitably modified, - // because they all look pretty much the same at this point - - /******************* - Fix for bug 7925. drop_gdb wan not dropping secondary file in - multi-shadow files. The structure was not being filled with the - info. Commented some code so that the structure will always be filled. - - -Sudesh 07/06/95 - - The original code : - === - if (shadow_file == file) - copy_header(tdbb); - else - === - ************************/ - - // Temporarly reverting the change ------- Sudesh 07/07/95 ******* - - if (shadow_file == file) - { - copy_header(tdbb); - } - else - { - --start; - header->hdr_data[0] = HDR_end; - header->hdr_end = HDR_SIZE; - header->hdr_next_page = 0; - - PAG_add_header_entry(tdbb, header, HDR_file, static_cast(strlen(file_name)), - reinterpret_cast(file_name)); - PAG_add_header_entry(tdbb, header, HDR_last_page, sizeof(start), - reinterpret_cast(&start)); - file->fil_fudge = 0; - temp_bdb.bdb_page = file->fil_min_page; - header->hdr_header.pag_pageno = temp_bdb.bdb_page.getPageNum(); - // It's header, never encrypted - if (!PIO_write(tdbb, shadow_file, &temp_bdb, reinterpret_cast(header), 0)) - { - delete[] spare_buffer; - return 0; - } - if (file->fil_min_page) { - file->fil_fudge = 1; - } - } if (file->fil_min_page) { file->fil_fudge = 1; } + } - delete[] spare_buffer; - - } // try - catch (const Firebird::Exception&) - { - delete[] spare_buffer; - throw; + if (file->fil_min_page) { + file->fil_fudge = 1; } return sequence; @@ -1004,9 +986,9 @@ void SDW_start(thread_db* tdbb, const TEXT* file_name, // catch errors: delete the shadow file if missing, and deallocate the spare buffer shadow = NULL; - SLONG* const spare_buffer = - FB_NEW_POOL(*tdbb->getDefaultPool()) SLONG[(dbb->dbb_page_size + PAGE_ALIGNMENT) / sizeof(SLONG)]; - UCHAR* spare_page = FB_ALIGN((UCHAR*) spare_buffer, PAGE_ALIGNMENT); + + Array temp; + UCHAR* const spare_page = temp.getAlignedBuffer(dbb->dbb_page_size, dbb->getIOBlockSize()); WIN window(DB_PAGE_SPACE, -1); jrd_file* shadow_file = 0; @@ -1087,8 +1069,6 @@ void SDW_start(thread_db* tdbb, const TEXT* file_name, PAG_init2(tdbb, shadow_number); - delete[] spare_buffer; - } // try catch (const Firebird::Exception& ex) { @@ -1101,7 +1081,7 @@ void SDW_start(thread_db* tdbb, const TEXT* file_name, PIO_close(shadow_file); delete shadow_file; } - delete[] spare_buffer; + if ((file_flags & FILE_manual) && !delete_files) { ERR_post(Arg::Gds(isc_shadow_missing) << Arg::Num(shadow_number)); } diff --git a/src/utilities/gstat/dba.epp b/src/utilities/gstat/dba.epp index fde3b546db..d88169d2d3 100644 --- a/src/utilities/gstat/dba.epp +++ b/src/utilities/gstat/dba.epp @@ -615,8 +615,8 @@ int gstat(Firebird::UtilSvc* uSvc) dba_fil* current = db_open(fileName.c_str(), fileName.length()); - SCHAR temp[RAW_HEADER_SIZE]; - tddba->page_size = sizeof(temp); + alignas(DIRECT_IO_BLOCK_SIZE) SCHAR temp[MAX(RAW_HEADER_SIZE, DIRECT_IO_BLOCK_SIZE)]; + tddba->page_size = MAX(RAW_HEADER_SIZE, DIRECT_IO_BLOCK_SIZE); tddba->global_buffer = (pag*) temp; tddba->page_number = -1; const header_page* header = (const header_page*) db_read((SLONG) 0); @@ -642,9 +642,14 @@ int gstat(Firebird::UtilSvc* uSvc) tddba->page_size = header->hdr_page_size; tddba->dp_per_pp = Ods::dataPagesPerPP(tddba->page_size); tddba->max_records = Ods::maxRecsPerDP(tddba->page_size); - tddba->buffer1 = (pag*) alloc(tddba->page_size); - tddba->buffer2 = (pag*) alloc(tddba->page_size); - tddba->global_buffer = (pag*) alloc(tddba->page_size); + { // scope + char* buff = alloc(tddba->page_size * 3 + DIRECT_IO_BLOCK_SIZE); + buff = FB_ALIGN(buff, DIRECT_IO_BLOCK_SIZE); + + tddba->buffer1 = (pag*) buff; + tddba->buffer2 = (pag*) (buff + tddba->page_size); + tddba->global_buffer = (pag*) (buff + tddba->page_size * 2); + } tddba->page_number = -1; // gather continuation files diff --git a/src/utilities/nbackup/nbackup.cpp b/src/utilities/nbackup/nbackup.cpp index 2d2c6af149..6db5a11687 100644 --- a/src/utilities/nbackup/nbackup.cpp +++ b/src/utilities/nbackup/nbackup.cpp @@ -1339,12 +1339,14 @@ void NBackup::backup_database(int level, Guid& guid, const PathName& fname) open_database_scan(); // Read database header - char unaligned_header_buffer[RAW_HEADER_SIZE + SECTOR_ALIGNMENT]; + const ULONG ioBlockSize = direct_io ? DIRECT_IO_BLOCK_SIZE : PAGE_ALIGNMENT; + const ULONG headerSize = MAX(RAW_HEADER_SIZE, ioBlockSize); - auto header = reinterpret_cast( - FB_ALIGN(unaligned_header_buffer, SECTOR_ALIGNMENT)); + Array header_buffer; + Ods::header_page* header = reinterpret_cast + (header_buffer.getAlignedBuffer(headerSize, ioBlockSize)); - if (read_file(dbase, header, RAW_HEADER_SIZE) != RAW_HEADER_SIZE) + if (read_file(dbase, header, headerSize) != headerSize) status_exception::raise(Arg::Gds(isc_nbackup_err_eofhdrdb) << dbname.c_str() << Arg::Num(1)); if (!Ods::isSupported(header)) @@ -1360,11 +1362,9 @@ void NBackup::backup_database(int level, Guid& guid, const PathName& fname) if ((header->hdr_flags & Ods::hdr_backup_mask) != Ods::hdr_nbak_stalled) status_exception::raise(Arg::Gds(isc_nbackup_db_notlock) << Arg::Num(header->hdr_flags)); - Array unaligned_page_buffer; - { // scope - UCHAR* buf = unaligned_page_buffer.getBuffer(header->hdr_page_size + SECTOR_ALIGNMENT); - page_buff = reinterpret_cast(FB_ALIGN(buf, SECTOR_ALIGNMENT)); - } // end scope + Array page_buffer; + Ods::pag* page_buff = reinterpret_cast + (page_buffer.getAlignedBuffer(header->hdr_page_size, ioBlockSize)); ULONG db_size = db_size_pages; seek_file(dbase, 0); @@ -1427,12 +1427,10 @@ void NBackup::backup_database(int level, Guid& guid, const PathName& fname) ULONG scnsSlot = 0; const ULONG pagesPerSCN = Ods::pagesPerSCN(header->hdr_page_size); - Array unaligned_scns_buffer; - Ods::scns_page* scns = NULL, *scns_buf = NULL; - { // scope - UCHAR* buf = unaligned_scns_buffer.getBuffer(header->hdr_page_size + SECTOR_ALIGNMENT); - scns_buf = reinterpret_cast(FB_ALIGN(buf, SECTOR_ALIGNMENT)); - } + Array scns_buffer; + Ods::scns_page* scns = NULL; + Ods::scns_page* scns_buf = reinterpret_cast + (scns_buffer.getAlignedBuffer(header->hdr_page_size, ioBlockSize)); while (true) {