Change in vdsm[master]: dd: use iflag=direct only when supported by the os

[Dan Kenigsberg]

Dan Kenigsberg has uploaded a new change for review.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

dd: use iflag=direct only when supported by the os

Much like as vdsm's use of fileUtils.DirectFile that was fixed in
http://gerrit.ovirt.org/9595, we should not pass the "direct" flag to
/bin/dd when the underlying filesystem does not support it.

Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Signed-off-by: Dan Kenigsberg 
---
M vdsm/storage/fileUtils.py
M vdsm/storage/misc.py
2 files changed, 24 insertions(+), 10 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/61/9661/1

diff --git a/vdsm/storage/fileUtils.py b/vdsm/storage/fileUtils.py
index e00a004..2bc2cac 100644
--- a/vdsm/storage/fileUtils.py
+++ b/vdsm/storage/fileUtils.py
@@ -238,6 +238,13 @@
         return open(path, mode)

+def pathSupportsDirectFile(path):
+    # 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
+    return vfstype not in ["tmpfs", "ramfs"]
+
+
 class DirectFile(object):
     def __init__(self, path, mode):
         if not "d" in mode:
@@ -247,13 +254,10 @@
             raise ValueError("Invalid mode parameter")

self._writable = True
-        # 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:
+        if pathSupportsDirectFile(path):
             flags = os.O_DIRECT
+        else:
+            flags = 0

if "r" in mode:
             if "+" in mode:
diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py
index 6360dcf..d85fb66 100644
--- a/vdsm/storage/misc.py
+++ b/vdsm/storage/misc.py
@@ -58,6 +58,7 @@
 from vdsm import constants
 import storage_exception as se
 from vdsm.betterPopen import BetterPopen
+import fileUtils
 import logUtils

@@ -272,7 +273,12 @@
     """
     Read the content of the file using /bin/dd command
     """
-    cmd = [constants.EXT_DD, "iflag=%s" % DIRECTFLAG, "if=%s" % name]
+    cmd = [constants.EXT_DD]
+
+    if fileUtils.pathSupportsDirectFile(name):
+        cmd.append("iflag=%s" % DIRECTFLAG)
+    cmd.append("if=%s" % name)
+
     if buffersize:
         cmd.extend(["bs=%d" % buffersize, "count=1"])
     (rc, out, err) = execCmd(cmd, sudo=False)
@@ -297,8 +303,11 @@
     while left > 0:
         (iounit, count, iooffset) = _alignData(left, offset)

-        cmd = [constants.EXT_DD, "iflag=%s" % DIRECTFLAG, "skip=%d" % iooffset,
-               "bs=%d" % iounit, "if=%s" % name, 'count=%s' % count]
+        cmd = [constants.EXT_DD]
+        if fileUtils.pathSupportsDirectFile(name):
+            cmd.append("iflag=%s" % DIRECTFLAG)
+        cmd.extend(["skip=%d" % iooffset, "bs=%d" % iounit, "if=%s" % name,
+                    "count=%s" % count])

(rc, out, err) = execCmd(cmd, raw=True)
         if rc:
@@ -381,7 +390,8 @@
         oflag = None
         conv = "notrunc"
         if (iounit % 512) == 0:
-            oflag = DIRECTFLAG
+            if fileUtils.pathSupportsDirectFile(dst):
+                oflag = DIRECTFLAG
         else:
             conv += ",%s" % DATASYNCFLAG

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg

Ewoud Kohl van Wijngaarden has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

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

(1 inline comment)

Looks good except a minor typo.

....................................................
File vdsm/storage/fileUtils.py
Line 239: 
Line 240: 
Line 241: def pathSupportsDirectFile(path):
Line 242:     # Memory-only file systems don't support direct IO because direct IO
Line 243:     # means "skip the page cache" and they are 100% page cahce.
I know this is cut+paste, but can you correct cahce to cache while you're moving it around?
Line 244:     vfstype = mount.findMountOfPath(path).getRecord().fs_vfstype
Line 245:     return vfstype not in ["tmpfs", "ramfs"]
Line 246: 
Line 247:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Ewoud Kohl van Wijngaarden has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

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

It's not a question of whether it supports diect file ignore

it's that for tmpfs and ramfs it doesn't matter that they don't support direct file

I'd call the method
directIONotRequired()
Because we should fail on other FSs that don't support direct IO and have a backing store

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Ewoud Kohl van Wijngaarden has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

Patch Set 2:

I'm not sure I agree. I think we have three situations:

* Direct IO is supported
* The flag is not supported but IO is direct (like tmpfs)
* It's not supported at all

Doesn't it depend on the situation what you want to do?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

Patch Set 2:

I'm not sure I understand - but if the problem is only naming, I'd send a quick fix on that.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Adam Litke has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

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

I think the debate comes down to one question:  Is it an error if a caller asks for a file to be opened O_DIRECT but for some reason O_DIRECT is not possible.

In my opinion, O_DIRECT is an advisory/optimization.  In other words, if it is not honored vdsm will still function correctly (albeit maybe with lower performance).  Given this, I would say it is not an error to silently drop the O_DIRECT flag for paths where it is not supported.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Shu Ming has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/misc.py
Line 390:         oflag = None
Line 391:         conv = "notrunc"
Line 392:         if (iounit % 512) == 0:
Line 393:             if fileUtils.pathRequiresFlagForDirectIO(dst):
Line 394:                 oflag = DIRECTFLAG
Nits.   I think DD_DIRECTFLAG may be more meaningful than DIRECTFLAG, because this flag is "dd" command only.  Also it will distinct itself  with os.O_DIRECT in fileUtils.py.
Line 395:         else:
Line 396:             conv += ",%s" % DATASYNCFLAG
Line 397: 
Line 398:         cmd = [constants.EXT_DD, "if=%s" % src, "of=%s" % dst,

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/misc.py
Line 390:         oflag = None
Line 391:         conv = "notrunc"
Line 392:         if (iounit % 512) == 0:
Line 393:             if fileUtils.pathRequiresFlagForDirectIO(dst):
Line 394:                 oflag = DIRECTFLAG
most probably right, but unrelated to this patch.
Line 395:         else:
Line 396:             conv += ",%s" % DATASYNCFLAG
Line 397: 
Line 398:         cmd = [constants.EXT_DD, "if=%s" % src, "of=%s" % dst,

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

Saggi Mizrahi has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

dd: use iflag=direct only when supported by the os

Much like as vdsm's use of fileUtils.DirectFile that was fixed in
http://gerrit.ovirt.org/9595, we should not pass the "direct" flag to
/bin/dd when the underlying filesystem does not support it.

Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Signed-off-by: Dan Kenigsberg 
---
M vdsm/storage/fileUtils.py
M vdsm/storage/misc.py
2 files changed, 24 insertions(+), 10 deletions(-)

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

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: dd: use iflag=direct only when supported by the os
......................................................................

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

thanks, reviewers.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I76f3ac6a9c220e34f78d0d922145bca56ee8e10e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dan Kenigsberg 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shu Ming