Change in vdsm[master]: Have direct file not use the O_DIRECT flag on tmpfs and ramf...

Saggi Mizrahi has uploaded a new change for review.

Change subject: Have direct file not use the O_DIRECT flag on tmpfs and ramfs
......................................................................

Have direct file not use the O_DIRECT flag on tmpfs and ramfs

tmpfs and ramfs don't support the O_DIRECT flags. This is intentional as
O_DIRECT tells the kernel to bypass the page-cache and those file
systems live solely on the page cache. Since the effect desired by
direct IO is accomplished on these file systems without the flag there
is no reason for the use to test the FS every time before opening a file
for direct access.

The reason we keep the same class instead of falling back to the regular
file object is so we keep the semantics of the DirectFile() class and
the user doesn't have to care that the underlying FS doesn't really
support direct IO.

Change-Id: I7db4136c1a34d960b17312c2c785fc3234b24b92
Signed-off-by: Saggi Mizrahi 
---
M vdsm/storage/fileUtils.py
M vdsm/storage/mount.py
2 files changed, 51 insertions(+), 16 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/95/9595/1

diff --git a/vdsm/storage/fileUtils.py b/vdsm/storage/fileUtils.py
index 13a8cfa..e00a004 100644
--- a/vdsm/storage/fileUtils.py
+++ b/vdsm/storage/fileUtils.py
@@ -31,11 +31,12 @@
 import ctypes
 from contextlib import contextmanager
 import subprocess
-
 import shutil
-from vdsm import constants
 import logging
 import errno
+
+from vdsm import constants
+import mount
 libc = ctypes.CDLL("libc.so.6", use_errno=True)

log = logging.getLogger('fileUtils')
@@ -184,10 +185,13 @@
             log.warning("Dir %s already exists", dirPath)
             if mode is not None:
                 statinfo = os.stat(dirPath)
-                if statinfo[stat.ST_MODE] != mode:
+                curMode = statinfo[stat.ST_MODE]
+                if curMode != mode:
                     raise OSError(errno.EPERM,
-                        "Existing %s permissions %s are not as requested %s" %
-                        (dirPath, oct(statinfo[stat.ST_MODE]), oct(mode)))
+                                  ("Existing %s permissions %s are not as "
+                                   "requested %s") % (dirPath,
+                                                      oct(curMode),
+                                                      oct(mode)))

def resolveUid(user):
@@ -219,7 +223,7 @@
     currentGid = stat.st_gid

if ((uid == currentUid or user == -1) and
-        (gid == currentGid or group == -1)):
+            (gid == currentGid or group == -1)):
         return True

os.chown(path, uid, gid)
@@ -243,7 +247,14 @@
             raise ValueError("Invalid mode parameter")

self._writable = True
-        flags = os.O_DIRECT
+        # Memory only file systems don't support direct IO because direct IO
+        # means "skip the page cache" and they are 100% page cahce.
+        vfstype = mount.findMountOfPath(path).getRecord().fs_vfstype
+        if vfstype in ["tmpfs", "ramfs"]:
+            flags = 0
+        else:
+            flags = os.O_DIRECT
+
         if "r" in mode:
             if "+" in mode:
                 flags |= os.O_RDWR
diff --git a/vdsm/storage/mount.py b/vdsm/storage/mount.py
index 94cfae0..aaa9eb0 100644
--- a/vdsm/storage/mount.py
+++ b/vdsm/storage/mount.py
@@ -34,14 +34,14 @@
 VFS_EXT3 = "ext3"

MountRecord = namedtuple("MountRecord", "fs_spec fs_file fs_vfstype "
-                          "fs_mntops fs_freq fs_passno")
+                         "fs_mntops fs_freq fs_passno")

_RE_ESCAPE = re.compile(r"\0\d\d")

def _parseFstabLine(line):
     (fs_spec, fs_file, fs_vfstype, fs_mntops,
-            fs_freq, fs_passno) = line.split()[:6]
+     fs_freq, fs_passno) = line.split()[:6]
     fs_mntops = fs_mntops.split(",")
     fs_freq = int(fs_freq)
     fs_passno = int(fs_passno)
