From 789f1a150c87f05905b02dfdd8ad77ac6c1ba46f Mon Sep 17 00:00:00 2001 From: Prashanth Pai Date: Thu, 10 Mar 2016 15:30:36 +0530 Subject: 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 Reviewed-on: http://review.gluster.org/13668 Reviewed-by: Thiago da Silva Tested-by: Thiago da Silva --- test/unit/obj/test_diskfile.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'test') 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 -- cgit