Fixed bug #7873 : Wrong memory buffer alignment and IO buffer size when working in direct IO mode (#7890)

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()
 	{

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;

src/jrd/CryptoManager.h

@@ -312,16 +312,16 @@ private:
 	public:
 		operator Ods::pag*()
 		{
-			return reinterpret_cast<Ods::pag*>(FB_ALIGN(buf, PAGE_ALIGNMENT));
+			return reinterpret_cast<Ods::pag*>(buf);
 		}
 
 		Ods::pag* operator->()
 		{
-			return reinterpret_cast<Ods::pag*>(FB_ALIGN(buf, PAGE_ALIGNMENT));
+			return reinterpret_cast<Ods::pag*>(buf);
 		}
 
 	private:
-		char buf[MAX_PAGE_SIZE + PAGE_ALIGNMENT - 1];
+		alignas(DIRECT_IO_BLOCK_SIZE) char buf[MAX_PAGE_SIZE];
 	};
 
 	class DbInfo;

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);

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

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<UCHAR, RAW_HEADER_SIZE + PAGE_ALIGNMENT> temp;
+	UCHAR* header_page_buffer = temp.getAlignedBuffer(headerSize, ioBlockSize);
+
 	Ods::header_page* const header_page = reinterpret_cast<Ods::header_page*>(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)
 	{

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<BYTE*>(FB_ALIGN(temp_buffers_space, PAGE_ALIGNMENT));
-	memset(temp_buffers, 0, database->dbb_page_size * 3);
+	UCHAR* temp_buffers = reinterpret_cast<UCHAR*>
+		(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)

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<UCHAR> temp_buffers_space;
 	ULONG *alloc_buffer, *empty_buffer, *spare_buffer;
 	ULONG current_scn;
 	Firebird::PathName diff_name;

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");

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<UCHAR, RAW_HEADER_SIZE + PAGE_ALIGNMENT> 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<UCHAR> 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<UCHAR> temp;
 	page_inv_page* pip = reinterpret_cast<Ods::page_inv_page*>
-		(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);

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<UCHAR> 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<Ods::pag*>(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<USHORT>(strlen(file_name)),
+								reinterpret_cast<const UCHAR*>(file_name));
+		PAG_add_header_entry(tdbb, header, HDR_last_page, sizeof(start),
+								reinterpret_cast<const UCHAR*>(&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<Ods::pag*>(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<USHORT>(strlen(file_name)),
-								 reinterpret_cast<const UCHAR*>(file_name));
-			PAG_add_header_entry(tdbb, header, HDR_last_page, sizeof(start),
-								 reinterpret_cast<const UCHAR*>(&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<Ods::pag*>(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<UCHAR> 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));
 		}

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

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<Ods::header_page*>(
-			FB_ALIGN(unaligned_header_buffer, SECTOR_ALIGNMENT));
+		Array<UCHAR> header_buffer;
+		Ods::header_page* header = reinterpret_cast<Ods::header_page*>
+			(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<UCHAR> unaligned_page_buffer;
-		{ // scope
-			UCHAR* buf = unaligned_page_buffer.getBuffer(header->hdr_page_size + SECTOR_ALIGNMENT);
-			page_buff = reinterpret_cast<Ods::pag*>(FB_ALIGN(buf, SECTOR_ALIGNMENT));
-		} // end scope
+		Array<UCHAR> page_buffer;
+		Ods::pag* page_buff = reinterpret_cast<Ods::pag*>
+			(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<UCHAR> 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<Ods::scns_page*>(FB_ALIGN(buf, SECTOR_ALIGNMENT));
-		}
+		Array<UCHAR> scns_buffer;
+		Ods::scns_page* scns = NULL;
+		Ods::scns_page* scns_buf = reinterpret_cast<Ods::scns_page*>
+			(scns_buffer.getAlignedBuffer(header->hdr_page_size, ioBlockSize));
 
 		while (true)
 		{