@@ -50,7 +50,7 @@
     fs_mntops = [_parseFstabPath(item) for item in fs_mntops]

return MountRecord(fs_spec, fs_file, fs_vfstype, fs_mntops,
-            fs_freq, fs_passno)
+                       fs_freq, fs_passno)

def _iterateMtab():
@@ -118,7 +118,7 @@
             continue

yield MountRecord(realSpec, rec.fs_file, rec.fs_vfstype,
-                rec.fs_mntops, rec.fs_freq, rec.fs_passno)
+                          rec.fs_mntops, rec.fs_freq, rec.fs_passno)

def iterMounts():
@@ -144,6 +144,30 @@
             return Mount(rec.fs_spec, rec.fs_file)

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):
@@ -195,7 +219,7 @@
         if not p.wait(timeout):
             p.kill()
             raise OSError(errno.ETIMEDOUT,
-                    "%s operation timed out" % os.path.basename(cmd[0]))
+                          "%s operation timed out" % os.path.basename(cmd[0]))

out, err = p.communicate()
         rc = p.returncode
@@ -232,9 +256,9 @@
                 return record

raise OSError(errno.ENOENT,
-                "Mount of `%s` at `%s` does not exist" %
-                (self.fs_spec, self.fs_file))
+                      "Mount of `%s` at `%s` does not exist" %
+                      (self.fs_spec, self.fs_file))

def __repr__(self):
-        return "<Mount fs_spec='%s' fs_file='%s'>" % \
-                (self.fs_spec, self.fs_file)
+        return ("<Mount fs_spec='%s' fs_file='%s'>" %
+                (self.fs_spec, self.fs_file))

--
To view, visit http://gerrit.ovirt.org/9595
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7db4136c1a34d960b17312c2c785fc3234b24b92
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Have direct file not use the O_DIRECT flag on tmpfs and ramfs
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

any chance you split this patch according to the substantial/cosmetic line?

....................................................
File vdsm/storage/fileUtils.py
Line 188:                 curMode = statinfo[stat.ST_MODE]
Line 189:                 if curMode != mode:
Line 190:                     raise OSError(errno.EPERM,
Line 191:                                   ("Existing %s permissions %s are not as "
Line 192:                                    "requested %s") % (dirPath,
I hate these pep8 fixes which are unrelated to the patch. They confuse and waste reviewer time, as well as future git-blamers.
Line 193:                                                       oct(curMode),
Line 194:                                                       oct(mode)))
Line 195: 
Line 196:

--
To view, visit http://gerrit.ovirt.org/9595
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7db4136c1a34d960b17312c2c785fc3234b24b92
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Dan Kenigsberg

Shu Ming has posted comments on this change.

Change subject: Have direct file not use the O_DIRECT flag on tmpfs and ramfs
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/9595
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7db4136c1a34d960b17312c2c785fc3234b24b92
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Have direct file not use the O_DIRECT flag on tmpfs and ramfs
......................................................................

Patch Set 2: Verified; Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/9595
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7db4136c1a34d960b17312c2c785fc3234b24b92
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Have direct file not use the O_DIRECT flag on tmpfs and ramfs
......................................................................

Have direct file not use the O_DIRECT flag on tmpfs and ramfs

tmpfs and ramfs don't support the O_DIRECT flags. This is intentional as
O_DIRECT tells the kernel to bypass the page-cache and those file
systems live solely on the page cache. Since the effect desired by
direct IO is accomplished on these file systems without the flag there
is no reason for the use to test the FS every time before opening a file
for direct access.

The reason we keep the same class instead of falling back to the regular
file object is so we keep the semantics of the DirectFile() class and
the user doesn't have to care that the underlying FS doesn't really
support direct IO.

Change-Id: I7db4136c1a34d960b17312c2c785fc3234b24b92
Signed-off-by: Saggi Mizrahi 
---
M vdsm/storage/fileUtils.py
M vdsm/storage/mount.py
2 files changed, 33 insertions(+), 2 deletions(-)

Approvals:
  Shu Ming: Looks good to me, but someone else must approve
  Dan Kenigsberg: Verified; Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/9595
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I7db4136c1a34d960b17312c2c785fc3234b24b92
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming