[PATCH 0/7] Reduce system calls

[PATCH 0/7] Reduce system calls

[Zdenek Kabelac]

Seems like over the time we have added lots of system calls
which are not fully using the device cache or the code is 
just distrustful too much.

This patch set tries to reduce the number of system calls
and should have some positive impact on RHZB 744926.

In my measurements it seems like it's giving extra 10%-30% speedup
of processing commands like lvcreate with larger set of devices in system.
(depends on the lvm.conf configuration as well.)

!!! NOTICE !!!

It's worth to note that our current code for obtaining device list from
udev is upto 80% slower that our normal scan (yep, udev is quite slow and
does about ~8 syscalls per device) so unless we try to extend this udev 
scan with just check only for real PV devices (which has been reject)
we have quite noticable speed regression when larger set of devices
is present in the system.

Patch set should be back-portable.

Careful review for '*' please.

Zdenek Kabelac (7):
  Add common initialization code for struct device
* Add caching of BLKGETSIZE64 ioctl
* Do not drop cache  block_size for cached device.
* Do not lstat common path prefix
  Minor cleanup of strchr analyzer warning
* Drop extra stat before open of device
  Skip double stat() call with each _insert

 lib/device/dev-cache.c |  103 +++++++++++++++++++++++++++---------------------
 lib/device/dev-io.c    |   39 +++++++-----------
 lib/device/device.h    |    1 +
 3 files changed, 75 insertions(+), 68 deletions(-)

--
1.7.7.3

[PATCH 1/7] Add common initialization code for struct device

[Zdenek Kabelac]

Avoid duplicate code and add _dev_init() where all common
member values are initialized.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/device/dev-cache.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/lib/device/dev-cache.c b/lib/device/dev-cache.c
index 3c44333..22e5cce 100644
--- a/lib/device/dev-cache.c
+++ b/lib/device/dev-cache.c
@@ -54,6 +54,19 @@ static struct {
 
 static int _insert(const char *path, int rec, int check_with_udev_db);
 
+static void _dev_init(struct device *dev)
+{
+	dev->block_size = -1;
+	dev->end = UINT64_C(0);
+	dev->fd = -1;
+	dev->max_error_count = dev_disable_after_error_count();
+	dev->open_count = 0;
+	dev->read_ahead = -1;
+	memset(dev->pvid, 0, sizeof(dev->pvid));
+	dm_list_init(&dev->aliases);
+	dm_list_init(&dev->open_list);
+}
+
 struct device *dev_create_file(const char *filename, struct device *dev,
 			       struct str_list *alias, int use_malloc)
 {
@@ -97,19 +110,12 @@ struct device *dev_create_file(const char *filename, struct device *dev,
 		return NULL;
 	}
 
-	dev->flags |= DEV_REGULAR;
-	dm_list_init(&dev->aliases);
+	_dev_init(dev);
+	dev->flags = DEV_REGULAR;
 	dm_list_add(&dev->aliases, &alias->list);
-	dev->end = UINT64_C(0);
 	dev->dev = 0;
-	dev->fd = -1;
-	dev->open_count = 0;
 	dev->error_count = 0;
 	dev->max_error_count = NO_DEV_ERROR_COUNT_LIMIT;
-	dev->block_size = -1;
-	dev->read_ahead = -1;
-	memset(dev->pvid, 0, sizeof(dev->pvid));
-	dm_list_init(&dev->open_list);
 
 	return dev;
 }
@@ -122,17 +128,11 @@ static struct device *_dev_create(dev_t d)
 		log_error("struct device allocation failed");
 		return NULL;
 	}
+
+	_dev_init(dev);
 	dev->flags = 0;
-	dm_list_init(&dev->aliases);
 	dev->dev = d;
-	dev->fd = -1;
-	dev->open_count = 0;
 	dev->max_error_count = dev_disable_after_error_count();
-	dev->block_size = -1;
-	dev->read_ahead = -1;
-	dev->end = UINT64_C(0);
-	memset(dev->pvid, 0, sizeof(dev->pvid));
-	dm_list_init(&dev->open_list);
 
 	return dev;
 }
-- 
1.7.7.3

[PATCH 2/7] Add caching of BLKGETSIZE64 ioctl

[Zdenek Kabelac]

