@cryptotaxi247 / kubo / commits / bb020ea1e

fix: deadlock while racing `ipfs dag import` and `ipfs repo gc`

This fixes a deadlock introduced in 1457b4fd4abff0bdcdccbacc26934cb7fa8e8a43. We can't use the coreapi here because it will try to take the PinLock (RLock) again, so revert this small part of 1457b4fd4abff0bdcdccbacc26934cb7fa8e8a43. This used cause a deadlock when concurrently running `ipfs dag import` concurrently with the GC. The bug is that `ipfs dag import` takes an RLock with the PinLock. *the cars are imported, leaving a wide window of time* Then GC Takes a Lock on that same RWMutex while taking the GC Lock (it blocks because it waits for the RLock to be released). Then the car imports are finished and `ipfs dag import` tries to aqcuire the PinLock (doing an RLock) again in `Api().Pin`. However at this point the RWMutex is starved, the runtime put a fence in front of RLocks if a Lock has been waiting for too lock (else you could have an endless stream of RLock / RUnlock forever delaying a Lock to ever go through). The issue is that `ipfs dag import`'s original RLock which is blocking everyone will be released once it returns, which only happens when `Api().Pin` completes. So we have a deadlock (ABA kind ?), because `ipfs dag import` waits on the GC Lock, which waits on `ipfs dag import`. Calling the Pinner directly does not acquire the PinLock again, and thus does not have this issue.

Jorropo committed Mar 25, 2023 at 11:19 UTC bb020ea1ef8765f873e06ae4fd7f58c075b04edc

1 file changed +13 -6

core/commands/dag/import.go

+13 -6

@@ -10,7 +10,6 @@ import (

10 ipldlegacy "github.com/ipfs/go-ipld-legacy"

11 "github.com/ipfs/go-libipfs/files"

12 "github.com/ipfs/interface-go-ipfs-core/options"

13 - "github.com/ipfs/interface-go-ipfs-core/path"

13 gocarv2 "github.com/ipld/go-car/v2"

15 "github.com/ipfs/kubo/core/commands/cmdenv"

@@ -108,6 +107,9 @@ func dagImport(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment

107 }

108 return nil

109 }()

110 + if err != nil {

111 + return err

112 + }

113 }

114

115 if err := batch.Commit(); err != nil {

@@ -125,11 +127,16 @@ func dagImport(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment

127 ret := RootMeta{Cid: c}

128

129 // This will trigger a full read of the DAG in the pinner, to make sure we have all blocks.

128 - // Ideally we would have a lighter merkledag.Walk() instead of the underlying merkledag.FetchDag,

129 - // then pinner.PinWithMode().

130 - err = api.Pin().Add(req.Context, path.IpldPath(c), options.Pin.Recursive(true))

131 - if err != nil {

132 - return err

130 + // Ideally we would do colloring of the pinning state while importing the blocks

131 + // and ensure the gray bucket is empty at the end (or use the network to download missing blocks).

132 + if block, err := node.Blockstore.Get(req.Context, c); err != nil {

133 + ret.PinErrorMsg = err.Error()

134 + } else if nd, err := ipldlegacy.DecodeNode(req.Context, block); err != nil {

135 + ret.PinErrorMsg = err.Error()

136 + } else if err := node.Pinning.Pin(req.Context, nd, true); err != nil {

137 + ret.PinErrorMsg = err.Error()

138 + } else if err := node.Pinning.Flush(req.Context); err != nil {

139 + ret.PinErrorMsg = err.Error()

140 }

141

142 return res.Emit(&CarImportOutput{Root: &ret})