os/bluestore/bluefs: Added minor improvements

Added comments.
Removed comments.
Fixed lock-tracking suffixes to functions stemming from change
_compact_log_dump_metadata_N -> _compact_log_dump_metadata_NF
Removed extra lock_fnode_print.

Signed-off-by: Adam Kupczyk <akupczyk@redhat.com>
(cherry picked from commit fd9ef8b0c1d4de01be36904a638b86fb8fa58702)

diff --git a/src/os/bluestore/BlueFS.cc b/src/os/bluestore/BlueFS.cc
index 8369f63e472d7233b7f671162c949d3f825f8d25..1b63bc81cc22c68206bd3ff6a7c25c9dd5b52d72 100644 (file)
--- a/src/os/bluestore/BlueFS.cc
+++ b/src/os/bluestore/BlueFS.cc
@@ -842,13 +842,13 @@ void BlueFS::umount(bool avoid_compact)
 
   _close_writer(log.writer);
   log.writer = NULL;
+  log.t.clear();
 
   vselector.reset(nullptr);
   _stop_alloc();
   nodes.file_map.clear();
   nodes.dir_map.clear();
   super = bluefs_super_t();
-  log.t.clear();
   _shutdown_logger();
 }
 
@@ -863,7 +863,7 @@ int BlueFS::prepare_new_device(int id, const bluefs_layout_t& layout)
       new_log_dev_cur = BDEV_NEWDB;
       new_log_dev_next = BDEV_DB;
     }
