From 67fbd34d8379a1b8232aea5d126a389f64bdc59a Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Wed, 5 Nov 2014 09:25:02 -0500 Subject: [PATCH 1/6] devmapper: Move file write and rename functionality in a separate function Currently we save device metadata and have a helper function saveMetadata() which converts data in json format as well as saves it to file. For converting data in json format, one needs to know what is being saved. Break this function down in two functions. One function only has file write capability and takes in argument about byte array of json data. Now this function does not have to know what data is being saved. It only knows about a stream of json data is being saved to a file. This allows me to reuse this function to save a different type of metadata. In this case I am planning to save NextDeviceId so that docker can use this device Id upon next restart. Otherwise docker starts from 0 which is suboptimal. Signed-off-by: Vivek Goyal --- daemon/graphdriver/devmapper/deviceset.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go index fdfc089a82..7c767fff97 100644 --- a/daemon/graphdriver/devmapper/deviceset.go +++ b/daemon/graphdriver/devmapper/deviceset.go @@ -200,11 +200,8 @@ func (devices *DeviceSet) removeMetadata(info *DevInfo) error { return nil } -func (devices *DeviceSet) saveMetadata(info *DevInfo) error { - jsonData, err := json.Marshal(info) - if err != nil { - return fmt.Errorf("Error encoding metadata to json: %s", err) - } +// Given json data and file path, write it to disk +func (devices *DeviceSet) writeMetaFile(jsonData []byte, filePath string) error { tmpFile, err := ioutil.TempFile(devices.metadataDir(), ".tmp") if err != nil { return fmt.Errorf("Error creating metadata file: %s", err) @@ -223,10 +220,23 @@ func (devices *DeviceSet) saveMetadata(info *DevInfo) error { if err := tmpFile.Close(); err != nil { return fmt.Errorf("Error closing metadata file %s: %s", tmpFile.Name(), err) } - if err := os.Rename(tmpFile.Name(), devices.metadataFile(info)); err != nil { + if err := os.Rename(tmpFile.Name(), filePath); err != nil { return fmt.Errorf("Error committing metadata file %s: %s", tmpFile.Name(), err) } + return nil +} + +func (devices *DeviceSet) saveMetadata(info *DevInfo) error { + jsonData, err := json.Marshal(info) + if err != nil { + return fmt.Errorf("Error encoding metadata to json: %s", err) + } + err = devices.writeMetaFile(jsonData, devices.metadataFile(info)) + if err != nil { + return err + } + if devices.NewTransactionId != devices.TransactionId { if err = setTransactionId(devices.getPoolDevName(), devices.TransactionId, devices.NewTransactionId); err != nil { return fmt.Errorf("Error setting devmapper transition ID: %s", err) From 8e9a18039be6ade0b8db65f7f298959055d86192 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Wed, 5 Nov 2014 09:25:02 -0500 Subject: [PATCH 2/6] devmapper: Export nextDeviceId so that json.Marshal() can operate on it I was trying to save nextDeviceId to a file but it would not work and json.Marshal() will do nothing. Then some search showed that I need to make first letter of struct field capital, exporting this field and now json.Marshal() works. This is a preparatory patch for the next one. Signed-off-by: Vivek Goyal --- daemon/graphdriver/devmapper/deviceset.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go index 7c767fff97..c02f369d23 100644 --- a/daemon/graphdriver/devmapper/deviceset.go +++ b/daemon/graphdriver/devmapper/deviceset.go @@ -68,7 +68,7 @@ type DeviceSet struct { devicePrefix string TransactionId uint64 NewTransactionId uint64 - nextDeviceId int + NextDeviceId int // Options dataLoopbackSize int64 @@ -407,7 +407,7 @@ func (devices *DeviceSet) setupBaseImage() error { log.Debugf("Initializing base device-manager snapshot") - id := devices.nextDeviceId + id := devices.NextDeviceId // Create initial device if err := createDevice(devices.getPoolDevName(), &id); err != nil { @@ -415,7 +415,7 @@ func (devices *DeviceSet) setupBaseImage() error { } // Ids are 24bit, so wrap around - devices.nextDeviceId = (id + 1) & 0xffffff + devices.NextDeviceId = (id + 1) & 0xffffff log.Debugf("Registering base device (id %v) with FS size %v", id, devices.baseFsSize) info, err := devices.registerDevice(id, "", devices.baseFsSize) @@ -703,7 +703,7 @@ func (devices *DeviceSet) AddDevice(hash, baseHash string) error { return fmt.Errorf("device %s already exists", hash) } - deviceId := devices.nextDeviceId + deviceId := devices.NextDeviceId if err := createSnapDevice(devices.getPoolDevName(), &deviceId, baseInfo.Name(), baseInfo.DeviceId); err != nil { log.Debugf("Error creating snap device: %s", err) @@ -711,7 +711,7 @@ func (devices *DeviceSet) AddDevice(hash, baseHash string) error { } // Ids are 24bit, so wrap around - devices.nextDeviceId = (deviceId + 1) & 0xffffff + devices.NextDeviceId = (deviceId + 1) & 0xffffff if _, err := devices.registerDevice(deviceId, hash, baseInfo.Size); err != nil { deleteDevice(devices.getPoolDevName(), deviceId) From 8c9e5e5e05f8ddfcf8cd5218edb83d9fe8238d81 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Wed, 5 Nov 2014 09:25:02 -0500 Subject: [PATCH 3/6] devmapper: Save and restore NextDeviceId in a file The way thin-pool right now is designed, user space is supposed to keep track of what device ids have already been used. If user space tries to create a new thin/snap device and device id has already been used, thin pool retuns -EEXIST. Upon receiving -EEXIST, current docker implementation simply tries the NextDeviceId++ and keeps on doing this till it finds a free device id. This approach has two issues. - It is little suboptimal. - If device id already exists, current kenrel implementation spits out a messsage on console. [17991.140135] device-mapper: thin: Creation of new snapshot 33 of device 3 failed. Here kenrel is trying to tell user that device id 33 has already been used. And this shows up for every device id docker tries till it reaches a point where device ids are not used. So if there are thousands of container and one is trying to create a new container after fresh docker start, expect thousands of such warnings to flood console. This patch saves the NextDeviceId in a file in /var/lib/docker/devmapper/metadata/deviceset-metadata and reads it back when docker starts. This way we don't retry lots of device ids which have already been used. There might be some device ids which are free but we will get back to them once device numbers wrap around (24bit limit on device ids). This patch should cut down on number of kernel warnings. Notice that I am creating a deviceset metadata file which is a global file for this pool. So down the line if we need to save more data we should be able to do that. Signed-off-by: Vivek Goyal --- daemon/graphdriver/devmapper/deviceset.go | 72 +++++++++++++++++------ 1 file changed, 55 insertions(+), 17 deletions(-) diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go index c02f369d23..b193480459 100644 --- a/daemon/graphdriver/devmapper/deviceset.go +++ b/daemon/graphdriver/devmapper/deviceset.go @@ -62,25 +62,25 @@ type MetaData struct { } type DeviceSet struct { - MetaData - sync.Mutex // Protects Devices map and serializes calls into libdevmapper - root string - devicePrefix string - TransactionId uint64 - NewTransactionId uint64 - NextDeviceId int + MetaData `json:"-"` + sync.Mutex `json:"-"` // Protects Devices map and serializes calls into libdevmapper + root string `json:"-"` + devicePrefix string `json:"-"` + TransactionId uint64 `json:"-"` + NewTransactionId uint64 `json:"-"` + NextDeviceId int `json:"next_device_id"` // Options - dataLoopbackSize int64 - metaDataLoopbackSize int64 - baseFsSize uint64 - filesystem string - mountOptions string - mkfsArgs []string - dataDevice string - metadataDevice string - doBlkDiscard bool - thinpBlockSize uint32 + dataLoopbackSize int64 `json:"-"` + metaDataLoopbackSize int64 `json:"-"` + baseFsSize uint64 `json:"-"` + filesystem string `json:"-"` + mountOptions string `json:"-"` + mkfsArgs []string `json:"-"` + dataDevice string `json:"-"` + metadataDevice string `json:"-"` + doBlkDiscard bool `json:"-"` + thinpBlockSize uint32 `json:"-"` } type DiskUsage struct { @@ -138,6 +138,10 @@ func (devices *DeviceSet) metadataFile(info *DevInfo) string { return path.Join(devices.metadataDir(), file) } +func (devices *DeviceSet) deviceSetMetaFile() string { + return path.Join(devices.metadataDir(), "deviceset-metadata") +} + func (devices *DeviceSet) oldMetadataFile() string { return path.Join(devices.loopbackDir(), "json") } @@ -545,6 +549,34 @@ func (devices *DeviceSet) ResizePool(size int64) error { return nil } +func (devices *DeviceSet) loadDeviceSetMetaData() error { + jsonData, err := ioutil.ReadFile(devices.deviceSetMetaFile()) + if err != nil { + return nil + } + + if err := json.Unmarshal(jsonData, devices); err != nil { + return nil + } + + return nil +} + +func (devices *DeviceSet) saveDeviceSetMetaData() error { + jsonData, err := json.Marshal(devices) + + if err != nil { + return fmt.Errorf("Error encoding metadata to json: %s", err) + } + + err = devices.writeMetaFile(jsonData, devices.deviceSetMetaFile()) + if err != nil { + return err + } + + return nil +} + func (devices *DeviceSet) initDevmapper(doInit bool) error { logInit(devices) @@ -676,6 +708,10 @@ func (devices *DeviceSet) initDevmapper(doInit bool) error { } } + // Right now this loads only NextDeviceId. If there is more metatadata + // down the line, we might have to move it earlier. + devices.loadDeviceSetMetaData() + // Setup the base image if doInit { if err := devices.setupBaseImage(); err != nil { @@ -955,6 +991,8 @@ func (devices *DeviceSet) Shutdown() error { if err := devices.deactivatePool(); err != nil { log.Debugf("Shutdown deactivate pool , error: %s", err) } + + devices.saveDeviceSetMetaData() devices.Unlock() return nil From ff56531de47c08157b2a37e6c6b6189a5006dba2 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Wed, 5 Nov 2014 14:39:54 -0500 Subject: [PATCH 4/6] devmapper: Fix gofmt related build failures My pull request failed the build due to gofmat issues. I have run gofmt on specified files and this commit fixes it. Signed-off-by: Vivek Goyal --- daemon/graphdriver/devmapper/deviceset.go | 34 +++++++++++------------ 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go index b193480459..e661dbb7ad 100644 --- a/daemon/graphdriver/devmapper/deviceset.go +++ b/daemon/graphdriver/devmapper/deviceset.go @@ -62,25 +62,25 @@ type MetaData struct { } type DeviceSet struct { - MetaData `json:"-"` - sync.Mutex `json:"-"` // Protects Devices map and serializes calls into libdevmapper - root string `json:"-"` - devicePrefix string `json:"-"` - TransactionId uint64 `json:"-"` - NewTransactionId uint64 `json:"-"` - NextDeviceId int `json:"next_device_id"` + MetaData `json:"-"` + sync.Mutex `json:"-"` // Protects Devices map and serializes calls into libdevmapper + root string `json:"-"` + devicePrefix string `json:"-"` + TransactionId uint64 `json:"-"` + NewTransactionId uint64 `json:"-"` + NextDeviceId int `json:"next_device_id"` // Options - dataLoopbackSize int64 `json:"-"` - metaDataLoopbackSize int64 `json:"-"` - baseFsSize uint64 `json:"-"` - filesystem string `json:"-"` - mountOptions string `json:"-"` - mkfsArgs []string `json:"-"` - dataDevice string `json:"-"` - metadataDevice string `json:"-"` - doBlkDiscard bool `json:"-"` - thinpBlockSize uint32 `json:"-"` + dataLoopbackSize int64 `json:"-"` + metaDataLoopbackSize int64 `json:"-"` + baseFsSize uint64 `json:"-"` + filesystem string `json:"-"` + mountOptions string `json:"-"` + mkfsArgs []string `json:"-"` + dataDevice string `json:"-"` + metadataDevice string `json:"-"` + doBlkDiscard bool `json:"-"` + thinpBlockSize uint32 `json:"-"` } type DiskUsage struct { From 0f57c902450b1d4f7a676dc693689debca002e98 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Thu, 6 Nov 2014 15:59:25 -0500 Subject: [PATCH 5/6] docker-remove-redundant-json-tags In previous patch I had introduce json:"-" tags to be on safer side to make sure certain fields are not marshalled/unmarshalled. But struct fields starting with small letter are not exported so they will not be marshalled anyway. So remove json:"-" tags from there. Signed-off-by: Vivek Goyal --- daemon/graphdriver/devmapper/deviceset.go | 30 +++++++++++------------ 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go index e661dbb7ad..59c5ec82e7 100644 --- a/daemon/graphdriver/devmapper/deviceset.go +++ b/daemon/graphdriver/devmapper/deviceset.go @@ -64,23 +64,23 @@ type MetaData struct { type DeviceSet struct { MetaData `json:"-"` sync.Mutex `json:"-"` // Protects Devices map and serializes calls into libdevmapper - root string `json:"-"` - devicePrefix string `json:"-"` - TransactionId uint64 `json:"-"` - NewTransactionId uint64 `json:"-"` - NextDeviceId int `json:"next_device_id"` + root string + devicePrefix string + TransactionId uint64 `json:"-"` + NewTransactionId uint64 `json:"-"` + NextDeviceId int `json:"next_device_id"` // Options - dataLoopbackSize int64 `json:"-"` - metaDataLoopbackSize int64 `json:"-"` - baseFsSize uint64 `json:"-"` - filesystem string `json:"-"` - mountOptions string `json:"-"` - mkfsArgs []string `json:"-"` - dataDevice string `json:"-"` - metadataDevice string `json:"-"` - doBlkDiscard bool `json:"-"` - thinpBlockSize uint32 `json:"-"` + dataLoopbackSize int64 + metaDataLoopbackSize int64 + baseFsSize uint64 + filesystem string + mountOptions string + mkfsArgs []string + dataDevice string + metadataDevice string + doBlkDiscard bool + thinpBlockSize uint32 } type DiskUsage struct { From 15c74bebc1ea2d51612b5809b4477551547a8b3d Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Fri, 7 Nov 2014 16:53:34 -0500 Subject: [PATCH 6/6] devmapper: Take care of some review comments Took care of some review comments from crosbymichael. v2: - Return "err = nil" if file deviceset-metadata file does not exist. - Use json.Decoder() interface for loading deviceset metadata. v3: - Reverted back to json marshal interface in loadDeviceSetMetaData(). Signed-off-by: Vivek Goyal --- daemon/graphdriver/devmapper/deviceset.go | 32 +++++++++++------------ 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/daemon/graphdriver/devmapper/deviceset.go b/daemon/graphdriver/devmapper/deviceset.go index 59c5ec82e7..125483f6c2 100644 --- a/daemon/graphdriver/devmapper/deviceset.go +++ b/daemon/graphdriver/devmapper/deviceset.go @@ -32,6 +32,8 @@ var ( DefaultThinpBlockSize uint32 = 128 // 64K = 128 512b sectors ) +const deviceSetMetaFile string = "deviceset-metadata" + type DevInfo struct { Hash string `json:"-"` DeviceId int `json:"device_id"` @@ -139,7 +141,7 @@ func (devices *DeviceSet) metadataFile(info *DevInfo) string { } func (devices *DeviceSet) deviceSetMetaFile() string { - return path.Join(devices.metadataDir(), "deviceset-metadata") + return path.Join(devices.metadataDir(), deviceSetMetaFile) } func (devices *DeviceSet) oldMetadataFile() string { @@ -236,8 +238,7 @@ func (devices *DeviceSet) saveMetadata(info *DevInfo) error { if err != nil { return fmt.Errorf("Error encoding metadata to json: %s", err) } - err = devices.writeMetaFile(jsonData, devices.metadataFile(info)) - if err != nil { + if err := devices.writeMetaFile(jsonData, devices.metadataFile(info)); err != nil { return err } @@ -552,29 +553,24 @@ func (devices *DeviceSet) ResizePool(size int64) error { func (devices *DeviceSet) loadDeviceSetMetaData() error { jsonData, err := ioutil.ReadFile(devices.deviceSetMetaFile()) if err != nil { - return nil + // For backward compatibility return success if file does + // not exist. + if os.IsNotExist(err) { + return nil + } + return err } - if err := json.Unmarshal(jsonData, devices); err != nil { - return nil - } - - return nil + return json.Unmarshal(jsonData, devices) } func (devices *DeviceSet) saveDeviceSetMetaData() error { jsonData, err := json.Marshal(devices) - if err != nil { return fmt.Errorf("Error encoding metadata to json: %s", err) } - err = devices.writeMetaFile(jsonData, devices.deviceSetMetaFile()) - if err != nil { - return err - } - - return nil + return devices.writeMetaFile(jsonData, devices.deviceSetMetaFile()) } func (devices *DeviceSet) initDevmapper(doInit bool) error { @@ -710,7 +706,9 @@ func (devices *DeviceSet) initDevmapper(doInit bool) error { // Right now this loads only NextDeviceId. If there is more metatadata // down the line, we might have to move it earlier. - devices.loadDeviceSetMetaData() + if err = devices.loadDeviceSetMetaData(); err != nil { + return err + } // Setup the base image if doInit {