os/bluestore/BlueFS: delay IOContext dtor until after io completes

It's possible for the IO to be in flight when the caller closes the
writer handle (although dangerous of them).  Queue the IOContext for
async cleanup when we sync everything to disk.

Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/os/bluestore/BlueFS.cc b/src/os/bluestore/BlueFS.cc
index 3b4ce730578d45e5d2807556440567c7665f055d..ad2c1dd34aac57db266370ae7bee098a2a4f231f 100644 (file)
--- a/src/os/bluestore/BlueFS.cc
+++ b/src/os/bluestore/BlueFS.cc
@@ -176,7 +176,7 @@ int BlueFS::mkfs(uuid_d osd_uuid)
 
   // clean up
   super = bluefs_super_t();
-  delete log_writer;
+  _close_writer(log_writer);
   log_writer = NULL;
   block_all.clear();
   _stop_alloc();
@@ -251,10 +251,17 @@ int BlueFS::mount()
 void BlueFS::umount()
 {
   dout(1) << __func__ << dendl;
+
   sync_metadata();
 
-  delete log_writer;
+  _close_writer(log_writer);
   log_writer = NULL;
+  // manually clean up it's iocs
+  for (auto p : ioc_reap_queue) {
+    delete p;
+  }
+  ioc_reap_queue.clear();
+
   block_all.clear();
   _stop_alloc();
   file_map.clear();
@@ -791,7 +798,7 @@ void BlueFS::_compact_log()
     assert(r == 0);
   }
 
-  delete log_writer;
+  _close_writer(log_writer);
 
   log_file->fnode.size = bl.length();
   log_writer = new FileWriter(log_file, bdev.size());
@@ -1123,6 +1130,8 @@ void BlueFS::sync_metadata()
   }
   dout(10) << __func__ << dendl;
   utime_t start = ceph_clock_now(NULL);
+  vector<IOContext*> iocv;
+  iocv.swap(ioc_reap_queue);
   for (auto p : alloc) {
     p->commit_start();
   }
@@ -1130,6 +1139,9 @@ void BlueFS::sync_metadata()
   for (auto p : alloc) {
     p->commit_finish();
   }
+  for (auto p : iocv) {
+    delete p;
+  }
   utime_t end = ceph_clock_now(NULL);
   utime_t dur = end - start;
   dout(10) << __func__ << " done in " << dur << dendl;
@@ -1212,6 +1224,20 @@ int BlueFS::open_for_write(
   return 0;
 }
 
+void BlueFS::_close_writer(FileWriter *h)
+{
+  dout(10) << __func__ << " " << h << dendl;
+  for (auto i : h->iocv) {
+    if (i->has_aios()) {
+      ioc_reap_queue.push_back(i);
+    } else {
+      delete i;
+    }
+  }
+  h->iocv.clear();
+  delete h;
+}
+
 int BlueFS::open_for_read(
   const string& dirname,
   const string& filename,

diff --git a/src/os/bluestore/BlueFS.h b/src/os/bluestore/BlueFS.h
index 9d3382f00a0693e45ca72caf6805b672ae2d04b3..85b6638b9a22f9f60173452f7c44449483f42615 100644 (file)
--- a/src/os/bluestore/BlueFS.h
+++ b/src/os/bluestore/BlueFS.h
@@ -82,9 +82,7 @@ public:
     }
     ~FileWriter() {
       file->num_writers.dec();
-      for (auto p : iocv) {
-       delete p;
-      }
+      assert(iocv.empty());  // caller must call BlueFS::close_writer()
     }
 
     void append(const char *buf, size_t len) {
@@ -182,6 +180,8 @@ private:
   vector<interval_set<uint64_t> > block_all;  ///< extents in bdev we own
   vector<Allocator*> alloc;                   ///< allocators for bdevs
 
+  vector<IOContext*> ioc_reap_queue;          ///< iocs from closed writers
+
   void _init_alloc();
   void _stop_alloc();
 
@@ -221,6 +221,8 @@ private:
   int _write_super();
   int _replay(); ///< replay journal
 
+  void _close_writer(FileWriter *h);
+
   // always put the super in the second 4k block.  FIXME should this be
   // block size independent?
   unsigned get_super_offset() {
@@ -260,6 +262,11 @@ public:
     FileReader **h,
     bool random = false);
 
+  void close_writer(FileWriter *h) {
+    Mutex::Locker l(lock);
+    _close_writer(h);
+  }
+
   int rename(const string& old_dir, const string& old_file,
             const string& new_dir, const string& new_file);
 

diff --git a/src/os/bluestore/BlueRocksEnv.cc b/src/os/bluestore/BlueRocksEnv.cc
index a7659dc79f6e3a6032a543a650a3e71e61d7d047..857472fa939180391e8d815144ddfb126d113fbf 100644 (file)
--- a/src/os/bluestore/BlueRocksEnv.cc
+++ b/src/os/bluestore/BlueRocksEnv.cc
@@ -156,7 +156,7 @@ class BlueRocksWritableFile : public rocksdb::WritableFile {
  public:
   BlueRocksWritableFile(BlueFS *fs, BlueFS::FileWriter *h) : fs(fs), h(h) {}
   ~BlueRocksWritableFile() {
-    delete h;
+    fs->close_writer(h);
   }
 
   // Indicates if the class makes use of unbuffered I/O