From 9e6b1852a78eda6ed2cb255d6be8a0d0e5a5ca40 Mon Sep 17 00:00:00 2001
From: Brian Goff <cpuguy83@gmail.com>
Date: Tue, 12 Apr 2016 17:09:55 -0400
Subject: [PATCH] Fix N+1 calling `Path()` on `volume ls`

Implements a `CachedPath` function on the volume plugin adapter that we
call from the volume list function instead of `Path.
If a driver does not implement `CachedPath` it will just call `Path`.

Also makes sure we store the path on Mount and remove the path on
Unmount.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
---
 daemon/create.go                              |  4 +++-
 daemon/inspect.go                             |  4 +++-
 daemon/list.go                                | 10 +++++++++-
 daemon/volumes.go                             |  5 ++---
 ...ocker_cli_start_volume_driver_unix_test.go | 19 +++++++++++++++++++
 volume/drivers/adapter.go                     | 17 ++++++++++++-----
 6 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/daemon/create.go b/daemon/create.go
index 18203fe2b4..e659ec526d 100644
--- a/daemon/create.go
+++ b/daemon/create.go
@@ -183,5 +183,7 @@ func (daemon *Daemon) VolumeCreate(name, driverName string, opts, labels map[str
 	}
 
 	daemon.LogVolumeEvent(v.Name(), "create", map[string]string{"driver": v.DriverName()})
-	return volumeToAPIType(v), nil
+	apiV := volumeToAPIType(v)
+	apiV.Mountpoint = v.Path()
+	return apiV, nil
 }
diff --git a/daemon/inspect.go b/daemon/inspect.go
index 2f810e3e86..4f497327bd 100644
--- a/daemon/inspect.go
+++ b/daemon/inspect.go
@@ -204,7 +204,9 @@ func (daemon *Daemon) VolumeInspect(name string) (*types.Volume, error) {
 	if err != nil {
 		return nil, err
 	}
-	return volumeToAPIType(v), nil
+	apiV := volumeToAPIType(v)
+	apiV.Mountpoint = v.Path()
+	return apiV, nil
 }
 
 func (daemon *Daemon) getBackwardsCompatibleNetworkSettings(settings *network.Settings) *v1p20.NetworkSettings {
diff --git a/daemon/list.go b/daemon/list.go
index c29eaadb42..e0ce5c20f3 100644
--- a/daemon/list.go
+++ b/daemon/list.go
@@ -491,7 +491,15 @@ func (daemon *Daemon) Volumes(filter string) ([]*types.Volume, []string, error)
 		return nil, nil, err
 	}
 	for _, v := range filterVolumes {
-		volumesOut = append(volumesOut, volumeToAPIType(v))
+		apiV := volumeToAPIType(v)
+		if vv, ok := v.(interface {
+			CachedPath() string
+		}); ok {
+			apiV.Mountpoint = vv.CachedPath()
+		} else {
+			apiV.Mountpoint = v.Path()
+		}
+		volumesOut = append(volumesOut, apiV)
 	}
 	return volumesOut, warnings, nil
 }
diff --git a/daemon/volumes.go b/daemon/volumes.go
index 37f4e7fa9f..b38fd50631 100644
--- a/daemon/volumes.go
+++ b/daemon/volumes.go
@@ -25,9 +25,8 @@ type mounts []container.Mount
 // volumeToAPIType converts a volume.Volume to the type used by the remote API
 func volumeToAPIType(v volume.Volume) *types.Volume {
 	tv := &types.Volume{
-		Name:       v.Name(),
-		Driver:     v.DriverName(),
-		Mountpoint: v.Path(),
+		Name:   v.Name(),
+		Driver: v.DriverName(),
 	}
 	if v, ok := v.(interface {
 		Labels() map[string]string
diff --git a/integration-cli/docker_cli_start_volume_driver_unix_test.go b/integration-cli/docker_cli_start_volume_driver_unix_test.go
index cc5d0b11e6..794980f69a 100644
--- a/integration-cli/docker_cli_start_volume_driver_unix_test.go
+++ b/integration-cli/docker_cli_start_volume_driver_unix_test.go
@@ -441,3 +441,22 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverGetEmptyResponse(c *
 	c.Assert(err, checker.NotNil, check.Commentf(out))
 	c.Assert(out, checker.Contains, "No such volume")
 }
+
+// Ensure only cached paths are used in volume list to prevent N+1 calls to `VolumeDriver.Path`
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverPathCalls(c *check.C) {
+	c.Assert(s.d.Start(), checker.IsNil)
+	c.Assert(s.ec.paths, checker.Equals, 0)
+
+	out, err := s.d.Cmd("volume", "create", "--name=test", "--driver=test-external-volume-driver")
+	c.Assert(err, checker.IsNil, check.Commentf(out))
+	c.Assert(s.ec.paths, checker.Equals, 1)
+
+	out, err = s.d.Cmd("volume", "ls")
+	c.Assert(err, checker.IsNil, check.Commentf(out))
+	c.Assert(s.ec.paths, checker.Equals, 1)
+
+	out, err = s.d.Cmd("volume", "inspect", "--format='{{.Mountpoint}}'", "test")
+	c.Assert(err, checker.IsNil, check.Commentf(out))
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
+	c.Assert(s.ec.paths, checker.Equals, 1)
+}
diff --git a/volume/drivers/adapter.go b/volume/drivers/adapter.go
index e7ca3d5034..1377ff3605 100644
--- a/volume/drivers/adapter.go
+++ b/volume/drivers/adapter.go
@@ -88,11 +88,14 @@ func (a *volumeAdapter) DriverName() string {
 }
 
 func (a *volumeAdapter) Path() string {
-	if len(a.eMount) > 0 {
-		return a.eMount
+	if len(a.eMount) == 0 {
+		a.eMount, _ = a.proxy.Path(a.name)
 	}
-	m, _ := a.proxy.Path(a.name)
-	return m
+	return a.eMount
+}
+
+func (a *volumeAdapter) CachedPath() string {
+	return a.eMount
 }
 
 func (a *volumeAdapter) Mount() (string, error) {
@@ -102,5 +105,9 @@ func (a *volumeAdapter) Mount() (string, error) {
 }
 
 func (a *volumeAdapter) Unmount() error {
-	return a.proxy.Unmount(a.name)
+	err := a.proxy.Unmount(a.name)
+	if err == nil {
+		a.eMount = ""
+	}
+	return err
 }