[PATCH 0/2] libceph: define ceph_create_snap_context()

[PATCH 0/2] libceph: define ceph_create_snap_context()

[Alex Elder]

While reviewing a recent rbd change Josh suggested that a
function I created for creating a snapshot context really
belonged in libceph.  These patches do that.  The first
defines the function in a new source file, and the second
puts it to work.

					-Alex

[PATCH 1/2] libceph: create source file "net/ceph/snapshot.c"
[PATCH 2/2] ceph: use ceph_create_snap_context()

[PATCH 1/2] libceph: create source file "net/ceph/snapshot.c"

[Alex Elder]

This creates a new source file "net/ceph/snapshot.c" to contain
utility routines related to ceph snapshot contexts.  The main
motivation was to define ceph_create_snap_context() as a common way
to create these structures, but I've moved the definitions of
ceph_get_snap_context() and ceph_put_snap_context() there too.
(The benefit of inlining those is very small, and I'd rather
keep this collection of functions together.)

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/libceph.h |   30 +++-------------
 net/ceph/Makefile            |    2 +-
 net/ceph/snapshot.c          |   78
++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 26 deletions(-)
 create mode 100644 net/ceph/snapshot.c

diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index 5493d7b..2e30248 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -157,31 +157,11 @@ struct ceph_snap_context {
 	u64 snaps[];
 };

-static inline struct ceph_snap_context *
-ceph_get_snap_context(struct ceph_snap_context *sc)
-{
-	/*
-	printk("get_snap_context %p %d -> %d\n", sc, atomic_read(&sc->nref),
-	       atomic_read(&sc->nref)+1);
-	*/
-	if (sc)
-		atomic_inc(&sc->nref);
-	return sc;
-}
-
-static inline void ceph_put_snap_context(struct ceph_snap_context *sc)
-{
-	if (!sc)
-		return;
-	/*
-	printk("put_snap_context %p %d -> %d\n", sc, atomic_read(&sc->nref),
-	       atomic_read(&sc->nref)-1);
-	*/
-	if (atomic_dec_and_test(&sc->nref)) {
-		/*printk(" deleting snap_context %p\n", sc);*/
-		kfree(sc);
-	}
-}
+extern struct ceph_snap_context *ceph_create_snap_context(u32 snap_count,
+					gfp_t gfp_flags);
+extern struct ceph_snap_context *ceph_get_snap_context(
+					struct ceph_snap_context *sc);
+extern void ceph_put_snap_context(struct ceph_snap_context *sc);

 /*
  * calculate the number of pages a given length and offset map onto,
diff --git a/net/ceph/Makefile b/net/ceph/Makefile
index e87ef43..958d9856 100644
--- a/net/ceph/Makefile
+++ b/net/ceph/Makefile
@@ -11,5 +11,5 @@ libceph-y := ceph_common.o messenger.o msgpool.o
buffer.o pagelist.o \
 	crypto.o armor.o \
 	auth_x.o \
 	ceph_fs.o ceph_strings.o ceph_hash.o \
-	pagevec.o
+	pagevec.o snapshot.o

diff --git a/net/ceph/snapshot.c b/net/ceph/snapshot.c
new file mode 100644
index 0000000..154683f
--- /dev/null
+++ b/net/ceph/snapshot.c
@@ -0,0 +1,78 @@
+/*
+ * snapshot.c    Ceph snapshot context utility routines (part of libceph)
+ *
+ * Copyright (C) 2013 Inktank Storage, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ */
+
+#include <stddef.h>
+
+#include <linux/types.h>
+#include <linux/export.h>
+#include <linux/ceph/libceph.h>
+
+/*
+ * Ceph snapshot contexts are reference counted objects, and the
+ * returned structure holds a single reference.  Acquire additional
+ * references with ceph_get_snap_context(), and release them with
+ * ceph_put_snap_context().  When the reference count reaches zero
+ * the entire structure is freed.
+ */
+
+/*
+ * Create a new ceph snapshot context large enough to hold the
+ * indicated number of snapshot ids (which can be 0).  Caller has
+ * to fill in snapc->seq and snapc->snaps[0..snap_count-1].
+ *
+ * Returns a null pointer if an error occurs.
+ */
+struct ceph_snap_context *ceph_create_snap_context(u32 snap_count,
+						gfp_t gfp_flags)
+{
+	struct ceph_snap_context *snapc;
+	size_t size;
+
+	size = sizeof (struct ceph_snap_context);
+	size += snap_count * sizeof (snapc->snaps[0]);
+	snapc = kzalloc(size, gfp_flags);
+	if (!snapc)
+		return NULL;
+
+	atomic_set(&snapc->nref, 1);
+	snapc->num_snaps = snap_count;
+
+	return snapc;
+}
+EXPORT_SYMBOL(ceph_create_snap_context);
+
+struct ceph_snap_context *ceph_get_snap_context(struct
ceph_snap_context *sc)
+{
+	if (sc)
+		atomic_inc(&sc->nref);
+	return sc;
+}
+EXPORT_SYMBOL(ceph_get_snap_context);
+
+void ceph_put_snap_context(struct ceph_snap_context *sc)
+{
+	if (!sc)
+		return;
+	if (atomic_dec_and_test(&sc->nref)) {
+		/*printk(" deleting snap_context %p\n", sc);*/
+		kfree(sc);
+	}
+}
+EXPORT_SYMBOL(ceph_put_snap_context);
-- 
1.7.9.5

[PATCH 2/2] ceph: use ceph_create_snap_context()

[Alex Elder]

Now that we have a library routine to create snap contexts, use it.

This is part of:
    http://tracker.ceph.com/issues/4857

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   41 ++++++-----------------------------------
 fs/ceph/snap.c      |    3 +--
 2 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 8b680ad..8b30d83 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -672,35 +672,6 @@ static void rbd_client_release(struct kref *kref)
 	kfree(rbdc);
 }

