Some comments written during the open/close fops code review.

git-svn-id: file:///svn/toku/tokudb@44737 c7de825b-a66e-492c-adef-691d508d4ae1

ft/cachetable.c

@@ -755,7 +755,7 @@ toku_cachefile_close(CACHEFILE *cfp, char **error_string, BOOL oplsn_valid, LSN
     // There may be reader, writer, or flusher threads on the kibbutz
     // that need to do work on pairs for this cf. Before we can close
     // the underlying file, we need to wait for them to finish. No new
-    // work shoudl start because clients of the cachetable are not supposed
+    // work should start because clients of the cachetable are not supposed
     // to use a cachefile in parallel with a close, or afterwards.
     wait_on_background_jobs_to_finish(cf);
 
@@ -2802,7 +2802,8 @@ static void cachetable_flush_cachefile(CACHETABLE ct, CACHEFILE cf) {
 
         // first we remove the PAIR from the cachetable's linked lists
         // and hashtable, so we guarantee that no other thread can access
-        // this PAIR if we release the cachetable lock        
+        // this PAIR if we release the cachetable lock (which happens in
+        // cachetable_only_write_locked_data() if the pair is dirty).
         cachetable_remove_pair(ct, p);
         // 
         // #5097 found a bug where another thread had a dirty PAIR pinned

ft/ft-internal.h

@@ -445,13 +445,15 @@ struct ft {
     TXNID txnid_that_created_or_locked_when_empty;
     TXNID txnid_that_suppressed_recovery_logs;
 
-    // protects modifying live_ft_handles, txns, and pinned_by_checkpoint
+    // Logically the reference count is zero if live_ft_handles is empty, txns is empty, and pinned_by_checkpoint is false.
+
+    // ft_ref_lock protects modifying live_ft_handles, txns, and pinned_by_checkpoint.
     toku_mutex_t ft_ref_lock;     
     struct toku_list live_ft_handles;
     // transactions that are using this header.  you should only be able
     // to modify this if you have a valid handle in the list of live brts
     OMT txns;
-    // Keep this header around for checkpoint, like a transaction
+    // A checkpoint is running.  If true, then keep this header around for checkpoint, like a transaction
     bool pinned_by_checkpoint;
 
     // If nonzero there was a write error.  Don't write any more, because it probably only gets worse.  This is the error code.

ft/ft-ops.c

@@ -3140,6 +3140,7 @@ toku_ft_handle_inherit_options(FT_HANDLE t, FT ft) {
 // This is the actual open, used for various purposes, such as normal use, recovery, and redirect.
 // fname_in_env is the iname, relative to the env_dir  (data_dir is already in iname as prefix).
 // The checkpointed version (checkpoint_lsn) of the dictionary must be no later than max_acceptable_lsn .
+// Requires: The multi-operation client lock must be held to prevent a checkpoint from occuring.
 static int
 ft_handle_open(FT_HANDLE ft_h, const char *fname_in_env, int is_create, int only_create, CACHETABLE cachetable, TOKUTXN txn, FILENUM use_filenum, DICTIONARY_ID use_dictionary_id, LSN max_acceptable_lsn) {
     int r;
@@ -3307,6 +3308,7 @@ toku_ft_handle_open_recovery(FT_HANDLE t, const char *fname_in_env, int is_creat
 }
 
 // Open a brt in normal use.  The FILENUM and dict_id are assigned by the ft_handle_open() function.
+// Requires: The multi-operation client lock must be held to prevent a checkpoint from occuring.
 int
 toku_ft_handle_open(FT_HANDLE t, const char *fname_in_env, int is_create, int only_create, CACHETABLE cachetable, TOKUTXN txn) {
     int r;
@@ -3419,6 +3421,7 @@ int
 toku_ft_handle_close (FT_HANDLE brt, bool oplsn_valid, LSN oplsn)
 // Effect: See ft-ops.h for the specification of this function.
 {
+    // There are error paths in the ft_handle_open that end with brt->ft==NULL.
     FT ft = brt->ft;
     if (ft) {
         toku_ft_remove_reference(brt->ft, oplsn_valid, oplsn, ft_remove_handle_ref_callback, brt);
@@ -5445,6 +5448,8 @@ void toku_ft_layer_destroy(void) {
     toku_portability_destroy();
 }
 
+// This lock serializes all opens and closes because the cachetable requires that clients do not try to open or close a cachefile in parallel.  We made
+// it coarser by not allowing any cachefiles to be open or closed in parallel.
 void toku_ft_open_close_lock(void) {
     toku_mutex_lock(&ft_open_close_lock);
 }

ft/ft.c

@@ -339,7 +339,9 @@ ft_note_pin_by_checkpoint (CACHEFILE UU(cachefile), void *header_v)
 }
 
 static void
-unpin_by_checkpoint_callback(FT ft, void *extra) {
+unpin_by_checkpoint_callback(FT ft, void *extra) 
+// Requires: the reflock is held.
+{
     invariant(extra == NULL);
     invariant(ft->pinned_by_checkpoint);
     ft->pinned_by_checkpoint = false; //Unpin
@@ -542,8 +544,10 @@ toku_ft_note_ft_handle_open(FT ft, FT_HANDLE live) {
     toku_ft_release_reflock(ft);
 }
 
-int
-toku_ft_needed_unlocked(FT h) {
+bool
+toku_ft_needed_unlocked(FT h) 
+// Effect: Return true iff the reference count is positive.
+{
     return !toku_list_empty(&h->live_ft_handles) || toku_omt_size(h->txns) != 0 || h->pinned_by_checkpoint;
 }
 
@@ -559,7 +563,9 @@ toku_ft_has_one_reference_unlocked(FT ft) {
 // Close brt.  If opsln_valid, use given oplsn as lsn in brt header instead of logging 
 // the close and using the lsn provided by logging the close.  (Subject to constraint 
 // that if a newer lsn is already in the dictionary, don't overwrite the dictionary.)
-int toku_remove_ft (FT h, char **error_string, BOOL oplsn_valid, LSN oplsn) {
+int toku_remove_ft (FT h, char **error_string, BOOL oplsn_valid, LSN oplsn)
+// Requires: we hold the open_close lock (because we are closing) and the multi_operation lock (to prevent checkpoints).
+{
     int r = 0;
     // Must do this work before closing the cf
     if (h->cf) {

src/ydb_db.c

@@ -114,7 +114,8 @@ create_iname(DB_ENV *env, u_int64_t id, char *hint, char *mark, int n) {
 
 static int toku_db_open(DB * db, DB_TXN * txn, const char *fname, const char *dbname, DBTYPE dbtype, u_int32_t flags, int mode);
 
-//DB->close()
+// Effect: Do the work required of DB->close().
+// requires: the multi_operation client lock is held.
 int 
 toku_db_close(DB * db) {
     int r = 0;
@@ -221,6 +222,7 @@ db_open_subdb(DB * db, DB_TXN * txn, const char *fname, const char *dbname, DBTY
 //  open file (toku_ft_handle_open() will handle logging)
 //  close txn
 //  if created a new iname, take full range lock
+// Requires: no checkpoint may take place during this function, which is enforced by holding the multi_operation_client_lock.
 static int 
 toku_db_open(DB * db, DB_TXN * txn, const char *fname, const char *dbname, DBTYPE dbtype, u_int32_t flags, int mode) {
     HANDLE_PANICKED_DB(db);
@@ -287,7 +289,7 @@ toku_db_open(DB * db, DB_TXN * txn, const char *fname, const char *dbname, DBTYP
         iname = create_iname(db->dbenv, id, hint, NULL, -1);  // allocated memory for iname
         toku_fill_dbt(&iname_dbt, iname, strlen(iname) + 1);
         //
-        // 0 for performance only, avoid unnecessary query
+        // put_flags will be 0 for performance only, avoid unnecessary query
         // if we are creating a hot index, per #3166, we do not want the write lock  in directory grabbed.
         // directory read lock is grabbed in toku_db_get above
         //