Skip to content

Commit

Permalink
Open all vlog files in RDWR mode (#923)
Browse files Browse the repository at this point in the history 
* 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 #769).

(cherry picked from commit afa8fae)
  • Loading branch information
Ibrahim Jarif committed Mar 10, 2020
1 parent 59a966b commit 2ada7cd
Showing 1 changed file with 42 additions and 75 deletions.
117 changes: 42 additions & 75 deletions value.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -148,33 +125,21 @@ 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.
// 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()
// 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
}

// You must hold lf.lock to sync()
Expand Down Expand Up @@ -717,18 +682,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")
Expand Down Expand Up @@ -785,13 +738,23 @@ 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.
case vlog.opt.SyncWrites:
flags |= y.Sync
}

// 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
// replay it, and can just open it in readonly mode.
if fid < ptr.Fid {
if err := lf.openReadOnly(); err != nil {
return err
}
continue
}

Expand All @@ -816,25 +779,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.
Expand All @@ -861,6 +805,29 @@ func (vlog *valueLog) open(db *DB, ptr valuePointer, replayFn logEntry) error {
return nil
}

func (lf *logFile) open(filename string, flags uint32) error {
var err error
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()
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 it. 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.")
defer vlog.elog.Finish()
Expand Down

0 comments on commit 2ada7cd

Please sign in to comment.