From 42c790d04b85e2d2665da7c13f800d03b263a22f Mon Sep 17 00:00:00 2001 From: Prashanth Pai Date: Thu, 25 Dec 2014 14:26:28 +0530 Subject: [PATCH] Avoid unnecessary unlink() on every successful PUT If you do a strace on object-server PUT operation, you'd see that there's an unlink() sys call which _always_ fails with ENOENT. mkstemp() creates a temp file which is renamed later to it's final object path in filesystem. Hence, on a successful object PUT, the tempfile would never exist in its original location after rename. Change-Id: I805c7c200107e2d56278f0fb35692a51cb1edc0b Signed-off-by: Prashanth Pai --- swift/obj/diskfile.py | 29 +++++++++++--- test/unit/obj/test_diskfile.py | 69 ++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 6 deletions(-) diff --git a/swift/obj/diskfile.py b/swift/obj/diskfile.py index c2c9e2f94d..28ca8af1e0 100644 --- a/swift/obj/diskfile.py +++ b/swift/obj/diskfile.py @@ -113,7 +113,7 @@ def read_metadata(fd): try: while True: metadata += xattr.getxattr(fd, '%s%s' % (METADATA_KEY, - (key or ''))) + (key or ''))) key += 1 except IOError as e: for err in 'ENOTSUP', 'EOPNOTSUPP': @@ -806,6 +806,11 @@ class DiskFileWriter(object): self._upload_size = 0 self._last_sync = 0 self._extension = '.data' + self._put_succeeded = False + + @property + def put_succeeded(self): + return self._put_succeeded def write(self, chunk): """ @@ -852,6 +857,10 @@ class DiskFileWriter(object): # After the rename completes, this object will be available for other # requests to reference. renamer(self._tmppath, target_path) + # If rename is successful, flag put as succeeded. This is done to avoid + # unnecessary os.unlink() of tempfile later. As renamer() has + # succeeded, the tempfile would no longer exist at its original path. + self._put_succeeded = True try: hash_cleanup_listdir(self._datadir) except OSError: @@ -1595,23 +1604,31 @@ class DiskFile(object): if not exists(self._tmpdir): mkdirs(self._tmpdir) fd, tmppath = mkstemp(dir=self._tmpdir) + dfw = None try: if size is not None and size > 0: try: fallocate(fd, size) except OSError: raise DiskFileNoSpace() - yield DiskFileWriter(self._name, self._datadir, fd, tmppath, + dfw = DiskFileWriter(self._name, self._datadir, fd, tmppath, self._bytes_per_sync, self._threadpool) + yield dfw finally: try: os.close(fd) except OSError: pass - try: - os.unlink(tmppath) - except OSError: - pass + if (dfw is None) or (not dfw.put_succeeded): + # Try removing the temp file only if put did NOT succeed. + # + # dfw.put_succeeded is set to True after renamer() succeeds in + # DiskFileWriter._finalize_put() + try: + os.unlink(tmppath) + except OSError: + self._logger.exception('Error removing tempfile: %s' % + tmppath) def write_metadata(self, metadata): """ diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index bb9e2542bd..424e6449b0 100644 --- a/test/unit/obj/test_diskfile.py +++ b/test/unit/obj/test_diskfile.py @@ -2370,5 +2370,74 @@ class TestDiskFile(unittest.TestCase): else: self.fail('`splice` not called with expected arguments') + def test_create_unlink_cleanup_DiskFileNoSpace(self): + # Test cleanup when DiskFileNoSpace() is raised. + df = self.df_mgr.get_diskfile(self.existing_device, '0', 'abc', '123', + 'xyz') + _m_fallocate = mock.MagicMock(side_effect=OSError(errno.ENOSPC, + os.strerror(errno.ENOSPC))) + _m_unlink = mock.Mock() + with mock.patch("swift.obj.diskfile.fallocate", _m_fallocate): + with mock.patch("os.unlink", _m_unlink): + try: + with df.create(size=100): + pass + except DiskFileNoSpace: + pass + else: + self.fail("Expected exception DiskFileNoSpace") + self.assertTrue(_m_fallocate.called) + self.assertTrue(_m_unlink.called) + self.assert_(len(self.df_mgr.logger.log_dict['exception']) == 0) + + def test_create_unlink_cleanup_renamer_fails(self): + # Test cleanup when renamer fails + _m_renamer = mock.MagicMock(side_effect=OSError(errno.ENOENT, + os.strerror(errno.ENOENT))) + _m_unlink = mock.Mock() + df = self._simple_get_diskfile() + data = '0' * 100 + metadata = { + 'ETag': md5(data).hexdigest(), + 'X-Timestamp': Timestamp(time()).internal, + 'Content-Length': str(100), + } + with mock.patch("swift.obj.diskfile.renamer", _m_renamer): + with mock.patch("os.unlink", _m_unlink): + try: + with df.create(size=100) as writer: + writer.write(data) + writer.put(metadata) + except OSError: + pass + else: + self.fail("Expected OSError exception") + self.assertFalse(writer.put_succeeded) + self.assertTrue(_m_renamer.called) + self.assertTrue(_m_unlink.called) + self.assert_(len(self.df_mgr.logger.log_dict['exception']) == 0) + + def test_create_unlink_cleanup_logging(self): + # Test logging of os.unlink() failures. + df = self.df_mgr.get_diskfile(self.existing_device, '0', 'abc', '123', + 'xyz') + _m_fallocate = mock.MagicMock(side_effect=OSError(errno.ENOSPC, + os.strerror(errno.ENOSPC))) + _m_unlink = mock.MagicMock(side_effect=OSError(errno.ENOENT, + os.strerror(errno.ENOENT))) + with mock.patch("swift.obj.diskfile.fallocate", _m_fallocate): + with mock.patch("os.unlink", _m_unlink): + try: + with df.create(size=100): + pass + except DiskFileNoSpace: + pass + else: + self.fail("Expected exception DiskFileNoSpace") + self.assertTrue(_m_fallocate.called) + self.assertTrue(_m_unlink.called) + self.assert_(self.df_mgr.logger.log_dict['exception'][0][0][0]. + startswith("Error removing tempfile:")) + if __name__ == '__main__': unittest.main()