From beed94d33d88746eeffbc3a9412aff95debf2624 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Mon, 8 Jul 2019 13:44:07 +0530 Subject: [PATCH 1/6] Open all vlog files in RDWR mode With this commit, all log files are opened in read-write mode. Earlier, we would open files in read-write mode for writing and then reopen them again in read only mode after writing was completed. This approach lead to unnecessary complexity and issues (see issue https://github.com/dgraph-io/badger/issues/769). --- value.go | 111 ++++++++++++++++++++----------------------------------- 1 file changed, 40 insertions(+), 71 deletions(-) diff --git a/value.go b/value.go index 815e808b2..dc01b2919 100644 --- a/value.go +++ b/value.go @@ -74,29 +74,6 @@ type logFile struct { loadingMode options.FileLoadingMode } -// openReadOnly assumes that we have a write lock on logFile. -func (lf *logFile) openReadOnly() error { - var err error - lf.fd, err = os.OpenFile(lf.path, os.O_RDONLY, 0666) - if err != nil { - return errors.Wrapf(err, "Unable to open %q as RDONLY.", lf.path) - } - - fi, err := lf.fd.Stat() - if err != nil { - return errors.Wrapf(err, "Unable to check stat for %q", lf.path) - } - y.AssertTrue(fi.Size() <= math.MaxUint32) - lf.size = uint32(fi.Size()) - - if err = lf.mmap(fi.Size()); err != nil { - _ = lf.fd.Close() - return y.Wrapf(err, "Unable to map file: %q", fi.Name()) - } - - return nil -} - func (lf *logFile) mmap(size int64) (err error) { if lf.loadingMode != options.MemoryMap { // Nothing to do @@ -148,33 +125,23 @@ func (lf *logFile) read(p valuePointer, s *y.Slice) (buf []byte, err error) { } func (lf *logFile) doneWriting(offset uint32) error { - // Sync before acquiring lock. (We call this from write() and thus know we have shared access + // Sync before acquiring lock. (We call this from write() and thus know we have shared access // to the fd.) if err := y.FileSync(lf.fd); err != nil { return errors.Wrapf(err, "Unable to sync value log: %q", lf.path) } - // Close and reopen the file read-only. Acquire lock because fd will become invalid for a bit. + // Acquire lock because fd will become invalid for a bit. // Acquiring the lock is bad because, while we don't hold the lock for a long time, it forces // one batch of readers wait for the preceding batch of readers to finish. - // - // If there's a benefit to reopening the file read-only, it might be on Windows. I don't know - // what the benefit is. Consider keeping the file read-write, or use fcntl to change - // permissions. lf.lock.Lock() defer lf.lock.Unlock() - if err := lf.munmap(); err != nil { - return err - } + // TODO: Confirm if we need to run a file sync after truncation. // Truncation must run after unmapping, otherwise Windows would crap itself. if err := lf.fd.Truncate(int64(offset)); err != nil { return errors.Wrapf(err, "Unable to truncate file: %q", lf.path) } - if err := lf.fd.Close(); err != nil { - return errors.Wrapf(err, "Unable to close value log: %q", lf.path) - } - - return lf.openReadOnly() + return nil } // You must hold lf.lock to sync() @@ -717,18 +684,6 @@ func errFile(err error, path string, msg string) error { } func (vlog *valueLog) replayLog(lf *logFile, offset uint32, replayFn logEntry) error { - var err error - mode := os.O_RDONLY - if vlog.opt.Truncate { - // We should open the file in RW mode, so it can be truncated. - mode = os.O_RDWR - } - lf.fd, err = os.OpenFile(lf.path, mode, 0) - if err != nil { - return errFile(err, lf.path, "Open file") - } - defer lf.fd.Close() - fi, err := lf.fd.Stat() if err != nil { return errFile(err, lf.path, "Unable to run file.Stat") @@ -786,12 +741,13 @@ func (vlog *valueLog) open(db *DB, ptr valuePointer, replayFn logEntry) error { lf, ok := vlog.filesMap[fid] y.AssertTrue(ok) + // Open log file "lf" in read-write mode + if err := vlog.openLogFile(lf); err != nil { + return err + } // This file is before the value head pointer. So, we don't need to // replay it, and can just open it in readonly mode. if fid < ptr.Fid { - if err := lf.openReadOnly(); err != nil { - return err - } continue } @@ -816,25 +772,6 @@ func (vlog *valueLog) open(db *DB, ptr valuePointer, replayFn logEntry) error { return err } vlog.db.opt.Infof("Replay took: %s\n", time.Since(now)) - - if fid < vlog.maxFid { - if err := lf.openReadOnly(); err != nil { - return err - } - } else { - var flags uint32 - switch { - case vlog.opt.ReadOnly: - // If we have read only, we don't need SyncWrites. - flags |= y.ReadOnly - case vlog.opt.SyncWrites: - flags |= y.Sync - } - var err error - if lf.fd, err = y.OpenExistingFile(vlog.fpath(fid), flags); err != nil { - return errFile(err, lf.path, "Open existing file") - } - } } // Seek to the end to start writing. @@ -860,6 +797,38 @@ func (vlog *valueLog) open(db *DB, ptr valuePointer, replayFn logEntry) error { } return nil } +func (vlog *valueLog) openLogFile(lf *logFile) error { + var flags uint32 + switch { + case vlog.opt.ReadOnly: + // If we have read only, we don't need SyncWrites. + flags |= y.ReadOnly + // Set sync flag only for the last vlog file. + case vlog.opt.SyncWrites && lf.fid == vlog.maxFid: + flags |= y.Sync + } + + var err error + if lf.fd, err = y.OpenExistingFile(vlog.fpath(lf.fid), flags); err != nil { + return errors.Wrapf(err, "Open existing file: %q", lf.path) + } + fstat, err := lf.fd.Stat() + if err != nil { + return errors.Wrapf(err, "Unable to check stat for %q", lf.path) + } + sz := fstat.Size() + if sz == 0 { + // File is empty. We don't need to mmap is. Return. + return nil + } + y.AssertTrue(sz <= math.MaxUint32) + lf.size = uint32(sz) + if err = lf.mmap(sz); err != nil { + _ = lf.fd.Close() + return errors.Wrapf(err, "Unable to map file: %q", fstat.Name()) + } + return nil +} func (vlog *valueLog) Close() error { vlog.elog.Printf("Stopping garbage collection of values.") From bebe11b9ebb2329f54748c2e7547e8656087fa51 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Mon, 8 Jul 2019 15:13:06 +0530 Subject: [PATCH 2/6] Fix typo --- value.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/value.go b/value.go index dc01b2919..09a6298fe 100644 --- a/value.go +++ b/value.go @@ -818,7 +818,7 @@ func (vlog *valueLog) openLogFile(lf *logFile) error { } sz := fstat.Size() if sz == 0 { - // File is empty. We don't need to mmap is. Return. + // File is empty. We don't need to mmap it. Return. return nil } y.AssertTrue(sz <= math.MaxUint32) From b43aa168e2874ac40e47ee619bde3520d4854119 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Wed, 10 Jul 2019 13:22:51 +0530 Subject: [PATCH 3/6] Remove lock from donewriting function --- value.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/value.go b/value.go index e1ad0ed76..4e93c3d5e 100644 --- a/value.go +++ b/value.go @@ -130,11 +130,6 @@ func (lf *logFile) doneWriting(offset uint32) error { if err := y.FileSync(lf.fd); err != nil { return errors.Wrapf(err, "Unable to sync value log: %q", lf.path) } - // Acquire lock because fd will become invalid for a bit. - // Acquiring the lock is bad because, while we don't hold the lock for a long time, it forces - // one batch of readers wait for the preceding batch of readers to finish. - lf.lock.Lock() - defer lf.lock.Unlock() // TODO: Confirm if we need to run a file sync after truncation. // Truncation must run after unmapping, otherwise Windows would crap itself. From 94509b96d13ef49c24ddb9264d409e79f91e9a37 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Wed, 10 Jul 2019 13:25:02 +0530 Subject: [PATCH 4/6] Add new line before function definition --- value.go | 1 + 1 file changed, 1 insertion(+) diff --git a/value.go b/value.go index 4e93c3d5e..7de19c501 100644 --- a/value.go +++ b/value.go @@ -792,6 +792,7 @@ func (vlog *valueLog) open(db *DB, ptr valuePointer, replayFn logEntry) error { } return nil } + func (vlog *valueLog) openLogFile(lf *logFile) error { var flags uint32 switch { From b2f5cc302f1eae849c472ed85cb498c77c169208 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Thu, 11 Jul 2019 17:34:41 +0530 Subject: [PATCH 5/6] Change vlog.openLogFile(..) => lf.Open(..) --- value.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/value.go b/value.go index 7de19c501..353050d98 100644 --- a/value.go +++ b/value.go @@ -136,6 +136,9 @@ func (lf *logFile) doneWriting(offset uint32) error { if err := lf.fd.Truncate(int64(offset)); err != nil { return errors.Wrapf(err, "Unable to truncate file: %q", lf.path) } + + // Previously we used to close the file after it was written and reopen it in read-only mode. + // We no longer open files in read-only mode. We keep all vlog files open in read-write mode. return nil } @@ -735,9 +738,18 @@ func (vlog *valueLog) open(db *DB, ptr valuePointer, replayFn logEntry) error { for _, fid := range fids { lf, ok := vlog.filesMap[fid] y.AssertTrue(ok) + var flags uint32 + switch { + case vlog.opt.ReadOnly: + // If we have read only, we don't need SyncWrites. + flags |= y.ReadOnly + // Set sync flag only for the last vlog file. + case vlog.opt.SyncWrites && lf.fid == vlog.maxFid: + flags |= y.Sync + } - // Open log file "lf" in read-write mode - if err := vlog.openLogFile(lf); err != nil { + // Open log file "lf" in read-write mode. + if err := lf.open(vlog.fpath(lf.fid), flags); err != nil { return err } // This file is before the value head pointer. So, we don't need to @@ -793,19 +805,9 @@ func (vlog *valueLog) open(db *DB, ptr valuePointer, replayFn logEntry) error { return nil } -func (vlog *valueLog) openLogFile(lf *logFile) error { - var flags uint32 - switch { - case vlog.opt.ReadOnly: - // If we have read only, we don't need SyncWrites. - flags |= y.ReadOnly - // Set sync flag only for the last vlog file. - case vlog.opt.SyncWrites && lf.fid == vlog.maxFid: - flags |= y.Sync - } - +func (lf *logFile) open(filename string, flags uint32) error { var err error - if lf.fd, err = y.OpenExistingFile(vlog.fpath(lf.fid), flags); err != nil { + if lf.fd, err = y.OpenExistingFile(filename, flags); err != nil { return errors.Wrapf(err, "Open existing file: %q", lf.path) } fstat, err := lf.fd.Stat() From 798e913e38089e46fe0d2c0eb62a4a589ad556ea Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Mon, 15 Jul 2019 15:11:01 +0530 Subject: [PATCH 6/6] Set sync flag for all files --- value.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/value.go b/value.go index 353050d98..a6211ad2b 100644 --- a/value.go +++ b/value.go @@ -743,8 +743,8 @@ func (vlog *valueLog) open(db *DB, ptr valuePointer, replayFn logEntry) error { case vlog.opt.ReadOnly: // If we have read only, we don't need SyncWrites. flags |= y.ReadOnly - // Set sync flag only for the last vlog file. - case vlog.opt.SyncWrites && lf.fid == vlog.maxFid: + // Set sync flag. + case vlog.opt.SyncWrites: flags |= y.Sync }