Pinning bugs & questions

[dirkmc]

1. It seems like PinAPI takes a blockstore.PinLock() for Add() and Update() but not Rm().

2. In PinAPI.Add() we call Pin(), then Provide(), then Flush(). But if Provide() fails then we’re left in a state where a pin has been added in memory but not flushed. @michaelavila points out that Provide() adds a CID to a queue and it is actually provided asynchronously, so the current code probably makes sense but it might be worth adding a comment explaining this is the case.

3. In pinner.Pin(), we

• Take a lock before calling dserv.Add() (seems like it would be more efficient to take the lock after calling dserv.Add())
• call dserv.Add() and then call dserv.Get() (for a direct pin) or mdag.FetchGraph() (for a recursive pin). Why do we call Get() when we've just called Add()?

4. We don't flush after pinning in block.Put()

[Stebalien]

1. Yes. The pin lock just ensures we don't GC while adding a pin. It doesn't matter when removing a pin.
2. You're right. I wonder if we should unpin in that case? It might be difficult to do that safely.
3. Good question. Could you submit a PR to fix these?
4. That's a bug. Mind submitting a PR?

[dirkmc]

@Stebalien with respect to point 1 above:
Removing a pin (and flushing the pin sets) will change the set of blocks in the blockstore (because the pin sets are stored in the blockstore). So are you sure we don’t need to take a lock?

[dirkmc]

With respect to point 3 I found this commit with message

add node to dagserv under lock
Otherwise, we could run a GC between adding and pinning.

I believe that comment is no longer accurate, because calls to Pin() should be protected by blockstore.PinLock(), does that sound right to you?

[Stebalien]

Removing a pin (and flushing the pin sets) will change the set of blocks in the blockstore (because the pin sets are stored in the blockstore). So are you sure we don’t need to take a lock?

You know, I think you're right. That API really is one massive footgun.

I believe that comment is no longer accurate, because calls to Pin() should be protected by blockstore.PinLock(), does that sound right to you?

Yes. Really, that was never accurate. I was trying to fix a bug and failed. That bug has since been fixed correctly.

[gammazero]

All of these items are addressed in the current pinner:

1. Pinner no longer touches blockstore to save pins.
2. Pins are not updated, in memory or disk, until fetching all blocks has been completed.
3. Both the Add and the Get are done outside the pinner lock. The git is needed to ensure all blocks are stored locally before pinning is done.
4. Storage is now flushed after any update.