-    _rewrite_log_and_layout_sync_LN_LD(false,
+    _rewrite_log_and_layout_sync_LNF_LD(false,
       BDEV_NEWDB,
       new_log_dev_cur,
       new_log_dev_next,
@@ -871,7 +871,7 @@ int BlueFS::prepare_new_device(int id, const bluefs_layout_t& layout)
       layout);
     //}
   } else if(id == BDEV_NEWWAL) {
-    _rewrite_log_and_layout_sync_LN_LD(false,
+    _rewrite_log_and_layout_sync_LNF_LD(false,
       BDEV_DB,
       BDEV_NEWWAL,
       BDEV_WAL,
@@ -1713,7 +1713,7 @@ int BlueFS::device_migrate_to_existing(
         new_log_dev_next;
   }
 
-  _rewrite_log_and_layout_sync_LN_LD(
+  _rewrite_log_and_layout_sync_LNF_LD(
     false,
     (flags & REMOVE_DB) ? BDEV_SLOW : BDEV_DB,
     new_log_dev_cur,
@@ -1848,7 +1848,7 @@ int BlueFS::device_migrate_to_new(
         BDEV_DB :
        BDEV_SLOW;
 
-  _rewrite_log_and_layout_sync_LN_LD(
+  _rewrite_log_and_layout_sync_LNF_LD(
     false,
     super_dev,
     new_log_dev_cur,
@@ -1875,6 +1875,14 @@ BlueFS::FileRef BlueFS::_get_file(uint64_t ino)
   }
 }
 
+
+/**
+To modify fnode both FileWriter::lock and File::lock must be obtained.
+The special case is when we modify bluefs log (ino 1) or
+we are compacting log (ino 0).
+
+In any case it is enough to hold File::lock to be sure fnode will not be modified.
+*/
 struct lock_fnode_print {
   BlueFS::FileRef file;
   lock_fnode_print(BlueFS::FileRef file) : file(file) {};
@@ -1891,6 +1899,7 @@ void BlueFS::_drop_link_D(FileRef file)
           << " on " << lock_fnode_print(file) << dendl;
   ceph_assert(file->refs > 0);
   ceph_assert(ceph_mutex_is_locked(log.lock));
+  ceph_assert(ceph_mutex_is_locked(nodes.lock));
 
   --file->refs;
   if (file->refs == 0) {
@@ -2136,7 +2145,7 @@ int64_t BlueFS::_read(
 void BlueFS::invalidate_cache(FileRef f, uint64_t offset, uint64_t length)
 {
   std::lock_guard l(f->lock);
-  dout(10) << __func__ << " file " << lock_fnode_print(f)
+  dout(10) << __func__ << " file " << f->fnode
           << " 0x" << std::hex << offset << "~" << length << std::dec
            << dendl;
   if (offset & ~super.block_mask()) {
@@ -2171,7 +2180,7 @@ void BlueFS::compact_log()
 {
   if (!cct->_conf->bluefs_replay_recovery_disable_compact) {
     if (cct->_conf->bluefs_compact_log_sync) {
-      _compact_log_sync_LN_LD();
+      _compact_log_sync_LNF_LD();
     } else {
       _compact_log_async_LD_NF_D();
     }
@@ -2202,7 +2211,7 @@ bool BlueFS::_should_start_compact_log_L_N()
   return true;
 }
 
-void BlueFS::_compact_log_dump_metadata_N(bluefs_transaction_t *t,
+void BlueFS::_compact_log_dump_metadata_NF(bluefs_transaction_t *t,
                                        int flags)
 {
   std::lock_guard nl(nodes.lock);
@@ -2287,12 +2296,16 @@ void BlueFS::_compact_log_async_dump_metadata_NF(bluefs_transaction_t *t,
   }
 }
 
-void BlueFS::_compact_log_sync_LN_LD()
+void BlueFS::_compact_log_sync_LNF_LD()
 {
   dout(10) << __func__ << dendl;
-  auto prefer_bdev =
-    vselector->select_prefer_bdev(log.writer->file->vselector_hint);
-  _rewrite_log_and_layout_sync_LN_LD(true,
+  uint8_t prefer_bdev;
+  {
+    std::lock_guard ll(log.lock);
+    prefer_bdev =
+      vselector->select_prefer_bdev(log.writer->file->vselector_hint);
+  }
+  _rewrite_log_and_layout_sync_LNF_LD(true,
     BDEV_DB,
     prefer_bdev,
     prefer_bdev,
@@ -2301,7 +2314,7 @@ void BlueFS::_compact_log_sync_LN_LD()
   logger->inc(l_bluefs_log_compactions);
 }
 
-void BlueFS::_rewrite_log_and_layout_sync_LN_LD(bool allocate_with_fallback,
+void BlueFS::_rewrite_log_and_layout_sync_LNF_LD(bool allocate_with_fallback,
                                         int super_dev,
                                         int log_dev,
                                         int log_dev_new,
@@ -2326,7 +2339,7 @@ void BlueFS::_rewrite_log_and_layout_sync_LN_LD(bool allocate_with_fallback,
                       << " flags:" << flags
                       << dendl;
   bluefs_transaction_t t;
-  _compact_log_dump_metadata_N(&t, flags);
+  _compact_log_dump_metadata_NF(&t, flags);
 
   dout(20) << __func__ << " op_jump_seq " << log.seq_live << dendl;
   t.op_jump_seq(log.seq_live);
@@ -2507,9 +2520,6 @@ void BlueFS::_compact_log_async_LD_NF_D() //also locks FW for new_writer
                     &new_log->fnode);
   ceph_assert(r == 0);
 
-  // we might have some more ops in log_t due to _allocate call
-  // t.claim_ops(log_t); no we no longer track allocations in log
-
   bufferlist bl;
   encode(t, bl);
   _pad_bl(bl);
@@ -2624,19 +2634,6 @@ void BlueFS::_pad_bl(bufferlist& bl)
   }
 }
 
-/**
- * Adds file modifications from `dirty.files` to bluefs transactions
- * already stored in `log.t`. Writes them to disk and waits until are stable.
- * Guarantees that file modifications with `want_seq` are already stable on disk.
- * In addition may insert jump forward transaction to log write position `jump_to`.
- *
- * it should lock ability to:
- * 1) add to log.t
- * 2) modify dirty.files
- * 3) add to pending_release
- *
- * pending_release and log.t go with same lock
- */
 
 // Returns log seq that was live before advance.
 uint64_t BlueFS::_log_advance_seq()
@@ -2971,7 +2968,7 @@ int BlueFS::_signal_dirty_to_log_D(FileWriter *h)
   return 0;
 }
 
-void BlueFS::flush_range/*WF*/(FileWriter *h, uint64_t offset, uint64_t length) {
+void BlueFS::flush_range(FileWriter *h, uint64_t offset, uint64_t length) {
   std::unique_lock hl(h->lock);
   _flush_range_F(h, offset, length);
 }
@@ -3174,7 +3171,7 @@ void BlueFS::append_try_flush(FileWriter *h, const char* buf, size_t len)
     }
   }
   if (flushed_sum) {
-    _maybe_compact_log_LN_NF_LD_D();
+    _maybe_compact_log_LNF_NF_LD_D();
   }
 }
 
@@ -3188,7 +3185,7 @@ void BlueFS::flush(FileWriter *h, bool force)
     ceph_assert(r == 0);
   }
   if (r == 0 && flushed) {
-    _maybe_compact_log_LN_NF_LD_D();
+    _maybe_compact_log_LNF_NF_LD_D();
   }
 }
 
@@ -3312,7 +3309,7 @@ int BlueFS::fsync(FileWriter *h)
   if (old_dirty_seq) {
     _flush_and_sync_log_LD(old_dirty_seq);
   }
-  _maybe_compact_log_LN_NF_LD_D();
+  _maybe_compact_log_LNF_NF_LD_D();
   return 0;
 }
 
@@ -3515,16 +3512,16 @@ void BlueFS::sync_metadata(bool avoid_compact)
   }
 
   if (!avoid_compact) {
-    _maybe_compact_log_LN_NF_LD_D();
+    _maybe_compact_log_LNF_NF_LD_D();
   }
 }
 
-void BlueFS::_maybe_compact_log_LN_NF_LD_D()
+void BlueFS::_maybe_compact_log_LNF_NF_LD_D()
 {
   if (!cct->_conf->bluefs_replay_recovery_disable_compact &&
       _should_start_compact_log_L_N()) {
     if (cct->_conf->bluefs_compact_log_sync) {
-      _compact_log_sync_LN_LD();
+      _compact_log_sync_LNF_LD();
     } else {
       _compact_log_async_LD_NF_D();
     }

diff --git a/src/os/bluestore/BlueFS.h b/src/os/bluestore/BlueFS.h
index 54dc62eaf87597f240b14e9ad3964a3760f871bc..2ffb2b8bb780cd9fc10bbc836411bb6d08042b9c 100644 (file)
--- a/src/os/bluestore/BlueFS.h
+++ b/src/os/bluestore/BlueFS.h
@@ -436,15 +436,15 @@ private:
     RENAME_SLOW2DB = 4,
     RENAME_DB2SLOW = 8,
   };
-  void _compact_log_dump_metadata_N(bluefs_transaction_t *t,
+  void _compact_log_dump_metadata_NF(bluefs_transaction_t *t,
                                 int flags);
   void _compact_log_async_dump_metadata_NF(bluefs_transaction_t *t,
                                           uint64_t capture_before_seq);
 
-  void _compact_log_sync_LN_LD();
+  void _compact_log_sync_LNF_LD();
   void _compact_log_async_LD_NF_D();
 
-  void _rewrite_log_and_layout_sync_LN_LD(bool allocate_with_fallback,
+  void _rewrite_log_and_layout_sync_LNF_LD(bool allocate_with_fallback,
                                    int super_dev,
                                    int log_dev,
                                    int new_log_dev,
@@ -575,7 +575,7 @@ public:
   /// sync any uncommitted state to disk
   void sync_metadata(bool avoid_compact);
   /// test and compact log, if necessary
-  void _maybe_compact_log_LN_NF_LD_D();
+  void _maybe_compact_log_LNF_NF_LD_D();
 
   void set_volume_selector(BlueFSVolumeSelector* s) {
     vselector.reset(s);