I'm still not sure about the StorageMax check. It's good that it tries to prevent effectless GC runs, but the calculation seems off to me.

// GetStorageUsage computes the storage space taken by the repo in bytes
func (r *FSRepo) GetStorageUsage() (uint64, error) {

    storage, err := gc.Repo.GetStorageUsage()
        // ...
    if storage+offset > gc.StorageMax {
        err := ErrMaxStorageExceeded
        log.Error(err)
        return err
    }

The check seems to assume that this is the space taken by all pinned objects, and that it can't possibly remove enough blocks to get back below watermark. Although it doesn't actually know how much space it can free, since GetStorageUsage() includes both pinned (not collectable) and non-pinned (collectable) blocks.

The check would have to subtract the size of all pinned blocks, wdyt?

I think it would also be okay to just leave the check out for now, and get smarter about GC later, incrementally.

The check seems to assume that this is the space taken by all pinned objects, and that it can't possibly remove enough blocks to get back below watermark.

Yes currently it is agnostic to the pin-state of the hashes.

I chose not to isolate the pinned blocks because ipfs repo gc and the repo storage are agnostic to pin state. But if you think it is more natural/manageable to do so, I can implement it.

(Also, I did propose for constraining the number of objects stored (unitless) instead of the space they occupy--faster to compute, but I'm not sure if it is harder to put the constraint)

so it looks to me like if the GC fails once, the periodic routine exits. This seems wrong to me. core/corerepo/gc.go:157

Fixed.

@rht still not sure we're understanding each other ;)

pasting a few lines from another chat:

right now auto gc is effectively useless if​ the repo is already bigger than StorageMax
the check in L177 is a little optimization that tries to prevent useless GC runs -- imagine the sum of all pinned blocks being above the watermark -- it'd just try and try to collect without effect
​> what it actually looks at though is size of the whole repo, not just pinned items

... so in my case (GetStorageUsage() = 27G, StorageMax = 20G) it refuses to run GC thinking it can't collect anything, while it can actually collect a lot.

I think the check should either be deactivated/removed, or calculate the storage usage of pinned blocks as the base

The point of this PR is to make GC run even when storage exceeds StorageMax.

The check is cheaper than running the gc itself, and should stay. But sure I will implement the isolating the pin from the gc check, this removes false positive errs.

Hehe you're right, sorry -- yesterday this patch didn't make a lot of sense to me, somehow, but at least it does now, lol.

I'll give it a test on venus.i.ipfs.io right now.

It works fine, thank you 👍

While you're at it I wonder if #1924 is still an issue? Though the mem fix is only in dev0.4.0.

This LGTM.

this LGTM -- solid work @rht @lgierth @whyrusleeping

I'll merge now to keep this moving, can remove the log in another PR