Use JSON file for `diskUsage.cache` and include note on accuracy of value #35

kevina · 2018-03-23T19:56:21Z

Alternative to #33.

Closes #31. Closes #37.

…alue.

Stebalien

So, I obviously prefer this way :). It gives us a single file that we can store multiple stats in atomically.

Stebalien · 2018-03-23T23:31:22Z

flatfs.go

+	diskUsage   int64
+	storedValue diskUsageValue
+	// updateLock must be held when updating storedValue or writing
+	// to a file, it doesn't need to be held when reading


to "a file"?

I mean when writing diskUsage to a file.

Stebalien · 2018-03-23T23:40:03Z

flatfs.go

 	tmp, err := ioutil.TempFile(fs.path, "du-")
 	if err != nil {
+		atomic.StoreInt64(&fs.storedValue.diskUsage, origVal)


Should definitely be logging these errros.

Stebalien · 2018-03-23T23:43:13Z

flatfs.go

-		// do not store on errors. just ignore them
-		return
-	}
+	fs.updateLock.Lock()


You know, really, we should probably just do this in a goroutine and signal the goroutine with a channel. We could end up with a lot of blocking here, especially because this the "should we update" check is racy. (old bug, I guess, but we should still fix it).

Your right, and that was going to be my next step.

Stebalien · 2018-03-23T23:45:49Z