Do not open/ioctl/close same device multiple times,
when its in device cache.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/device/dev-cache.c |    1 +
 lib/device/dev-io.c    |   27 ++++++++++++++++-----------
 lib/device/device.h    |    1 +
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/lib/device/dev-cache.c b/lib/device/dev-cache.c
index 22e5cce..61f479d 100644
--- a/lib/device/dev-cache.c
+++ b/lib/device/dev-cache.c
@@ -56,6 +56,7 @@ static int _insert(const char *path, int rec, int check_with_udev_db);
 
 static void _dev_init(struct device *dev)
 {
+	dev->blkgetsize64 = -1;
 	dev->block_size = -1;
 	dev->end = UINT64_C(0);
 	dev->fd = -1;
diff --git a/lib/device/dev-io.c b/lib/device/dev-io.c
index 9b486a9..46b90b5 100644
--- a/lib/device/dev-io.c
+++ b/lib/device/dev-io.c
@@ -252,21 +252,26 @@ static int _dev_get_size_dev(const struct device *dev, uint64_t *size)
 	int fd;
 	const char *name = dev_name(dev);
 
-	if ((fd = open(name, O_RDONLY)) < 0) {
-		log_sys_error("open", name);
-		return 0;
-	}
+	if (dev->blkgetsize64 == -1) {
+		if ((fd = open(name, O_RDONLY)) < 0) {
+			log_sys_error("open", name);
+			return 0;
+		}
+
+		if (ioctl(fd, BLKGETSIZE64, &dev->blkgetsize64) < 0) {
+			log_sys_error("ioctl BLKGETSIZE64", name);
+			if (close(fd))
+				log_sys_error("close", name);
+			return 0;
+		}
 
-	if (ioctl(fd, BLKGETSIZE64, size) < 0) {
-		log_sys_error("ioctl BLKGETSIZE64", name);
 		if (close(fd))
 			log_sys_error("close", name);
-		return 0;
-	}
 
-	*size >>= BLKSIZE_SHIFT;	/* Convert to sectors */
-	if (close(fd))
-		log_sys_error("close", name);
+	} else
+		log_debug("Using cached size for device %s.", name);
+
+	*size = dev->blkgetsize64 >> BLKSIZE_SHIFT;  /* Convert to sectors */
 
 	log_very_verbose("%s: size is %" PRIu64 " sectors", name, *size);
 
diff --git a/lib/device/device.h b/lib/device/device.h
index 8c32a03..1d6d27d 100644
--- a/lib/device/device.h
+++ b/lib/device/device.h
@@ -45,6 +45,7 @@ struct device {
 	int read_ahead;
 	uint32_t flags;
 	uint64_t end;
+	uint64_t blkgetsize64;
 	struct dm_list open_list;
 
 	char pvid[ID_LEN + 1];
-- 
1.7.7.3

[PATCH 3/7] Do not drop cache block_size for cached device.

[Zdenek Kabelac]

When we read i.e. read_ahead value - we open/close device,
which will cause dropping and rechecking device for the
block_size value.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/device/dev-io.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/lib/device/dev-io.c b/lib/device/dev-io.c
index 46b90b5..bf30fe0 100644
--- a/lib/device/dev-io.c
+++ b/lib/device/dev-io.c
@@ -587,7 +587,6 @@ static void _close(struct device *dev)
 	if (close(dev->fd))
 		log_sys_error("close", dev_name(dev));
 	dev->fd = -1;
-	dev->block_size = -1;
 	dm_list_del(&dev->open_list);
 
 	log_debug("Closed %s", dev_name(dev));
-- 
1.7.7.3

[PATCH 4/7] Do not lstat common path prefix

[Zdenek Kabelac]

When both path have common prefix i.e. /dev/disk/by-id
skip  2xlstat for /dev  /dev/disk /dev/disk/by-id
and directly lstat only different part of path.

Reduces greatly amount of lstat calls on system with
lots of devices.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/device/dev-cache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/lib/device/dev-cache.c b/lib/device/dev-cache.c
index 61f479d..9b401ee 100644
--- a/lib/device/dev-cache.c
+++ b/lib/device/dev-cache.c
@@ -266,6 +266,8 @@ static int _compare_paths(const char *path0, const char *path1)
 	strncpy(p1, path1, PATH_MAX);
 	s0 = &p0[0] + 1;
 	s1 = &p1[0] + 1;
+	while (*s0 && *s0 == *s1)
+                s0++, s1++;
 
 	/* We prefer symlinks - they exist for a reason!
 	 * So we prefer a shorter path before the first symlink in the name.
-- 
1.7.7.3

[PATCH 5/7] Minor cleanup of strchr analyzer warning

[Zdenek Kabelac]

While the code knows, path0 and path1 has the same amount of '/'
otherwise the shorter path wins - the static analyzer is confused.
So just add extra check also for 's1' strchr to clean away warning
about unchecked strchr().

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/device/dev-cache.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/lib/device/dev-cache.c b/lib/device/dev-cache.c
index 9b401ee..161bff7 100644
--- a/lib/device/dev-cache.c
+++ b/lib/device/dev-cache.c
@@ -264,8 +264,8 @@ static int _compare_paths(const char *path0, const char *path1)
 
 	strncpy(p0, path0, PATH_MAX);
 	strncpy(p1, path1, PATH_MAX);
-	s0 = &p0[0] + 1;
-	s1 = &p1[0] + 1;
+	s0 = p0 + 1;
+	s1 = p1 + 1;
 	while (*s0 && *s0 == *s1)
                 s0++, s1++;
 
@@ -273,12 +273,11 @@ static int _compare_paths(const char *path0, const char *path1)
 	 * So we prefer a shorter path before the first symlink in the name.
 	 * FIXME Configuration option to invert this? */
 	while (s0) {
-		s0 = strchr(s0, '/');
-		s1 = strchr(s1, '/');
-		if (s0) {
+		if ((s0 = strchr(s0, '/')))
 			*s0 = '\0';
+		if ((s1 = strchr(s1, '/')))
 			*s1 = '\0';
-		}
+
 		if (lstat(p0, &stat0)) {
 			log_sys_very_verbose("lstat", p0);
 			return 1;
@@ -291,10 +290,9 @@ static int _compare_paths(const char *path0, const char *path1)
 			return 0;
 		if (!S_ISLNK(stat0.st_mode) && S_ISLNK(stat1.st_mode))
 			return 1;
-		if (s0) {
-			*s0++ = '/';
-			*s1++ = '/';
-		}
+
+		if (s0) *s0++ = '/';
+		if (s1) *s1++ = '/';
 	}
 
 	/* ASCII comparison */
-- 
1.7.7.3

[PATCH 6/7] Drop extra stat before open of device

[Zdenek Kabelac]

Use the cached value of stat() and dropping extra call for 'stat()'
before calling 'open()' and expect, that if device has been changed,
it would be catched with the 'fstat()' call that follows the 'open()'
call. If the device has dissappered 'open()' will fail.

This safes one extra 'stat()' call per each device.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/device/dev-io.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/lib/device/dev-io.c b/lib/device/dev-io.c
index bf30fe0..950e197 100644
--- a/lib/device/dev-io.c
+++ b/lib/device/dev-io.c
@@ -454,17 +454,6 @@ int dev_open_flags(struct device *dev, int flags, int direct, int quiet)
 	else if (!(name = dev_name_confirmed(dev, quiet)))
 		return_0;
 
-	if (!(dev->flags & DEV_REGULAR)) {
-		if (stat(name, &buf) < 0) {
-			log_sys_error("%s: stat failed", name);
-			return 0;
-		}
-		if (buf.st_rdev != dev->dev) {
-			log_error("%s: device changed", name);
-			return 0;
-		}
-	}
-
 #ifdef O_DIRECT_SUPPORT
 	if (direct) {
 		if (!(dev->flags & DEV_O_DIRECT_TESTED))
-- 
1.7.7.3

[PATCH 7/7] Skip double stat() call with each _insert

[Zdenek Kabelac]

When the device is inserted in dev_name_confirmed() stat() is
called twice as _insert() has it's own stat() call.

Extend _insert() parameter with struct stat* - which could be reused
if it has been just obtained.  Udev path passes NULL and code is
doing then its own stat() as before.

Signed-off-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 lib/device/dev-cache.c |   48 ++++++++++++++++++++++++++++++------------------
 1 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/lib/device/dev-cache.c b/lib/device/dev-cache.c
index 161bff7..9abaf1c 100644
--- a/lib/device/dev-cache.c
+++ b/lib/device/dev-cache.c
@@ -52,7 +52,8 @@ static struct {
 #define _free(x) dm_pool_free(_cache.mem, (x))
 #define _strdup(x) dm_pool_strdup(_cache.mem, (x))
 
-static int _insert(const char *path, int rec, int check_with_udev_db);
+static int _insert(const char *path, const struct stat *info,
+		   int rec, int check_with_udev_db);
 
 static void _dev_init(struct device *dev)
 {
@@ -443,7 +444,7 @@ static int _insert_dir(const char *dir)
 				return_0;
 
 			_collapse_slashes(path);
-			r &= _insert(path, 1, 0);
+			r &= _insert(path, NULL, 1, 0);
 			dm_free(path);
 
 			free(dirent[n]);
@@ -511,11 +512,11 @@ static int _insert_udev_dir(struct udev *udev, const char *dir)
 		device = udev_device_new_from_syspath(udev, udev_list_entry_get_name(device_entry));
 
 		node_name = udev_device_get_devnode(device);
-		r &= _insert(node_name, 0, 0);
+		r &= _insert(node_name, NULL, 0, 0);
 
 		udev_list_entry_foreach(symlink_entry, udev_device_get_devlinks_list_entry(device)) {
 			symlink_name = udev_list_entry_get_name(symlink_entry);
-			r &= _insert(symlink_name, 0, 0);
+			r &= _insert(symlink_name, NULL, 0, 0);
 		}
 
 		udev_device_unref(device);
@@ -569,29 +570,33 @@ static void _insert_dirs(struct dm_list *dirs)
 
 #endif	/* UDEV_SYNC_SUPPORT */
 
-static int _insert(const char *path, int rec, int check_with_udev_db)
+static int _insert(const char *path, const struct stat *info,
+		   int rec, int check_with_udev_db)
 {
-	struct stat info;
+	struct stat tinfo;
 	int r = 0;
 
-	if (stat(path, &info) < 0) {
-		log_sys_very_verbose("stat", path);
-		return 0;
+	if (!info) {
+		if (stat(path, &tinfo) < 0) {
+			log_sys_very_verbose("stat", path);
+			return 0;
+		}
+		info = &tinfo;
 	}
 
-	if (check_with_udev_db && !_device_in_udev_db(info.st_rdev)) {
+	if (check_with_udev_db && !_device_in_udev_db(info->st_rdev)) {
 		log_very_verbose("%s: Not in udev db", path);
 		return 0;
 	}
 
-	if (S_ISDIR(info.st_mode)) {	/* add a directory */
+	if (S_ISDIR(info->st_mode)) {	/* add a directory */
 		/* check it's not a symbolic link */
-		if (lstat(path, &info) < 0) {
+		if (lstat(path, &tinfo) < 0) {
 			log_sys_very_verbose("lstat", path);
 			return 0;
 		}
 
-		if (S_ISLNK(info.st_mode)) {
+		if (S_ISLNK(tinfo.st_mode)) {
 			log_debug("%s: Symbolic link to directory", path);
 			return 0;
 		}
@@ -600,12 +605,12 @@ static int _insert(const char *path, int rec, int check_with_udev_db)
 			r = _insert_dir(path);
 
 	} else {		/* add a device */
-		if (!S_ISBLK(info.st_mode)) {
+		if (!S_ISBLK(info->st_mode)) {
 			log_debug("%s: Not a block device", path);
 			return 0;
 		}
 
-		if (!_insert_dev(path, info.st_rdev))
+		if (!_insert_dev(path, info->st_rdev))
 			return_0;
 
 		r = 1;
@@ -865,7 +870,7 @@ const char *dev_name_confirmed(struct device *dev, int quiet)
 		if (dm_list_size(&dev->aliases) > 1) {
 			dm_list_del(dev->aliases.n);
 			if (!r)
-				_insert(name, 0, obtain_device_list_from_udev());
+				_insert(name, &buf, 0, obtain_device_list_from_udev());
 			continue;
 		}
 
@@ -887,13 +892,20 @@ struct device *dev_cache_get(const char *name, struct dev_filter *f)
 		return d;
 
 	/* If the entry's wrong, remove it */
-	if (d && (stat(name, &buf) || (buf.st_rdev != d->dev))) {
+	if (stat(name, &buf) < 0) {
+		if (d)
+			dm_hash_remove(_cache.names, name);
+		log_sys_very_verbose("stat", name);
+		return NULL;
+	}
+
+	if (d && (buf.st_rdev != d->dev)) {
 		dm_hash_remove(_cache.names, name);
 		d = NULL;
 	}
 
 	if (!d) {
-		_insert(name, 0, obtain_device_list_from_udev());
+		_insert(name, &buf, 0, obtain_device_list_from_udev());
 		d = (struct device *) dm_hash_lookup(_cache.names, name);
 		if (!d) {
 			_full_scan(0);
-- 
1.7.7.3