From 0e3c532bf78587e146fa88c849495ae30f43fa32 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Thu, 1 Feb 2024 19:09:03 +0200 Subject: [PATCH] app/vmselect/netstorage: prevent from disk write IO when closing temporary files Remove temporary file before closing it in order to signal the OS that it shouldn't store the file contents from page cache to disk when the file is closed. Gracefully handle the case when the file cannot be removed before being closed - in this case remove the file after closing it. This allows working on Windows. Also remove superflouos opening of temporary file for reading - re-use already opened file handle for writing. This is a follow-up for 9b1e0022878cef95bdcd32dfae164fd16a1338bd Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4020 Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/70 --- app/vmselect/netstorage/tmp_blocks_file.go | 56 ++++++++++++++++------ lib/fs/reader_at.go | 13 ++++- 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/app/vmselect/netstorage/tmp_blocks_file.go b/app/vmselect/netstorage/tmp_blocks_file.go index ba3e60de3..78288d538 100644 --- a/app/vmselect/netstorage/tmp_blocks_file.go +++ b/app/vmselect/netstorage/tmp_blocks_file.go @@ -136,21 +136,25 @@ func (tbf *tmpBlocksFile) Finalize() error { return fmt.Errorf("cannot write the remaining %d bytes to %q: %w", len(tbf.buf), fname, err) } tbf.buf = tbf.buf[:0] - r := fs.MustOpenReaderAt(fname) + r := fs.NewReaderAt(tbf.f) + // Hint the OS that the file is read almost sequentiallly. // This should reduce the number of disk seeks, which is important // for HDDs. r.MustFadviseSequentialRead(true) + // Collect local stats in order to improve performance on systems with big number of CPU cores. // See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3966 r.SetUseLocalStats() + tbf.r = r + tbf.f = nil return nil } func (tbf *tmpBlocksFile) MustReadBlockRefAt(partRef storage.PartRef, addr tmpBlockAddr) storage.BlockRef { var buf []byte - if tbf.f == nil { + if tbf.r == nil { buf = tbf.buf[addr.offset : addr.offset+uint64(addr.size)] } else { bb := tmpBufPool.Get() @@ -169,21 +173,45 @@ func (tbf *tmpBlocksFile) MustReadBlockRefAt(partRef storage.PartRef, addr tmpBl var tmpBufPool bytesutil.ByteBufferPool func (tbf *tmpBlocksFile) MustClose() { - if tbf.f == nil { + if tbf.f != nil { + // tbf.f could be non-nil if Finalize wasn't called. + // In this case tbf.r must be nil. + if tbf.r != nil { + logger.Panicf("BUG: tbf.r must be nil when tbf.f!=nil") + } + + // Try removing the file before closing it in order to prevent from flushing the in-memory data + // from page cache to the disk and save disk write IO. This may fail on non-posix systems such as Windows. + // Gracefully handle this case by attempting to remove the file after closing it. + fname := tbf.f.Name() + errRemove := os.Remove(fname) + if err := tbf.f.Close(); err != nil { + logger.Panicf("FATAL: cannot close %q: %s", fname, err) + } + if errRemove != nil { + if err := os.Remove(fname); err != nil { + logger.Panicf("FATAL: cannot remove %q: %s", fname, err) + } + } + tbf.f = nil return } - if tbf.r != nil { - // tbf.r could be nil if Finalize wasn't called. - tbf.r.MustClose() - } - fname := tbf.f.Name() - if err := tbf.f.Close(); err != nil { - logger.Panicf("FATAL: cannot close %q: %s", fname, err) + if tbf.r == nil { + // Nothing to do + return } - // We cannot remove unclosed at non-posix filesystems, like windows - if err := os.Remove(fname); err != nil { - logger.Panicf("FATAL: cannot remove %q: %s", fname, err) + + // Try removing the file before closing it in order to prevent from flushing the in-memory data + // from page cache to the disk and save disk write IO. This may fail on non-posix systems such as Windows. + // Gracefully handle this case by attempting to remove the file after closing it. + fname := tbf.r.Path() + errRemove := os.Remove(fname) + tbf.r.MustClose() + if errRemove != nil { + if err := os.Remove(fname); err != nil { + logger.Panicf("FATAL: cannot remove %q: %s", fname, err) + } } - tbf.f = nil + tbf.r = nil } diff --git a/lib/fs/reader_at.go b/lib/fs/reader_at.go index 1b170f9e2..3fdd85bd1 100644 --- a/lib/fs/reader_at.go +++ b/lib/fs/reader_at.go @@ -121,7 +121,7 @@ func (r *ReaderAt) MustFadviseSequentialRead(prefetch bool) { } } -// MustOpenReaderAt opens ReaderAt for reading from filename. +// MustOpenReaderAt opens ReaderAt for reading from the file located at path. // // MustClose must be called on the returned ReaderAt when it is no longer needed. func MustOpenReaderAt(path string) *ReaderAt { @@ -129,17 +129,28 @@ func MustOpenReaderAt(path string) *ReaderAt { if err != nil { logger.Panicf("FATAL: cannot open file for reading: %s", err) } + return NewReaderAt(f) +} + +// NewReaderAt returns ReaderAt for reading from f. +// +// NewReaderAt takes ownership for f, so it shouldn't be closed by the caller. +// +// MustClose must be called on the returned ReaderAt when it is no longer needed. +func NewReaderAt(f *os.File) *ReaderAt { var r ReaderAt r.f = f if !*disableMmap { fi, err := f.Stat() if err != nil { + path := f.Name() MustClose(f) logger.Panicf("FATAL: error in fstat(%q): %s", path, err) } size := fi.Size() data, err := mmapFile(f, size) if err != nil { + path := f.Name() MustClose(f) logger.Panicf("FATAL: cannot mmap %q: %s", path, err) }