librbd: clean up snapshot handling a bit

* snapid should determine whether our mapped snapshot is gone, not snapname
* snap_set(<nonexistent_snap>) shouldn't reset us to CEPH_NOSNAP
* snapname should be set before using the it in the perfcounter name
* snapname and image name don't need to be passed as arguments since an
  ImageCtx already contains that info
* ictx_check() doesn't need to check for non-existent snaps - only I/Os care,
  so check in check_io() instead

Signed-off-by: Josh Durgin <josh.durgin@dreamhost.com>

1 files changed, 47 insertions, 51 deletions

diff --git a/src/librbd.cc b/src/librbd.cc
index 485f709664f..22789e0dead 100644
--- a/src/librbd.cc
+++ b/src/librbd.cc
@@ -105,6 +105,7 @@ namespace librbd {
     vector<snap_t> snaps;
     std::map<std::string, struct SnapInfo> snaps_by_name;
     uint64_t snapid;
+    bool snap_exists; // false if our snapid was deleted
     std::string name;
     std::string snapname;
     IoCtx data_ctx, md_ctx;
@@ -118,10 +119,11 @@ namespace librbd {
     LibrbdWriteback *writeback_handler;
     ObjectCacher::ObjectSet *object_set;
 
-    ImageCtx(std::string imgname, IoCtx& p)
+    ImageCtx(std::string imgname, const char *snap, IoCtx& p)
       : cct((CephContext*)p.cct()),
 	perfcounter(NULL),
 	snapid(CEPH_NOSNAP),
+	snap_exists(true),
 	name(imgname),
 	needs_refresh(true),
 	refresh_lock("librbd::ImageCtx::refresh_lock"),
@@ -133,7 +135,8 @@ namespace librbd {
       data_ctx.dup(p);
 
       string pname = string("librbd-") + data_ctx.get_pool_name() + string("/") + name;
-      if (snapname.length()) {
+      if (snap) {
+	snapname = snap;
 	pname += "@";
 	pname += snapname;
       }
@@ -206,7 +209,6 @@ namespace librbd {
 	snapid = it->second.id;
 	return 0;
       }
-      snap_unset();
       return -ENOENT;
     }
 
@@ -253,7 +255,8 @@ namespace librbd {
 	return header.image_size;
       } else {
 	map<std::string,SnapInfo>::const_iterator p = snaps_by_name.find(snapname);
-	assert(p != snaps_by_name.end());
+	if (p == snaps_by_name.end())
+	  return 0;
 	return p->second.size;
       }
     }
@@ -467,10 +470,10 @@ namespace librbd {
   int add_snap(ImageCtx *ictx, const char *snap_name);
   int rm_snap(ImageCtx *ictx, const char *snap_name);
   int ictx_check(ImageCtx *ictx);
-  int ictx_refresh(ImageCtx *ictx, const char *snap_name);
+  int ictx_refresh(ImageCtx *ictx);
   int copy(ImageCtx& srci, IoCtx& dest_md_ctx, const char *destname);
 
-  int open_image(IoCtx& io_ctx, ImageCtx *ictx, const char *name, const char *snap_name);
+  int open_image(ImageCtx *ictx);
   void close_image(ImageCtx *ictx);
 
   void trim_image(IoCtx& io_ctx, const rbd_obj_header_ondisk &header, uint64_t newsize,
@@ -1172,36 +1175,22 @@ int ictx_check(ImageCtx *ictx)
   if (needs_refresh) {
     Mutex::Locker l(ictx->lock);
 
-    string snap;
-    if (ictx->snapid != CEPH_NOSNAP)
-      snap = ictx->snapname;
-
-    int r = ictx_refresh(ictx, snap.length() ? snap.c_str() : NULL);
+    int r = ictx_refresh(ictx);
     if (r < 0) {
       lderr(cct) << "Error re-reading rbd header: " << cpp_strerror(-r) << dendl;
       return r;
     }
-
-    // check if the snapshot at which we were reading was removed
-    if (ictx->snapname != snap) {
-      lderr(cct) << "tried to read from a snapshot that no longer exists: " << snap << dendl;
-      return -ENOENT;
-    }
   }
   return 0;
 }
 
-int ictx_refresh(ImageCtx *ictx, const char *snap_name)
+int ictx_refresh(ImageCtx *ictx)
 {
   CephContext *cct = ictx->cct;
   assert(ictx->lock.is_locked());
   bufferlist bl, bl2;
 
-  if (snap_name) {
-    ldout(cct, 20) << "ictx_refresh " << ictx << " snap = " << snap_name << dendl;
-  } else {
-    ldout(cct, 20) << "ictx_refresh " << ictx << " no snap" << dendl;
-  }
+  ldout(cct, 20) << "ictx_refresh " << ictx << dendl;
 
   int r = read_header(ictx->md_ctx, ictx->md_oid(), &(ictx->header), NULL);
   if (r < 0) {
@@ -1213,6 +1202,7 @@ int ictx_refresh(ImageCtx *ictx, const char *snap_name)
     lderr(cct) << "Error listing snapshots: " << cpp_strerror(-r) << dendl;
     return r;
   }
+  r = 0;
 
   std::map<snap_t, std::string> old_snap_ids;
   for (std::map<std::string, struct SnapInfo>::iterator it =
@@ -1252,13 +1242,11 @@ int ictx_refresh(ImageCtx *ictx, const char *snap_name)
     return -EIO;
   }
 
-  if (snap_name) {
-    r = ictx->snap_set(snap_name);
-    if (r < 0) {
-      lderr(cct) << "could not set snap to " << snap_name << ": " << cpp_strerror(-r) << dendl;
-      return r;
-    }
-    ictx->data_ctx.snap_set_read(ictx->snapid);
+  if (ictx->snapid != CEPH_NOSNAP &&
+      ictx->get_snapid(ictx->snapname) == CEPH_NOSNAP) {
+    lderr(cct) << "tried to read from a snapshot that no longer exists: "
+	       << ictx->snapname << dendl;
+    ictx->snap_exists = false;
   }
 
   ictx->data_ctx.selfmanaged_snap_set_write_ctx(ictx->snapc.seq, ictx->snaps);
@@ -1311,6 +1299,12 @@ int snap_rollback(ImageCtx *ictx, const char *snap_name, ProgressContext& prog_c
   if (r < 0)
     return r;
 
+  if (!ictx->snap_exists)
+    return -ENOENT;
+
+  if (ictx->snapid != CEPH_NOSNAP)
+    return -EROFS;
+
   Mutex::Locker l(ictx->lock);
   snap_t snapid = ictx->get_snapid(snap_name);
   if (snapid == CEPH_NOSNAP) {
@@ -1340,8 +1334,7 @@ int snap_rollback(ImageCtx *ictx, const char *snap_name, ProgressContext& prog_c
     return r;
   }
 
-  // refresh without setting the snapid we read from
-  ictx_refresh(ictx, NULL);
+  ictx_refresh(ictx);
   snap_t new_snapid = ictx->get_snapid(snap_name);
   ldout(cct, 20) << "snapid is " << ictx->snapid << " new snapid is " << new_snapid << dendl;
 
@@ -1387,9 +1380,9 @@ int copy(ImageCtx& ictx, IoCtx& dest_md_ctx, const char *destname,
     return r;
   }
 
-  cp.destictx = new librbd::ImageCtx(destname, dest_md_ctx);
+  cp.destictx = new librbd::ImageCtx(destname, NULL, dest_md_ctx);
   cp.src_size = src_size;
-  r = open_image(dest_md_ctx, cp.destictx, destname, NULL);
+  r = open_image(cp.destictx);
   if (r < 0) {
     lderr(cct) << "failed to read newly created header" << dendl;
     return r;
@@ -1408,15 +1401,12 @@ int copy(ImageCtx& ictx, IoCtx& dest_md_ctx, const char *destname,
 
 int snap_set(ImageCtx *ictx, const char *snap_name)
 {
-  ldout(ictx->cct, 20) << "snap_set " << ictx << " snap = " << (snap_name ? snap_name : "NULL") << dendl;
-
-  int r = ictx_check(ictx);
-  if (r < 0)
-    return r;
+  ldout(ictx->cct, 20) << "snap_set " << ictx << " snap = "
+		       << (snap_name ? snap_name : "NULL") << dendl;
 
   Mutex::Locker l(ictx->lock);
   if (snap_name) {
-    r = ictx->snap_set(snap_name);
+    int r = ictx->snap_set(snap_name);
     if (r < 0) {
       return r;
     }
@@ -1424,25 +1414,27 @@ int snap_set(ImageCtx *ictx, const char *snap_name)
     ictx->snap_unset();
   }
 
+  ictx->snap_exists = true;
   ictx->data_ctx.snap_set_read(ictx->snapid);
 
   return 0;
 }
 
-int open_image(IoCtx& io_ctx, ImageCtx *ictx, const char *name, const char *snap_name)
+int open_image(ImageCtx *ictx)
 {
-  CephContext *cct = (CephContext *)io_ctx.cct();
-  string sn = snap_name ? snap_name : "NULL";
-  ldout(cct, 20) << "open_image " << &io_ctx << " ictx =  " << ictx
-		 << " name =  " << name << " snap_name = "
-		 << (snap_name ? snap_name : "NULL") << dendl;
+  ldout(ictx->cct, 20) << "open_image: ictx =  " << ictx
+		       << " name =  '" << ictx->name << "' snap_name = '"
+		       << ictx->snapname << "'" << dendl;
 
   ictx->lock.Lock();
-  int r = ictx_refresh(ictx, snap_name);
+  int r = ictx_refresh(ictx);
   ictx->lock.Unlock();
   if (r < 0)
     return r;
 
+  ictx->snap_set(ictx->snapname);
+  ictx->data_ctx.snap_set_read(ictx->snapid);
+
   WatchCtx *wctx = new WatchCtx(ictx);
   if (!wctx)
     return -ENOMEM;
@@ -1778,8 +1770,12 @@ int check_io(ImageCtx *ictx, uint64_t off, uint64_t len)
 {
   ictx->lock.Lock();
   uint64_t image_size = ictx->get_image_size();
+  bool snap_exists = ictx->snap_exists;
   ictx->lock.Unlock();
 
+  if (!snap_exists)
+    return -ENOENT;
+
   if ((uint64_t)(off + len) > image_size)
     return -EINVAL;
   return 0;
@@ -2060,11 +2056,11 @@ int RBD::open(IoCtx& io_ctx, Image& image, const char *name)
 
 int RBD::open(IoCtx& io_ctx, Image& image, const char *name, const char *snapname)
 {
-  ImageCtx *ictx = new ImageCtx(name, io_ctx);
+  ImageCtx *ictx = new ImageCtx(name, snapname, io_ctx);
   if (!ictx)
     return -ENOMEM;
 
-  int r = librbd::open_image(io_ctx, ictx, name, snapname);
+  int r = librbd::open_image(ictx);
   if (r < 0)
     return r;
 
@@ -2379,10 +2375,10 @@ extern "C" int rbd_open(rados_ioctx_t p, const char *name, rbd_image_t *image, c
 {
   librados::IoCtx io_ctx;
   librados::IoCtx::from_rados_ioctx_t(p, io_ctx);
-  librbd::ImageCtx *ictx = new librbd::ImageCtx(name, io_ctx);
+  librbd::ImageCtx *ictx = new librbd::ImageCtx(name, snap_name, io_ctx);
   if (!ictx)
     return -ENOMEM;
-  int r = librbd::open_image(io_ctx, ictx, name, snap_name);
+  int r = librbd::open_image(ictx);
   *image = (rbd_image_t)ictx;
   return r;
 }