Repository: dgraph-io/badger
Pull request #341 (2017-12-06 06:32:53)

Make mmap-ing of value log optional.
We introduce a new option ValueLogLoadingMode, which controls
whether the value log is read via mmap, or by standard File I/O.

Methods that fall in the path to reading the value from log now
take an additional argument for the slice which would be used to
read the data, if the ValueLoadLoadingMode is set to FileIO.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/dgraph-io/badger/341)
<!-- Reviewable:end -->


manishrjain: @janardhan1993 Can you have a look as well? This is a deep change and can easily have bugs. So, we need to be thorough with this one.

---

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

*[value.go, line 143 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L-zTtOP91wnxKH4nOR9:-L-zTtOP91wnxKH4nORA:bamomk3) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/value.go#L143)):*
> ```Go
> 
> func (lf *logFile) doneWriting(offset uint32) error {
> 	// Sync before acquiring lock.  (We call this from writvlog.Readvlog.Reade() and thus know we have shared access
> ```

Looks like a typo.

---

*[value.go, line 159 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L-zTzfrgeiCb6fy6VPA:-L-zTzfsIpVADrJjC_6k:b76911o) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/value.go#L159)):*
> ```Go
> 		return err
> 	}
> 	defer lf.lock.Unlock()
> ```

This change of behavior looks buggy.

---

*[value.go, line 814 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L-zUtoY7BLwqlS1TaS-:-L-zUtoY7BLwqlS1TaS0:b-cajrix) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/value.go#L814)):*
> ```Go
> 
> 	buf, err := lf.read(vp, s)
> 	return buf, lf.lock.RUnlock, err
> ```

If the file is not mmaped, we don't need to hold a lock onto it, because we're copying the value directly into slice anyway.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/341#-:-L-zVG-oOq-pWPTSza9P:baj8a2o)*
<!-- Sent from Reviewable.io -->

janardhan1993: 



Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

*[iterator.go, line 152 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L01uG3h0hCbmRdWFihr:-L01uG3h0hCbmRdWFihs:bhau0bj) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/iterator.go#L152)):*
> ```Go
> 	if item.db.opt.ValueLogLoadingMode == options.MemoryMap {
> 		buf := item.slice.Resize(len(val))
> 		copy(buf, val)
> ```

We hold a lock over vlog and don't munmap, do we still need to copy here ?

---

*[value.go, line 125 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L01ucMR92OSDlBAJUK-:-L01ucMR92OSDlBAJUK0:bc1yt85) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/value.go#L125)):*
> ```Go
> 		buf = s.Resize(int(p.Len))
> 		var n int
> 		n, err = lf.fd.ReadAt(buf, int64(offset))
> ```

Throw error if n < offset

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/341)*
<!-- Sent from Reviewable.io -->

deepakjois: 



Review status: 5 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

*[value.go, line 125 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L01ucMR92OSDlBAJUK-:-L030fjukJf10MU36LV9:bz191y0) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/value.go#L125)):*
`n` and `offset` are not comparable. Do you mean `n < len(buf)`? If so that is already covered.

> ReadAt always returns a non-nil error when n < len(b).

---

*[value.go, line 143 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L-zTtOP91wnxKH4nOR9:-L02zCAI-iffNZ5qv82z:b-886gsx) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/value.go#L143)):*
<details><summary><i>Previously, manishrjain (Manish R Jain) wrote…</i></summary><blockquote>

Looks like a typo.
</blockquote></details>

Fixed

---

*[value.go, line 159 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L-zTzfrgeiCb6fy6VPA:-L031UsgjrN7A6duJUtQ:bnkfa0r) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/value.go#L159)):*
<details><summary><i>Previously, manishrjain (Manish R Jain) wrote…</i></summary><blockquote>

This change of behavior looks buggy.
</blockquote></details>

I guess it is a typo. I intended to have `defer lf.lock.Unlock()` before doing `lf.munmap()`.

Fixed now.

---

