Reduce system calls

PATCH New Zdenek Kabelac 1 participant 8 messages Nov 25, 2011

Share & tools

Zdenek Kabelac

author

2011-11-25 09:59 UTC #0 lore raw

cover Reduce system calls

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

Zdenek Kabelac

author

2011-11-25 09:59 UTC #1 lore raw

PATCH 1/7 Add common initialization code for struct device

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

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/device/dev-cache.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

lib/device/dev-cache.c +17 ~~-17~~

index 3c44333..22e5cce 100644
@@ -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;
 }

Signature · 1 line

1.7.7.3

Zdenek Kabelac

author

2011-11-25 09:59 UTC #2 lore raw

PATCH 2/7 Add caching of BLKGETSIZE64 ioctl

In reply to Zdenek Kabelac

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

Signed-off-by Zdenek Kabelac <[email protected]>

---
 lib/device/dev-cache.c |    1 +
 lib/device/dev-io.c    |   27 ++++++++++++++++-----------
 lib/device/device.h    |    1 +
 3 files changed, 18 insertions(+), 11 deletions(-)

lib/device/dev-cache.c +1 -0

index 22e5cce..61f479d 100644
@@ -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;

lib/device/dev-io.c +16 ~~-11~~

index 9b486a9..46b90b5 100644
@@ -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);

lib/device/device.h +1 -0

index 8c32a03..1d6d27d 100644
@@ -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];

Signature · 1 line

1.7.7.3

Zdenek Kabelac

author

2011-11-25 09:59 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 lib/device/dev-io.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

lib/device/dev-io.c +0 -1

index 46b90b5..bf30fe0 100644
@@ -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));

Signature · 1 line

1.7.7.3

Zdenek Kabelac

author

2011-11-25 09:59 UTC #4 lore raw

PATCH 4/7 Do not lstat common path prefix

In reply to 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 <[email protected]>

---
 lib/device/dev-cache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

lib/device/dev-cache.c +2 -0

index 61f479d..9b401ee 100644
@@ -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.

Signature · 1 line

1.7.7.3

Zdenek Kabelac

author

2011-11-25 09:59 UTC #5 lore raw

PATCH 5/7 Minor cleanup of strchr analyzer warning

In reply to 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 <[email protected]>

---
 lib/device/dev-cache.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

lib/device/dev-cache.c +8 ~~-10~~

index 9b401ee..161bff7 100644
@@ -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 */

Signature · 1 line

1.7.7.3

Zdenek Kabelac

author

2011-11-25 09:59 UTC #6 lore raw

PATCH 6/7 Drop extra stat before open of device

In reply to 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 <[email protected]>

---
 lib/device/dev-io.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

lib/device/dev-io.c +0 ~~-11~~

index bf30fe0..950e197 100644
@@ -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))

Signature · 1 line

1.7.7.3

Zdenek Kabelac

author

2011-11-25 09:59 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 lib/device/dev-cache.c |   48 ++++++++++++++++++++++++++++++------------------
 1 files changed, 30 insertions(+), 18 deletions(-)

lib/device/dev-cache.c +30 ~~-18~~

index 161bff7..9abaf1c 100644
@@ -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);

Signature · 1 line

1.7.7.3