Remove redundant syscalls in POST path

During process of POST requests which updates object metadata (xattrs),
the following (ordered) sequence of syscalls were being made twice:
open(), fstat(), fgetxattr(), close()

Intuitively, one may assume that a getxattr() and setxattr() is enough
to fulfil the POST request as it is only supposed to update metadata.
But this isn't the case. The above series of syscalls is made first
during disk_file.open(). This will trigger an update of all stale
metadata (outdated size/etag) and the result is retained in a diskfile
class attribute named 'self._metadata'

Instead of using this pre-fetched metadata, the POST path was internally
invoking disk_file.open() again in disk_file.write_metadata(). This is
redundant and serves no purpose. self._metadata was being erased during
the context manager cleanup of disk_file.open()

This change is simple and does the following:
* Don't erase fetched metadata during context manager exit of open()
* Use a different internal variable to detect and raise DiskFileNotOpen
* Re-use self._metadata if available in disk_file.write_metadata()

Here's comparing syscalls made (POST path) with and without this fix:
https://bugzilla.redhat.com/show_bug.cgi?id=1314171#c4

Change-Id: Ib64c103e5904428df20ec6e8f10140f4f68e7f79
Signed-off-by: Prashanth Pai <ppai@redhat.com>
Reviewed-on: http://review.gluster.org/13668
Reviewed-by: Thiago da Silva <thiago@redhat.com>
Tested-by: Thiago da Silva <thiago@redhat.com>

2 files changed, 31 insertions, 5 deletions

diff --git a/gluster/swift/obj/diskfile.py b/gluster/swift/obj/diskfile.py
index b776d0f..1cbb2e7 100644
--- a/gluster/swift/obj/diskfile.py
+++ b/gluster/swift/obj/diskfile.py
@@ -576,6 +576,7 @@ class DiskFile(object):
             self._put_datadir = self._container_path
 
         self._data_file = os.path.join(self._put_datadir, self._obj)
+        self._disk_file_open = False
 
     def open(self):
         """
@@ -641,6 +642,7 @@ class DiskFile(object):
                 # Re-raise the original exception after fd has been closed
                 raise
 
+        self._disk_file_open = True
         return self
 
     def _is_object_expired(self, metadata):
@@ -674,7 +676,7 @@ class DiskFile(object):
             previously invoked the :func:`swift.obj.diskfile.DiskFile.open`
             method.
         """
-        if self._metadata is None:
+        if not self._disk_file_open:
             raise DiskFileNotOpen()
         return self
 
@@ -694,19 +696,23 @@ class DiskFile(object):
             the REST API *before* the object has actually been read. It is the
             responsibility of the implementation to properly handle that.
         """
-        self._metadata = None
+        self._disk_file_open = False
         self._close_fd()
 
     def get_metadata(self):
         """
         Provide the metadata for a previously opened object as a dictionary.
 
+        This is invoked by Swift code in the GET path as follows:
+        with disk_file.open():
+            metadata = disk_file.get_metadata()
+
         :returns: object's metadata dictionary
         :raises DiskFileNotOpen: if the
             :func:`swift.obj.diskfile.DiskFile.open` method was not previously
             invoked
         """
-        if self._metadata is None:
+        if not self._disk_file_open:
             raise DiskFileNotOpen()
         return self._metadata
 
@@ -715,6 +721,9 @@ class DiskFile(object):
         Return the metadata for an object without requiring the caller to open
         the object first.
 
+        This method is invoked by Swift code in POST, PUT, HEAD and DELETE path
+        metadata = disk_file.read_metadata()
+
         :returns: metadata dictionary for an object
         :raises DiskFileError: this implementation will raise the same
                             errors as the `open()` method.
@@ -736,7 +745,7 @@ class DiskFile(object):
                            OS buffer cache
         :returns: a :class:`swift.obj.diskfile.DiskFileReader` object
         """
-        if self._metadata is None:
+        if not self._disk_file_open:
             raise DiskFileNotOpen()
         dr = DiskFileReader(
             self._fd, self._threadpool, self._mgr.disk_chunk_size,
@@ -915,6 +924,8 @@ class DiskFile(object):
         Write a block of metadata to an object without requiring the caller to
         open the object first.
 
+        This method is only called in the POST path.
+
         :param metadata: dictionary of metadata to be associated with the
                          object
         :raises DiskFileError: this implementation will raise the same
@@ -933,7 +944,10 @@ class DiskFile(object):
         metadata. If there are any, it should be appended to the metadata obj
         the user is trying to write.
         """
-        orig_metadata = self.read_metadata()
+        # If metadata has been previously fetched, use that.
+        # Stale metadata (outdated size/etag) would've been updated when
+        # metadata is fetched for the first time.
+        orig_metadata = self._metadata or read_metadata(self._data_file)
 
         sys_keys = [X_CONTENT_TYPE, X_ETAG, 'name', X_CONTENT_LENGTH,
                     X_OBJECT_TYPE, X_TYPE]
diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py
index 6e2d0b1..9ef186b 100644
--- a/test/unit/obj/test_diskfile.py
+++ b/test/unit/obj/test_diskfile.py
@@ -191,6 +191,7 @@ class TestDiskFile(unittest.TestCase):
         gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "z")
         assert gdf._obj == "z"
         assert gdf._fd is None
+        assert gdf._disk_file_open is False
         assert gdf._metadata is None
         assert not gdf._is_dir
         with gdf.open():
@@ -198,6 +199,8 @@ class TestDiskFile(unittest.TestCase):
             assert not gdf._is_dir
             assert gdf._fd is not None
             assert gdf._metadata == exp_md
+            assert gdf._disk_file_open is True
+        assert gdf._disk_file_open is False
         self.assertRaises(DiskFileNotOpen, gdf.get_metadata)
         self.assertRaises(DiskFileNotOpen, gdf.reader)
         self.assertRaises(DiskFileNotOpen, gdf.__enter__)
@@ -233,12 +236,15 @@ class TestDiskFile(unittest.TestCase):
         assert gdf._obj == "z"
         assert gdf._fd is None
         assert gdf._metadata is None
+        assert gdf._disk_file_open is False
         assert not gdf._is_dir
         with gdf.open():
             assert not gdf._is_dir
             assert gdf._data_file == the_file
             assert gdf._fd is not None
             assert gdf._metadata == exp_md, "%r != %r" % (gdf._metadata, exp_md)
+            assert gdf._disk_file_open is True
+        assert gdf._disk_file_open is False
 
     def test_open_invalid_existing_metadata(self):
         the_path = os.path.join(self.td, "vol0", "bar")
@@ -256,9 +262,12 @@ class TestDiskFile(unittest.TestCase):
         assert gdf._obj == "z"
         assert not gdf._is_dir
         assert gdf._fd is None
+        assert gdf._disk_file_open is False
         with gdf.open():
             assert gdf._data_file == the_file
             assert gdf._metadata != inv_md
+            assert gdf._disk_file_open is True
+        assert gdf._disk_file_open is False
 
     def test_open_isdir(self):
         the_path = os.path.join(self.td, "vol0", "bar")
@@ -278,9 +287,12 @@ class TestDiskFile(unittest.TestCase):
         gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "d")
         assert gdf._obj == "d"
         assert gdf._is_dir is False
+        assert gdf._disk_file_open is False
         with gdf.open():
             assert gdf._is_dir
             assert gdf._data_file == the_dir
+            assert gdf._disk_file_open is True
+        assert gdf._disk_file_open is False
 
     def _create_and_get_diskfile(self, dev, par, acc, con, obj, fsize=256):
         # FIXME: assumes account === volume