flatfs.go

 func (fs *Datastore) calculateDiskUsage() error {
 	// Try to obtain a previously stored value from disk
 	if persDu := fs.readDiskUsageFile(); persDu > 0 {
-		atomic.StoreInt64(&fs.diskUsage, persDu)
+		fs.diskUsage = persDu


Eventually, I'd like to also mark the on-file disk usage as an estimate until we close. That way, if we crash, we know that our estimate may be off by a bit. However, we can probably do that in a future PR (your preference).

This now means that the datastore needs to be properly closed, to clean up the background thread.

Kubuxu · 2018-03-24T12:54:48Z

Won't this need migration?

Stebalien · 2018-03-24T17:36:43Z

Won't this need migration?

We haven't bubbled the previous changes yet.

This simplifies the code with perhaps a slightly higher overhead during write operations.

Stebalien

Some nits/suggestions.

Stebalien · 2018-03-24T21:32:53Z

flatfs.go

+
+	fs.checkpointCh = make(chan bool, 1)
+	fs.done = make(chan bool)
+	go func() {


Nit: Can just call go fs.checkpointLoop().

Stebalien · 2018-03-24T21:33:43Z

flatfs.go

@@ -207,6 +241,12 @@ func Open(path string, syncFiles bool) (*Datastore, error) {
 		// elements in the datastore.
 		return nil, err
 	}
+
+	fs.checkpointCh = make(chan bool, 1)


nit: chan struct{} is cheeper.

Stebalien · 2018-03-24T21:35:41Z

flatfs.go

+func (fs *Datastore) deactivate() error {
+	if fs.checkpointCh != nil {
+		fs.checkpointCh <- true
+		close(fs.checkpointCh)


Nit: can just close the channel and always checkpoint one more time.

Stebalien · 2018-03-24T21:41:58Z

flatfs.go

+	}
+
+	fs.storedValue.diskUsage = du
+	fs.dirty = false


nit: may be cleaner to return errors from this function instead of using this shared dirty flag.

I tried that, and it seamed to make the code more complicated.

Stebalien · 2018-03-26T21:10:20Z

flatfs.go

+			newDu := float64(du)
+			lastCheckpointDu := float64(fs.storedValue.diskUsage)
+			diff := math.Abs(newDu - lastCheckpointDu)
+			if (lastCheckpointDu * diskUsageCheckpointPercent / 100.0) < diff {


ultra nit: can avoid floats here (lastCheckpointDu * diskUsageCheckpointPercent) < diff * 100

We can avoid the division but not the use of floats. diskUsageCheckpointPercent is a float and I don't want to make it an integer.

Stebalien · 2018-03-26T21:22:44Z

flatfs.go

+			// Otherwise insure the value will be written to disk after
+			// `diskUsageCheckpointTimeout`
+			if fs.dirty && !timerActive {
+				timer.Reset(diskUsageCheckpointTimeout)


We don't appear to ever turn this off.

Oops. Forgot to add timerActive = true.

Still missing a call to Stop as far as I can tell ~~(and a defered call to Stop)~~.

As far as I can tell, there is no need to call Stop here. If timerActive is false than the timer already expired. From the go docs (https://golang.org/pkg/time/#Timer.Reset):

If a program has already received a value from t.C, the timer is known to have expired, and t.Reset can be used directly.

And so that we are clear the expected behavior is for the DiskUsage to be written out every 2 seconds regardless of activity (rather than only do it after idling for 2 seconds). This is more likely to keep the Disk Usage estimate accurate in case of a crash during periods of high activity and a single write every 2 seconds is unlikely to add much overhead.

kevina · 2018-03-27T08:36:58Z

Note. I enhanced the tests and am now getting this failure:

--- FAIL: TestDiskUsage (0.95s)
    --- FAIL: TestDiskUsage/next-to-last (0.32s)
        flatfs_test.go:401: /tmp/test-datastore-flatfs-557319926
        flatfs_test.go:416: duNew: 1238
        flatfs_test.go:433: duPostPut: 4038
        flatfs_test.go:448: duPostDelete: 2038
        flatfs_test.go:451: duFinal: 2038
        flatfs_test.go:470: Unexpected value for diskUsage in diskUsage.cache: 2058 (expected 2038)

I put debug statements in the code and discovered that the value stored in diskUsage.cache is a value from a previous write. I verified that the final write to disk on close is being done in the correct order and has the expected value. Right now my only conclusion is that the o.s. is doing the renaming from the temporary files out of order and an older rename is being done before the final, but I can't seam to find any documentation that this is ever a problem when doing a "write to temp file and rename over the original". I do know that you should probably sync the temporary file before close but I tried that and it didn't seam to help.

I will look more into this tomorrow, but suggestions welcome. Do I need to sync the directory also?

hsanjuan

I think many checkpoints are discarded because the channel is full (// checkpoint request already pending). Thus you may be discarding the last one with the expected size in the test because previous ones are pending.

kevina · 2018-03-27T19:49:42Z

I think many checkpoints are discarded because the channel is full (// checkpoint request already pending). Thus you may be discarding the last one with the expected size in the test because previous ones are pending.

That is the intended behavior, it keeps unnecessary ones from piling up. The final checkpoint is not discarded because it only happens when the channel is closed. I verified this with debug print statements that I didn't push.

Also sync contents of diskUsage.cache to disk after the initial calculation and during shutdown.

kevina · 2018-03-27T23:08:50Z

I found the bug, I was doing something stupid. This should be ready for review now.

hsanjuan · 2018-03-28T11:15:40Z

Are we sure this is not overkill in terms of code and complexity just to inform the user that the disk usage is approximate (which always is anyway) ?

I can imagine that either users care about accuracy, in which case, given we cannot tell them how far the approximation is from reality, they'll want to manually calculate and fix their diskUsage files. Or don't care so much, in which case it is the same how the approximation was performed. This might be solving a pure documentation issue with complex code and I'm not sure it's the best thing. Just my two cents...

kevina · 2018-03-28T16:36:58Z

@hsanjuan this was mostly @Stebalien idea, it also solves some other problems such as doing the checkpoint asynchronously and ensuring the disk usage is written to disk every 2 seconds.

Stebalien · 2018-03-29T06:01:44Z

@hsanjuan basically, the only complex changes here are performance related. The "use a single json file" change should actually simplify things (although my motivation was to allow us to atomically update the accuracy and the size).

Kubuxu · 2018-03-29T11:22:25Z

flatfs.go

-	diskUsage           int64
-	diskUsageCheckpoint int64
+	// atmoic operations should always be used with diskUsage
+	diskUsage int64


Variables operated by atomics need to be aligned. This isn't guaranteed by structs in go, the solution for this is either embedded struct or placing it as the first element in a struct.

This is not really related to this patchset but should be fixed either way.

Hm. That's something that should go a the top of the documentation and in big bold letters everywhere else.

@Kubuxu

This isn't guaranteed by structs in go

To the best I can tell this is only a problem on 32-bit architectures that only guarantee 4 byte alignment on int64. Correct?

Hm. That's something that should go a the top of the documentation and in big bold letters everywhere else.

Thats what I said. Its actually at the bottom, in normal font:
https://golang.org/pkg/sync/atomic/#pkg-note-BUG

@kevina no it depends on how Go lays out the structure and x86-32 does not have atomic ops on misaligned 64bit variables.

@Kubuxu I think we are saying the same thing. See http://www.tapirgames.com/blog/golang-memory-alignment

Stebalien · 2018-03-29T08:46:58Z

flatfs.go

+		return exactA
+	}
+	return ""
+}


Should probably cover all cases (even if "cover" is return a).

There shouldn't be any other cases. The best I can do is if one is an empty string return the other. If a and b disagree otherwise the is no good value to return so I will just return "".

Then I'd return some form of "unknown".

Stebalien

Two small changes, LGTM otherwise.

Stebalien · 2018-04-02T22:39:03Z

flatfs.go

 	"strings"
 	"sync"
 	"sync/atomic"
 	"time"

-	"github.com/ipfs/go-datastore"
-	"github.com/ipfs/go-datastore/query"
+	"gx/ipfs/QmeiCcJfDW1GJnWUArudsv5rQsihpi4oyddPhdqo3CfX6i/go-datastore"


Shouldn't be rewritten.

Oops! I have been using git gui and hand picking the chunks to include, looks like I wasn't careful enough.

Stebalien · 2018-04-02T22:47:06Z

flatfs.go

+			// Otherwise insure the value will be written to disk after
+			// `diskUsageCheckpointTimeout`
+			if fs.dirty && !timerActive {
+				timer.Reset(diskUsageCheckpointTimeout)


Stebalien · 2018-04-02T22:56:00Z

flatfs.go

+	}
+
+	if err := os.Rename(tmp.Name(), filepath.Join(fs.path, DiskUsageFile)); err != nil {
+		log.Warningf("cound not write disk usage: %v", err)


This should probably be deleting the temporary file on failure (or at least trying to do so).

ghost assigned kevina Mar 23, 2018

ghost added the status/in-progress In progress label Mar 23, 2018

Use JSON file for diskUsage.cache and include note on accuracy of v…

fdadcca

…alue.

kevina force-pushed the kevina/diskusage-notes2 branch from ddada74 to fdadcca Compare March 23, 2018 20:05

kevina mentioned this pull request Mar 23, 2018

Store note on accuracy of initial disk usage calculation in diskUsage notes #33

Closed

kevina requested review from Stebalien and hsanjuan March 23, 2018 20:06

Stebalien reviewed Mar 23, 2018

View reviewed changes

kevina mentioned this pull request Mar 24, 2018

gx: update go-ds-flatfs ipfs/kubo#4872

Merged

Write diskusage in a background goroutine.

3633dfa

This now means that the datastore needs to be properly closed, to clean up the background thread.

kevina force-pushed the kevina/diskusage-notes2 branch from c2d5de5 to 3633dfa Compare March 24, 2018 05:21

kevina added 5 commits March 24, 2018 17:16

Determine when to write the diskusage in the checkpoint go routine

933a589

This simplifies the code with perhaps a slightly higher overhead during write operations.

Fix bug in test case.

69ad27b

Insure disk usage is written to disk within 2 seconds.

2df76d7

Log errors in writeDiskUsageFile()

9cb90a6

Tweaks.

4496513

kevina requested a review from Stebalien March 25, 2018 03:03

Stebalien reviewed Mar 26, 2018

View reviewed changes

Stebalien requested changes Mar 26, 2018

View reviewed changes

Code cleanups and bug fixes.

0043f36

kevina requested a review from Stebalien March 26, 2018 22:56

kevina force-pushed the kevina/diskusage-notes2 branch from 75d2d25 to 6392d21 Compare March 27, 2018 08:08

hsanjuan reviewed Mar 27, 2018

View reviewed changes

Enhance Test cases and fix bugs found.

6846908

Also sync contents of diskUsage.cache to disk after the initial calculation and during shutdown.

kevina force-pushed the kevina/diskusage-notes2 branch from 1ab6972 to 6846908 Compare March 27, 2018 23:00

Update README.

08c78dc

kevina force-pushed the kevina/diskusage-notes2 branch from 2e97c91 to 08c78dc Compare March 27, 2018 23:27

Kubuxu reviewed Mar 29, 2018

View reviewed changes

Stebalien reviewed Mar 29, 2018

View reviewed changes

kevina added 2 commits March 29, 2018 15:49

Enhance combineAccuracy to handle unknown cases better.

12c0c1d

Ensure diskUsage is correctly aligned for atomic operations.

903d4a7

kevina mentioned this pull request Apr 2, 2018

Don't display calculating datastore size on empty repo. #37

Closed

kevina force-pushed the kevina/diskusage-notes2 branch from c81a057 to a97ac3b Compare April 2, 2018 19:15

kevina requested a review from Stebalien April 2, 2018 20:57

Stebalien requested changes Apr 2, 2018

View reviewed changes

kevina force-pushed the kevina/diskusage-notes2 branch from a97ac3b to d7d25ca Compare April 2, 2018 23:29

kevina added 2 commits April 2, 2018 20:08

Only display "Calculating ..." msg if calculation takes more then 5 sec.

24e5fbe

Remove temp file on error in writeDiskUsageFile.

ab36d36

kevina force-pushed the kevina/diskusage-notes2 branch from 44bbdf5 to ab36d36 Compare April 3, 2018 00:09

Stebalien approved these changes Apr 3, 2018

View reviewed changes

kevina requested a review from Stebalien April 3, 2018 00:22

Stebalien merged commit 65bd5fb into master Apr 3, 2018

ghost removed the status/in-progress In progress label Apr 3, 2018

Stebalien deleted the kevina/diskusage-notes2 branch April 3, 2018 00:23

kevina removed the request for review from Stebalien April 3, 2018 00:24

Use JSON file for diskUsage.cache and include note on accuracy of value #35

Use JSON file for diskUsage.cache and include note on accuracy of value #35

Conversation

kevina commented Mar 23, 2018 • edited Loading

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Mar 24, 2018

Stebalien commented Mar 24, 2018

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

kevina Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina commented Mar 27, 2018 • edited Loading

hsanjuan left a comment

Choose a reason for hiding this comment

kevina commented Mar 27, 2018 • edited Loading

kevina commented Mar 27, 2018

hsanjuan commented Mar 28, 2018

kevina commented Mar 28, 2018

Stebalien commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use JSON file for `diskUsage.cache` and include note on accuracy of value #35

Use JSON file for `diskUsage.cache` and include note on accuracy of value #35

kevina commented Mar 23, 2018 •

edited

Loading

Stebalien Mar 27, 2018 •

edited

Loading

kevina Mar 27, 2018 •

edited

Loading

kevina commented Mar 27, 2018 •

edited

Loading

kevina commented Mar 27, 2018 •

edited

Loading