Nir Soffer has uploaded a new change for review. Change subject: fileUtils: Remove support for direct io on memory filesystems ......................................................................
fileUtils: Remove support for direct io on memory filesystems In commits e8f3848d12 and 127936c994, we added support for memory based filesystems such as tempfs and ramfs. This allows direct io tests to work when using temporary files in /tmp. These patches disabled direct io when reading or writing to such filesystems. Turns out that the code for checking file system type, required for these changes, has O(N^2) complexity, and profiles we did in the field on RHEV-H systems with many storage domains show that this code is major performance issue. We suspect that this issue is related to several other bugs and customer cases. Further, code using direct io, such as DirectFile, shouldn't try to outsmart the caller, disabling direct io magically. The test writer should use the appropriate filesystem for such tests. This patch remove the support for memory filesystems, and use current directory when testing code that uses direct io. I tested disabling memory filesystems on two setups. The first was a single customer RHEV-H machine running 3.3 (test performed by customer), and the second 2 Fedora 19 hosts running master. Both systems are connected to 30 storage domains, are idle and have no vms running. On the RHEV-H machine, after disabling memory filesystems, average cpu usage dropped from 240% to 17%. On the Fedora systems, average cpu usage dropped from 10% to 5% when running as spm, and from 5% to 3% when running as hsm. Change-Id: I7b652b5c78ff8b48a39df2dc57c02496bec666a1 Bug-Url: https://bugzilla.redhat.com/1090664 Relates-to: https://bugzilla.redhat.com/1074097 Relates-to: https://bugzilla.redhat.com/1081962 Relates-to: http://gerrit.ovirt.org/9595 Relates-to: http://gerrit.ovirt.org/9661 Signed-off-by: Nir Soffer <[email protected]> --- M tests/fileUtilTests.py M tests/miscTests.py M tests/testrunner.py M vdsm/storage/fileUtils.py M vdsm/storage/misc.py M vdsm/storage/mount.py 6 files changed, 20 insertions(+), 60 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/41/27441/1 diff --git a/tests/fileUtilTests.py b/tests/fileUtilTests.py index 39d1da9..c3f4b03 100644 --- a/tests/fileUtilTests.py +++ b/tests/fileUtilTests.py @@ -35,7 +35,7 @@ platea lacus morbi nisl montes ve. Ac. A, consectetuer erat, justo eu. Elementum et, phasellus fames et rutrum donec magnis eu bibendum. Arcu, ante aliquam ipsum ut facilisis ad.""" - with temporaryPath(data=data) as srcPath: + with temporaryPath(data=data, dir='.') as srcPath: # two nested withs to be py2.6 friendly. with fileUtils.open_ex(srcPath, "dr") as f: self.assertEquals(f.read(), data) @@ -58,7 +58,7 @@ quam. """ self.assertTrue(len(data) > 512) - with temporaryPath(data=data) as srcPath: + with temporaryPath(data=data, dir='.') as srcPath: # two nested withs to be py2.6 friendly. with fileUtils.open_ex(srcPath, "dr") as f: f.seek(512) @@ -69,7 +69,7 @@ suscipit nec integer sociosqu. Fermentum. Ante orci luctus, ipsum ullamcorper enim arcu class neque inceptos class. Ut, sagittis torquent, commodo facilisi.""" - with temporaryPath() as srcPath: + with temporaryPath(dir='.') as srcPath: with fileUtils.open_ex(srcPath, "dw") as f: f.write(data) with fileUtils.open_ex(srcPath, "r") as f: @@ -98,7 +98,7 @@ """ self.assertTrue(len(data) > 512) - with temporaryPath() as srcPath: + with temporaryPath(dir='.') as srcPath: with fileUtils.open_ex(srcPath, "dw") as f: f.write(data[:512]) f.write(data[512:]) @@ -124,7 +124,7 @@ """ self.assertTrue(len(data) > 512) - with temporaryPath() as srcPath: + with temporaryPath(dir='.') as srcPath: with fileUtils.open_ex(srcPath, "wd") as f: f.write(data[:512]) diff --git a/tests/miscTests.py b/tests/miscTests.py index 0f3caf2..70abe62 100644 --- a/tests/miscTests.py +++ b/tests/miscTests.py @@ -379,8 +379,8 @@ if (len(data) % 512) == 0: data += "!" - with temporaryPath(perms=0o666, data=data) as srcPath: - with temporaryPath(perms=0o666) as dstPath: + with temporaryPath(perms=0o666, data=data, dir='.') as srcPath: + with temporaryPath(perms=0o666, dir='.') as dstPath: # Copy rc, out, err = misc.ddWatchCopy(srcPath, dstPath, None, len(data)) @@ -392,7 +392,7 @@ self.assertEquals(readData, data) def _createDataFile(self, data, repetitions): - fd, path = tempfile.mkstemp() + fd, path = tempfile.mkstemp(dir='.') try: for i in xrange(repetitions): @@ -471,8 +471,8 @@ # Makes sure we round up to a complete block size data *= 512 - with temporaryPath(perms=0o666, data=data) as srcPath: - with temporaryPath(perms=0o666) as dstPath: + with temporaryPath(perms=0o666, data=data, dir='.') as srcPath: + with temporaryPath(perms=0o666, dir='.') as dstPath: # Copy rc, out, err = misc.ddWatchCopy(srcPath, dstPath, None, len(data)) @@ -724,7 +724,7 @@ """ dataLength = len(writeData) - fd, path = tempfile.mkstemp() + fd, path = tempfile.mkstemp(dir='.') f = os.fdopen(fd, "wb") written = 0 @@ -855,7 +855,7 @@ "but to kill myself? That would put a damper on my " "search for answers. Not at all productive.") # (C) Jhonen Vasquez - Johnny the Homicidal Maniac - with temporaryPath(data=writeData) as path: + with temporaryPath(data=writeData, dir='.') as path: # read readData = misc.readfile(path) diff --git a/tests/testrunner.py b/tests/testrunner.py index 985a2b3..ec049a7 100644 --- a/tests/testrunner.py +++ b/tests/testrunner.py @@ -127,8 +127,8 @@ @contextmanager -def temporaryPath(perms=None, data=None): - fd, src = tempfile.mkstemp() +def temporaryPath(perms=None, data=None, dir=None): + fd, src = tempfile.mkstemp(dir=dir) if data is not None: f = os.fdopen(fd, "wb") f.write(data) diff --git a/vdsm/storage/fileUtils.py b/vdsm/storage/fileUtils.py index b4dbd14..227f1c5 100644 --- a/vdsm/storage/fileUtils.py +++ b/vdsm/storage/fileUtils.py @@ -36,7 +36,7 @@ import errno from vdsm import constants -import mount + libc = ctypes.CDLL("libc.so.6", use_errno=True) log = logging.getLogger('Storage.fileUtils') @@ -201,13 +201,6 @@ return open(path, mode) -def pathRequiresFlagForDirectIO(path): - # Memory-only file systems don't support direct IO because direct IO - # means "skip the page cache" and they are 100% page cache. - vfstype = mount.findMountOfPath(path).getRecord().fs_vfstype - return vfstype not in ["tmpfs", "ramfs"] - - class DirectFile(object): def __init__(self, path, mode): if "d" not in mode: @@ -217,10 +210,7 @@ raise ValueError("Invalid mode parameter") self._writable = True - if pathRequiresFlagForDirectIO(path): - flags = os.O_DIRECT - else: - flags = 0 + flags = os.O_DIRECT if "r" in mode: if "+" in mode: diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py index 0ba4f1f..12d4d97 100644 --- a/vdsm/storage/misc.py +++ b/vdsm/storage/misc.py @@ -52,7 +52,6 @@ from vdsm import constants from vdsm import utils import storage_exception as se -import fileUtils import logUtils from caps import isOvirtNode @@ -193,8 +192,7 @@ def _readfile(name, buffersize=None): cmd = [constants.EXT_DD] - if fileUtils.pathRequiresFlagForDirectIO(name): - cmd.append("iflag=%s" % DIRECTFLAG) + cmd.append("iflag=%s" % DIRECTFLAG) cmd.append("if=%s" % name) if buffersize: @@ -265,11 +263,8 @@ while left > 0: (iounit, count, iooffset) = _alignData(left, offset) - cmd = [constants.EXT_DD] - if fileUtils.pathRequiresFlagForDirectIO(name): - cmd.append("iflag=%s" % DIRECTFLAG) - cmd.extend(["skip=%d" % iooffset, "bs=%d" % iounit, "if=%s" % name, - "count=%s" % count]) + cmd = [constants.EXT_DD, "iflag=%s" % DIRECTFLAG, "skip=%d" % iooffset, + "bs=%d" % iounit, "if=%s" % name, "count=%s" % count] (rc, out, err) = execCmd(cmd, raw=True) if rc: @@ -352,8 +347,7 @@ oflag = None conv = "notrunc" if (iounit % 512) == 0: - if fileUtils.pathRequiresFlagForDirectIO(dst): - oflag = DIRECTFLAG + oflag = DIRECTFLAG else: conv += ",%s" % DATASYNCFLAG diff --git a/vdsm/storage/mount.py b/vdsm/storage/mount.py index 90cacc7..1671bf8 100644 --- a/vdsm/storage/mount.py +++ b/vdsm/storage/mount.py @@ -154,30 +154,6 @@ raise OSError(errno.ENOENT, 'Mount target %s not found' % target) -def findMountOfPath(path): - # TBD: Bind mounts, should we ignore them? - # Follow symlinks (if file exists) - path = os.path.realpath(path) - - # Make sure using canonical representation - path = os.path.normpath(path) - - # Find longest match - maxLen = 0 - mountRec = None - for rec in _iterMountRecords(): - mntPath = os.path.normpath(rec.fs_file) - if len(mntPath) > maxLen: - if path.startswith(mntPath): - maxLen = len(mntPath) - mountRec = rec - - if mountRec is None: - raise OSError(errno.ENOENT, os.strerror(errno.ENOENT)) - - return Mount(mountRec.fs_spec, mountRec.fs_file) - - def getMountFromDevice(device): device = normpath(device) for rec in _iterMountRecords(): -- To view, visit http://gerrit.ovirt.org/27441 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7b652b5c78ff8b48a39df2dc57c02496bec666a1 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