-/* Caller has to fill in snapc->seq and snapc->snaps[0..snap_count-1] */
-
-static struct ceph_snap_context *rbd_snap_context_create(u32 snap_count)
-{
-	struct ceph_snap_context *snapc;
-	size_t size;
-
-	size = sizeof (struct ceph_snap_context);
-	size += snap_count * sizeof (snapc->snaps[0]);
-	snapc = kzalloc(size, GFP_KERNEL);
-	if (!snapc)
-		return NULL;
-
-	atomic_set(&snapc->nref, 1);
-	snapc->num_snaps = snap_count;
-
-	return snapc;
-}
-
-static inline void rbd_snap_context_get(struct ceph_snap_context *snapc)
-{
-	(void)ceph_get_snap_context(snapc);
-}
-
-static inline void rbd_snap_context_put(struct ceph_snap_context *snapc)
-{
-	ceph_put_snap_context(snapc);
-}
-
 /*
  * Drop reference to ceph client node. If it's not referenced anymore,
release
  * it.
@@ -820,7 +791,7 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,

 	header->image_size = le64_to_cpu(ondisk->image_size);

-	header->snapc = rbd_snap_context_create(snap_count);
+	header->snapc = ceph_create_snap_context(snap_count, GFP_KERNEL);
 	if (!header->snapc)
 		goto out_err;
 	header->snapc->seq = le64_to_cpu(ondisk->snap_seq);
@@ -1753,7 +1724,7 @@ static struct rbd_img_request *rbd_img_request_create(

 	if (write_request) {
 		down_read(&rbd_dev->header_rwsem);
-		rbd_snap_context_get(rbd_dev->header.snapc);
+		ceph_get_snap_context(rbd_dev->header.snapc);
 		up_read(&rbd_dev->header_rwsem);
 	}

@@ -1805,7 +1776,7 @@ static void rbd_img_request_destroy(struct kref *kref)
 	rbd_assert(img_request->obj_request_count == 0);

 	if (img_request_write_test(img_request))
-		rbd_snap_context_put(img_request->snapc);
+		ceph_put_snap_context(img_request->snapc);

 	if (img_request_child_test(img_request))
 		rbd_obj_request_put(img_request->obj_request);
@@ -3071,7 +3042,7 @@ static int rbd_dev_v1_refresh(struct rbd_device
*rbd_dev, u64 *hver)
 	kfree(rbd_dev->header.snap_sizes);
 	kfree(rbd_dev->header.snap_names);
 	/* osd requests may still refer to snapc */
-	rbd_snap_context_put(rbd_dev->header.snapc);
+	ceph_put_snap_context(rbd_dev->header.snapc);

 	if (hver)
 		*hver = h.obj_version;
@@ -3914,7 +3885,7 @@ static int rbd_dev_v2_snap_context(struct
rbd_device *rbd_dev, u64 *ver)
 		goto out;
 	ret = 0;

-	snapc = rbd_snap_context_create(snap_count);
+	snapc = ceph_create_snap_context(snap_count, GFP_KERNEL);
 	if (!snapc) {
 		ret = -ENOMEM;
 		goto out;
@@ -4590,7 +4561,7 @@ static void rbd_dev_unprobe(struct rbd_device
*rbd_dev)
 	/* Free dynamic fields from the header, then zero it out */

 	header = &rbd_dev->header;
-	rbd_snap_context_put(header->snapc);
+	ceph_put_snap_context(header->snapc);
 	kfree(header->snap_sizes);
 	kfree(header->snap_names);
 	kfree(header->object_prefix);
diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index cbb2f54..f01645a 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -332,10 +332,9 @@ static int build_snap_context(struct
ceph_snap_realm *realm)
 	err = -ENOMEM;
 	if (num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
 		goto fail;
-	snapc = kzalloc(sizeof(*snapc) + num*sizeof(u64), GFP_NOFS);
+	snapc = ceph_create_snap_context(num, GFP_NOFS);
 	if (!snapc)
 		goto fail;
-	atomic_set(&snapc->nref, 1);

 	/* build (reverse sorted) snap vector */
 	num = 0;
-- 
1.7.9.5

Re: [PATCH 0/2] libceph: define ceph_create_snap_context()

[Alex Elder]

On 04/30/2013 07:17 AM, Alex Elder wrote:
> While reviewing a recent rbd change Josh suggested that a
> function I created for creating a snapshot context really
> belonged in libceph.  These patches do that.  The first
> defines the function in a new source file, and the second
> puts it to work.

These patches are available in the "review/wip-rbd-cleanup-6"
branch of the ceph-client git repository, which is based on
branch "review/wip-rbd-cleanup-5".

All of the patches that I just posted are available there as
well:
    [PATCH 0/3] rbd: a few small issues
    [PATCH 0/5] rbd: eliminate object version tracking
    [PATCH 0/4] rbd: get rid of the snapshot list

					-Alex
> 					-Alex
> 
> [PATCH 1/2] libceph: create source file "net/ceph/snapshot.c"
> [PATCH 2/2] ceph: use ceph_create_snap_context()
> 

Re: [PATCH 0/2] libceph: define ceph_create_snap_context()

[Josh Durgin]

On 04/30/2013 05:17 AM, Alex Elder wrote:
> While reviewing a recent rbd change Josh suggested that a
> function I created for creating a snapshot context really
> belonged in libceph.  These patches do that.  The first
> defines the function in a new source file, and the second
> puts it to work.
>
> 					-Alex
>
> [PATCH 1/2] libceph: create source file "net/ceph/snapshot.c"
> [PATCH 2/2] ceph: use ceph_create_snap_context()

These look good.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>