*[value.go, line 814 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L-zUtoY7BLwqlS1TaS-:-L033W-jdxkjcQ3bIQdg:bh79iux) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/value.go#L814)):*
<details><summary><i>Previously, manishrjain (Manish R Jain) wrote…</i></summary><blockquote>

If the file is not mmaped, we don't need to hold a lock onto it, because we're copying the value directly into slice anyway.
</blockquote></details>

Good point.

I have now changed the behavior to immediately unlock if we are using File I/O.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/341)*
<!-- Sent from Reviewable.io -->

deepakjois: 



Review status: 5 of 9 files reviewed at latest revision, 5 unresolved discussions.

---

*[iterator.go, line 152 at r1](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L01uG3h0hCbmRdWFihr:-L036GOnIjcpfTdw0dWz:b-9ot0oh) ([raw file](https://github.com/dgraph-io/badger/blob/8ba769be7ad9692bac1654b4ec91bddc06617374/iterator.go#L152)):*
<details><summary><i>Previously, janardhan1993 (Janardhan Reddy) wrote…</i></summary><blockquote>

We hold a lock over vlog and don't munmap, do we still need to copy here ?
</blockquote></details>

I made a small change elsewhere based on Manish’s comment. We dont hold a lock anymore, if ValueLogLoadingMode is File I/O.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/341)*
<!-- Sent from Reviewable.io -->

manishrjain: <img class="emoji" title=":lgtm:" alt=":lgtm:" align="absmiddle" src="https://reviewable.io/lgtm.png" height="20" width="61"/> Couple of comments.

---

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

*[value.go, line 820 at r2](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L07hD7DWkWTQhHXmcDU:-L07hD7DWkWTQhHXmcDV:b-rxuluq) ([raw file](https://github.com/dgraph-io/badger/blob/1b12c379a429b39cf9a41dcf256ed31327f587f3/value.go#L820)):*
> ```Go
> 	// and return an empty function as callback.
> 	lf.lock.RUnlock()
> 	return buf, func() {}, err
> ```

Can return nil instead of empty func. runCallback checks for nil, before invoking the func.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/341#-:-L07halOkzPgxS4OmqL-:bojlezm)*
<!-- Sent from Reviewable.io -->

deepakjois: 



Review status: 7 of 9 files reviewed at latest revision, 3 unresolved discussions.

---

*[value.go, line 820 at r2](https://reviewable.io:443/reviews/dgraph-io/badger/341#-L07hD7DWkWTQhHXmcDU:-L07z6SjiHfw8_nSYHt3:b-896fix) ([raw file](https://github.com/dgraph-io/badger/blob/1b12c379a429b39cf9a41dcf256ed31327f587f3/value.go#L820)):*
<details><summary><i>Previously, manishrjain (Manish R Jain) wrote…</i></summary><blockquote>

Can return nil instead of empty func. runCallback checks for nil, before invoking the func.
</blockquote></details>

Done.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/dgraph-io/badger/341)*
<!-- Sent from Reviewable.io -->


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ea3d58b..0fa6c33 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+## [1.3.0] - 2017-12-12
+* Add `DB.NextSequence()` method to generate monotonically increasing integer
+  sequences.
+* Add `DB.Size()` method to return the size of LSM and value log files.
+* Tweaked mmap code to make Windows 32-bit builds work.
+* Tweaked build tags on some files to make iOS builds work.
+* Fix `DB.PurgeOlderVersions()` to not violate some constraints.
+
 ## [1.2.0] - 2017-11-30
 * Expose a `Txn.SetEntry()` method to allow setting the key-value pair
   and all the metadata at the same time.
@@ -24,7 +32,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [1.0.1] - 2017-11-06
 * Fix an uint16 overflow when resizing key slice
 
-[Unreleased]: https://github.com/dgraph-io/badger/compare/v1.2.0...HEAD
+[Unreleased]: https://github.com/dgraph-io/badger/compare/v1.3.0...HEAD
+[1.3.0]: https://github.com/dgraph-io/badger/compare/v1.2.0...v1.3.0
 [1.2.0]: https://github.com/dgraph-io/badger/compare/v1.1.1...v1.2.0
 [1.1.1]: https://github.com/dgraph-io/badger/compare/v1.1.0...v1.1.1
 [1.1.0]: https://github.com/dgraph-io/badger/compare/v1.0.1...v1.1.0
diff --git a/appveyor.yml b/appveyor.yml
index 6c0e485..79dac33 100644
--- a/appveyor.yml
+++ b/appveyor.yml
@@ -34,6 +34,7 @@ test_script:
   # Unit tests
   - ps: Add-AppveyorTest "Unit Tests" -Outcome Running
   - go test -v github.com/dgraph-io/badger/...
+  - go test -v -vlog_mmap=false github.com/dgraph-io/badger/...
   - ps: Update-AppveyorTest "Unit Tests" -Outcome Passed
 
 notifications:
diff --git a/contrib/cover.sh b/contrib/cover.sh
index 0d0ce4b..5e2c179 100644
--- a/contrib/cover.sh
+++ b/contrib/cover.sh
@@ -17,4 +17,7 @@ for PKG in $(go list ./...|grep -v -E 'vendor'); do
   tail -n +2 $TMP >> $OUT
 done
 
+# Another round of tests after turning off mmap
+go test -v -vlog_mmap=false github.com/dgraph-io/badger
+
 popd &> /dev/null
diff --git a/db.go b/db.go
index 8dd0b50..9427ca3 100644
--- a/db.go
+++ b/db.go
@@ -29,6 +29,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/dgraph-io/badger/options"
+
 	"golang.org/x/net/trace"
 
 	"github.com/dgraph-io/badger/skl"
@@ -212,6 +214,10 @@ func Open(opt Options) (db *DB, err error) {
 	if !(opt.ValueLogFileSize <= 2<<30 && opt.ValueLogFileSize >= 1<<20) {
 		return nil, ErrValueLogSize
 	}
+	if !(opt.ValueLogLoadingMode == options.FileIO ||
+		opt.ValueLogLoadingMode == options.MemoryMap) {
+		return nil, ErrInvalidLoadingMode
+	}
 	manifestFile, manifest, err := openOrCreateManifestFile(opt.Dir)
 	if err != nil {
 		return nil, err
@@ -890,6 +896,7 @@ func (db *DB) purgeVersionsBelow(txn *Txn, key []byte, ts uint64) error {
 	opts.AllVersions = true
 	opts.PrefetchValues = false
 	it := txn.NewIterator(opts)
+	defer it.Close()
 
 	var entries []*Entry
 
@@ -922,10 +929,11 @@ func (db *DB) PurgeOlderVersions() error {
 		opts.AllVersions = true
 		opts.PrefetchValues = false
 		it := txn.NewIterator(opts)
+		defer it.Close()
 
 		var entries []*Entry
 		var lastKey []byte
-		var count int
+		var count, size int
 		var wg sync.WaitGroup
 		errChan := make(chan error, 1)
 
@@ -955,22 +963,27 @@ func (db *DB) PurgeOlderVersions() error {
 				continue
 			}
 			// Found an older version. Mark for deletion
-			entries = append(entries,
-				&Entry{
-					Key:  y.KeyWithTs(lastKey, item.version),
-					meta: bitDelete,
-				})
+			e := &Entry{
+				Key:  y.KeyWithTs(lastKey, item.version),
+				meta: bitDelete,
+			}
 			db.vlog.updateGCStats(item)
-			count++
+			curSize := e.estimateSize(db.opt.ValueThreshold)
 
-			// Batch up 1000 entries at a time and write
-			if count == 1000 {
+			// Batch up min(1000, maxBatchCount) entries at a time and write
+			// Ensure that total batch size doesn't exceed maxBatchSize
+			if count == 1000 || count+1 >= int(db.opt.maxBatchCount) ||
+				size+curSize >= int(db.opt.maxBatchSize) {
 				if err := batchSetAsyncIfNoErr(entries); err != nil {
 					return err
 				}
 				count = 0
+				size = 0
 				entries = []*Entry{}
 			}
+			size += curSize
+			count++
+			entries = append(entries, e)
 		}
 
 		// Write last batch pending deletes
diff --git a/errors.go b/errors.go
index 5ea3e25..b03a24f 100644
--- a/errors.go
+++ b/errors.go
@@ -77,6 +77,10 @@ var (
 
 	// ErrZeroBandwidth is returned if the user passes in zero bandwidth for sequence.
 	ErrZeroBandwidth = errors.New("Bandwidth must be greater than zero")
+
+	// ErrInvalidLoadingMode is returned when opt.ValueLogLoadingMode option is not
+	// within the valid range
+	ErrInvalidLoadingMode = errors.New("Invalid ValueLogLoadingMode, must be FileIO or MemoryMap")
 )
 
 const maxKeySize = 1 << 16 // Key length can't be more than uint16, as determined by table::header.
diff --git a/iterator.go b/iterator.go
index b2824e3..1938ec9 100644
--- a/iterator.go
+++ b/iterator.go
@@ -22,6 +22,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/dgraph-io/badger/options"
+
 	"github.com/dgraph-io/badger/y"
 	farm "github.com/dgryski/go-farm"
 )
@@ -127,7 +129,7 @@ func (item *Item) yieldItemValue() ([]byte, func(), error) {
 
 	var vp valuePointer
 	vp.Decode(item.vptr)
-	return item.db.vlog.Read(vp)
+	return item.db.vlog.Read(vp, item.slice)
 }
 
 func runCallback(cb func()) {
@@ -145,9 +147,13 @@ func (item *Item) prefetchValue() {
 	if val == nil {
 		return
 	}
-	buf := item.slice.Resize(len(val))
-	copy(buf, val)
-	item.val = buf
+	if item.db.opt.ValueLogLoadingMode == options.MemoryMap {
+		buf := item.slice.Resize(len(val))
+		copy(buf, val)
+		item.val = buf
+	} else {
+		item.val = val
+	}
 }
 
 // EstimatedSize returns approximate size of the key-value pair.
diff --git a/options.go b/options.go
index d8f4430..9b8bc6e 100644
--- a/options.go
+++ b/options.go
@@ -46,6 +46,9 @@ type Options struct {
 	// How should LSM tree be accessed.
 	TableLoadingMode options.FileLoadingMode
 
+	// How should value log be accessed
+	ValueLogLoadingMode options.FileLoadingMode
+
 	// 3. Flags that user might want to review
 	// ----------------------------------------
 	// The following affect all levels of LSM tree.
@@ -92,6 +95,7 @@ var DefaultOptions = Options{
 	LevelOneSize:        256 << 20,
 	LevelSizeMultiplier: 10,
 	TableLoadingMode:    options.LoadToRAM,
+	ValueLogLoadingMode: options.MemoryMap,
 	// table.MemoryMap to mmap() the tables.
 	// table.Nothing to not preload the tables.
 	MaxLevels:               7,
diff --git a/value.go b/value.go
index 1e99f27..c53d1b9 100644
--- a/value.go
+++ b/value.go
@@ -35,6 +35,8 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/dgraph-io/badger/options"
+
 	"github.com/dgraph-io/badger/y"
 	"github.com/pkg/errors"
 	"golang.org/x/net/trace"
@@ -60,11 +62,12 @@ type logFile struct {
 	//
 	// Use shared ownership when reading/writing the file or memory map, use
 	// exclusive ownership to open/close the descriptor, unmap or remove the file.
-	lock sync.RWMutex
-	fd   *os.File
-	fid  uint32
-	fmap []byte
-	size uint32
+	lock        sync.RWMutex
+	fd          *os.File
+	fid         uint32
+	fmap        []byte
+	size        uint32
+	loadingMode options.FileLoadingMode
 }
 
 // openReadOnly assumes that we have a write lock on logFile.
@@ -90,6 +93,10 @@ func (lf *logFile) openReadOnly() error {
 }
 
 func (lf *logFile) mmap(size int64) (err error) {
+	if lf.loadingMode != options.MemoryMap {
+		// Nothing to do
+		return nil
+	}
 	lf.fmap, err = y.Mmap(lf.fd, false, size)
 	if err == nil {
 		err = y.Madvise(lf.fmap, false) // Disable readahead
@@ -97,17 +104,35 @@ func (lf *logFile) mmap(size int64) (err error) {
 	return err
 }
 
-// Acquire lock on mmap if you are calling this
-func (lf *logFile) read(p valuePointer) (buf []byte, err error) {
+func (lf *logFile) munmap() (err error) {
+	if lf.loadingMode != options.MemoryMap {
+		// Nothing to do
+		return nil
+	}
+	if err := y.Munmap(lf.fmap); err != nil {
+		return errors.Wrapf(err, "Unable to munmap value log: %q", lf.path)
+	}
+	return nil
+}
+
+// Acquire lock on mmap/file if you are calling this
+func (lf *logFile) read(p valuePointer, s *y.Slice) (buf []byte, err error) {
 	var nbr int64
 	offset := p.Offset
-	size := uint32(len(lf.fmap))
-	valsz := p.Len
-	if offset >= size || offset+valsz > size {
-		err = y.ErrEOF
+	if lf.loadingMode == options.FileIO {
+		buf = s.Resize(int(p.Len))
+		var n int
+		n, err = lf.fd.ReadAt(buf, int64(offset))
+		nbr = int64(n)
 	} else {
-		buf = lf.fmap[offset : offset+valsz]
-		nbr = int64(valsz)
+		size := uint32(len(lf.fmap))
+		valsz := p.Len
+		if offset >= size || offset+valsz > size {
+			err = y.ErrEOF
+		} else {
+			buf = lf.fmap[offset : offset+valsz]
+			nbr = int64(valsz)
+		}
 	}
 	y.NumReads.Add(1)
 	y.NumBytesRead.Add(nbr)
@@ -129,8 +154,8 @@ func (lf *logFile) doneWriting(offset uint32) error {
 	// permissions.
 	lf.lock.Lock()
 	defer lf.lock.Unlock()
-	if err := y.Munmap(lf.fmap); err != nil {
-		return errors.Wrapf(err, "Unable to munmap value log: %q", lf.path)
+	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.
@@ -413,7 +438,7 @@ func (vlog *valueLog) decrIteratorCount() error {
 
 func (vlog *valueLog) deleteLogFile(lf *logFile) error {
 	path := vlog.fpath(lf.fid)
-	if err := y.Munmap(lf.fmap); err != nil {
+	if err := lf.munmap(); err != nil {
 		_ = lf.fd.Close()
 		return err
 	}
@@ -481,7 +506,11 @@ func (vlog *valueLog) openOrCreateFiles() error {
 		}
 		found[fid] = struct{}{}
 
-		lf := &logFile{fid: uint32(fid), path: vlog.fpath(uint32(fid))}
+		lf := &logFile{
+			fid:         uint32(fid),
+			path:        vlog.fpath(uint32(fid)),
+			loadingMode: vlog.opt.ValueLogLoadingMode,
+		}
 		vlog.filesMap[uint32(fid)] = lf
 		if uint32(fid) > maxFid {
 			maxFid = uint32(fid)
@@ -517,7 +546,7 @@ func (vlog *valueLog) openOrCreateFiles() error {
 
 func (vlog *valueLog) createVlogFile(fid uint32) (*logFile, error) {
 	path := vlog.fpath(fid)
-	lf := &logFile{fid: fid, path: path}
+	lf := &logFile{fid: fid, path: path, loadingMode: vlog.opt.ValueLogLoadingMode}
 	vlog.writableLogOffset = 0
 
 	var err error
@@ -559,7 +588,7 @@ func (vlog *valueLog) Close() error {
 	for id, f := range vlog.filesMap {
 
 		f.lock.Lock() // We won’t release the lock.
-		if munmapErr := y.Munmap(f.fmap); munmapErr != nil && err == nil {
+		if munmapErr := f.munmap(); munmapErr != nil && err == nil {
 			err = munmapErr
 		}
 
@@ -757,7 +786,7 @@ func (vlog *valueLog) getFileRLocked(fid uint32) (*logFile, error) {
 
 // Read reads the value log at a given location.
 // TODO: Make this read private.
-func (vlog *valueLog) Read(vp valuePointer) ([]byte, func(), error) {
+func (vlog *valueLog) Read(vp valuePointer, s *y.Slice) ([]byte, func(), error) {
 	// Check for valid offset if we are reading to writable log.
 	if vp.Fid == vlog.maxFid && vp.Offset >= vlog.writableOffset() {
 		return nil, nil, errors.Errorf(
@@ -765,7 +794,7 @@ func (vlog *valueLog) Read(vp valuePointer) ([]byte, func(), error) {
 			vp.Offset, vlog.writableOffset())
 	}
 
-	buf, cb, err := vlog.readValueBytes(vp)
+	buf, cb, err := vlog.readValueBytes(vp, s)
 	if err != nil {
 		return nil, cb, err
 	}
@@ -775,14 +804,20 @@ func (vlog *valueLog) Read(vp valuePointer) ([]byte, func(), error) {
 	return buf[n : n+h.vlen], cb, nil
 }
 
-func (vlog *valueLog) readValueBytes(vp valuePointer) ([]byte, func(), error) {
+func (vlog *valueLog) readValueBytes(vp valuePointer, s *y.Slice) ([]byte, func(), error) {
 	lf, err := vlog.getFileRLocked(vp.Fid)
 	if err != nil {
 		return nil, nil, errors.Wrapf(err, "Unable to read from value log: %+v", vp)
 	}
 
-	buf, err := lf.read(vp)
-	return buf, lf.lock.RUnlock, err
+	buf, err := lf.read(vp, s)
+	if vlog.opt.ValueLogLoadingMode == options.MemoryMap {
+		return buf, lf.lock.RUnlock, err
+	}
+	// If we are using File I/O we unlock the file immediately
+	// and return an empty function as callback.
+	lf.lock.RUnlock()
+	return buf, nil, err
 }
 
 // Test helper
@@ -892,6 +927,7 @@ func (vlog *valueLog) doRunGC(gcThreshold float64, head valuePointer) (err error
 
 	start := time.Now()
 	y.AssertTrue(vlog.kv != nil)
+	s := new(y.Slice)
 	err = vlog.iterate(lf, 0, func(e Entry, vp valuePointer) error {
 		esz := float64(vp.Len) / (1 << 20) // in MBs. +4 for the CAS stuff.
 		skipped += esz
@@ -941,7 +977,7 @@ func (vlog *valueLog) doRunGC(gcThreshold float64, head valuePointer) (err error
 		} else {
 			vlog.elog.Printf("Reason=%+v\n", r)
 
-			buf, cb, err := vlog.readValueBytes(vp)
+			buf, cb, err := vlog.readValueBytes(vp, s)
 			if err != nil {
 				return errStop
 			}
diff --git a/y/file_dsync.go b/y/file_dsync.go
index 5e62833..bd25c9e 100644
--- a/y/file_dsync.go
+++ b/y/file_dsync.go
@@ -1,4 +1,4 @@
-// +build !dragonfly,!freebsd,!windows
+// +build !dragonfly,!freebsd,!windows,!darwin
 
 /*
  * Copyright 2017 Dgraph Labs, Inc. and Contributors
diff --git a/y/file_nodsync.go b/y/file_nodsync.go
index b68be7a..be295f2 100644
--- a/y/file_nodsync.go
+++ b/y/file_nodsync.go
@@ -1,4 +1,4 @@
-// +build dragonfly freebsd windows
+// +build dragonfly freebsd windows darwin
 
 /*
  * Copyright 2017 Dgraph Labs, Inc. and Contributors


Test patch:
diff --git a/db_test.go b/db_test.go
index d1672d2..fc8c33e 100644
--- a/db_test.go
+++ b/db_test.go
@@ -19,6 +19,7 @@ package badger
 import (
 	"bytes"
 	"encoding/binary"
+	"flag"
 	"fmt"
 	"io/ioutil"
 	"log"
@@ -32,10 +33,14 @@ import (
 	"testing"
 	"time"
 
+	"github.com/dgraph-io/badger/options"
+
 	"github.com/dgraph-io/badger/y"
 	"github.com/stretchr/testify/require"
 )
 
+var mmap = flag.Bool("vlog_mmap", true, "Specify if value log must be memory-mapped")
+
 func getTestOptions(dir string) Options {
 	opt := DefaultOptions
 	opt.MaxTableSize = 1 << 15 // Force more compaction.
@@ -43,6 +48,9 @@ func getTestOptions(dir string) Options {
 	opt.Dir = dir
 	opt.ValueDir = dir
 	opt.SyncWrites = false
+	if !*mmap {
+		opt.ValueLogLoadingMode = options.FileIO
+	}
 	return opt
 }
 
diff --git a/value_test.go b/value_test.go
index 294a359..7fae7f5 100644
--- a/value_test.go
+++ b/value_test.go
@@ -59,8 +59,9 @@ func TestValueBasic(t *testing.T) {
 	require.Len(t, b.Ptrs, 2)
 	t.Logf("Pointer written: %+v %+v\n", b.Ptrs[0], b.Ptrs[1])
 
-	buf1, cb1, err1 := log.readValueBytes(b.Ptrs[0])
-	buf2, cb2, err2 := log.readValueBytes(b.Ptrs[1])
+	s := new(y.Slice)
+	buf1, cb1, err1 := log.readValueBytes(b.Ptrs[0], s)
+	buf2, cb2, err2 := log.readValueBytes(b.Ptrs[1], s)
 	require.NoError(t, err1)
 	require.NoError(t, err2)
 	defer runCallback(cb1)
@@ -529,6 +530,42 @@ func TestValueLogTrigger(t *testing.T) {
 	require.Equal(t, ErrRejected, err, "Error should be returned after closing DB.")
 }
 
+func TestValueLogGC(t *testing.T) {
+	dir, err := ioutil.TempDir("", "badger")
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+
+	opt := getTestOptions(dir)
+	opt.ValueLogFileSize = 15 << 20
+	runBadgerTest(t, &opt, func(t *testing.T, kv *DB) {
+		sz := 32 << 10
+		for j := 0; j < 40; j++ {
+			err := kv.Update(func(txn *Txn) error {
+				for i := 0; i < 45; i++ {
+					v := make([]byte, sz)
+					rand.Read(v[:rand.Intn(sz)])
+					require.NoError(t, txn.Set([]byte(fmt.Sprintf("key%d", i)), v))
+				}
+				return nil
+			})
+			require.NoError(t, err)
+		}
+		fids := kv.vlog.sortedFids()
+		require.NoError(t, kv.PurgeOlderVersions())
+		require.NoError(t, kv.RunValueLogGC(0.3))
+		newFids := kv.vlog.sortedFids()
+		require.Equal(t, len(newFids)+1, len(fids))
+		for i, fid := range fids {
+			if i < len(newFids) && newFids[i] == fid {
+				continue
+			}
+			// Check that vlog is deleted.
+			_, err = os.Stat(fmt.Sprintf("dir%c%06d.vlog", os.PathSeparator, fid))
+			require.Error(t, err)
+		}
+	})
+}
+
 func createVlog(t *testing.T, entries []*Entry) []byte {
 	dir, err := ioutil.TempDir("", "badger")
 	require.NoError(t, err)
@@ -606,7 +643,8 @@ func BenchmarkReadWrite(b *testing.B) {
 							b.Fatalf("Zero length of ptrs")
 						}
 						idx := rand.Intn(ln)
-						buf, cb, err := vl.readValueBytes(ptrs[idx])
+						s := new(y.Slice)
+						buf, cb, err := vl.readValueBytes(ptrs[idx], s)
 						if err != nil {
 							b.Fatalf("Benchmark Read: %v", err)
 						}