[PATCH 0/7] dm-snapshot prefetch patches

[PATCH 0/7] dm-snapshot prefetch patches

[Mikulas Patocka]

Hi

Here I'm sending the dm-snapshot prefetch patches.

Please find the system where prefetch is relevant (the system where it is 
most annoying to have long snapshot load times) and find the best number 
of read-ahead buffers for that system.

For my system (dm-loop on filesystem on 15k Seagate SCSI disk), it is best 
to prefetch 12 buffers. The patch series improves snapshot load time from 
2.8s to 1.7s.

Mikulas

[PATCH 1/7] dm-bufio: return success or failure on prefetch

[Mikulas Patocka]

This patch makes dm_bufio_prefetch return 1 on success and 0 on failure.
A prefetch failure may happen in case of temporary memory shortage.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-bufio.c |    8 ++++++--
 drivers/md/dm-bufio.h |    4 ++--
 2 files changed, 8 insertions(+), 4 deletions(-)

Index: linux-3.13-rc7/drivers/md/dm-bufio.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-bufio.c	2014-01-09 16:11:31.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-bufio.c	2014-01-09 16:16:43.000000000 +0100
@@ -1068,10 +1068,11 @@ void *dm_bufio_new(struct dm_bufio_clien
 }
 EXPORT_SYMBOL_GPL(dm_bufio_new);
 
-void dm_bufio_prefetch(struct dm_bufio_client *c,
+int dm_bufio_prefetch(struct dm_bufio_client *c,
 		       sector_t block, unsigned n_blocks)
 {
 	struct blk_plug plug;
+	int success = 1;
 
 	LIST_HEAD(write_list);
 
@@ -1104,13 +1105,16 @@ void dm_bufio_prefetch(struct dm_bufio_c
 			if (!n_blocks)
 				goto flush_plug;
 			dm_bufio_lock(c);
-		}
+		} else
+			success = 0;
 	}
 
 	dm_bufio_unlock(c);
 
 flush_plug:
 	blk_finish_plug(&plug);
+
+	return success;
 }
 EXPORT_SYMBOL_GPL(dm_bufio_prefetch);
 
Index: linux-3.13-rc7/drivers/md/dm-bufio.h
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-bufio.h	2014-01-09 16:15:47.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-bufio.h	2014-01-09 16:15:55.000000000 +0100
@@ -67,8 +67,8 @@ void *dm_bufio_new(struct dm_bufio_clien
  * The function starts to read the blocks and returns without waiting for
  * I/O to finish.
  */
-void dm_bufio_prefetch(struct dm_bufio_client *c,
-		       sector_t block, unsigned n_blocks);
+int dm_bufio_prefetch(struct dm_bufio_client *c,
+		      sector_t block, unsigned n_blocks);
 
 /*
  * Release a reference obtained with dm_bufio_{read,get,new}. The data

[PATCH 2/7] dm-bufio: introduce dm_bufio_forget

[Mikulas Patocka]

Introduce a new function dm_bufio_forget. It frees the given buffer.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-bufio.c |   22 ++++++++++++++++++++++
 drivers/md/dm-bufio.h |    7 +++++++
 2 files changed, 29 insertions(+)

Index: linux-3.13-rc7/drivers/md/dm-bufio.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-bufio.c	2014-01-10 00:04:40.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-bufio.c	2014-01-10 00:15:33.000000000 +0100
@@ -1354,6 +1354,28 @@ retry:
 }
 EXPORT_SYMBOL_GPL(dm_bufio_release_move);
 
+/*
+ * Free the given buffer.
+ *
+ * This is just a hint, if the buffer is in use or dirty, this function
+ * does nothing.
+ */
+void dm_bufio_forget(struct dm_bufio_client *c, sector_t block)
+{
+	struct dm_buffer *b;
+
+	dm_bufio_lock(c);
+
+	b = __find(c, block);
+	if (b && likely(!b->hold_count) && likely(!b->state)) {
+		__unlink_buffer(b);
+		__free_buffer_wake(b);
+	}
+
+	dm_bufio_unlock(c);
+}
+EXPORT_SYMBOL(dm_bufio_forget);
+
 unsigned dm_bufio_get_block_size(struct dm_bufio_client *c)
 {
 	return c->block_size;
Index: linux-3.13-rc7/drivers/md/dm-bufio.h
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-bufio.h	2014-01-10 00:11:58.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-bufio.h	2014-01-10 00:13:27.000000000 +0100
@@ -108,6 +108,13 @@ int dm_bufio_issue_flush(struct dm_bufio
  */
 void dm_bufio_release_move(struct dm_buffer *b, sector_t new_block);
 
+/*
+ * Free the given buffer.
+ * This is just a hint, if the buffer is in use or dirty, this function
+ * does nothing.
+ */
+void dm_bufio_forget(struct dm_bufio_client *c, sector_t block);
+
 unsigned dm_bufio_get_block_size(struct dm_bufio_client *c);
 sector_t dm_bufio_get_device_size(struct dm_bufio_client *c);
 sector_t dm_bufio_get_block_number(struct dm_buffer *b);

[PATCH 3/7] dm-bufio: introduce dm_bufio_set_minimum_buffers

[Mikulas Patocka]

The function dm_bufio_set_minimum_buffers sets the number of buffers
before freeing happens. dm-bufio may hold more buffers if enough memory is
available.

There is no guarantee that the specified number of buffers will be
available - if you need a guarantee, use the argument reserved_buffers for
dm_bufio_client_create.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-bufio.c |   14 ++++++++++++--
 drivers/md/dm-bufio.h |    5 +++++
 2 files changed, 17 insertions(+), 2 deletions(-)

Index: linux-3.13-rc7/drivers/md/dm-bufio.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-bufio.c	2014-01-11 18:05:24.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-bufio.c	2014-01-11 18:09:01.000000000 +0100
@@ -104,6 +104,8 @@ struct dm_bufio_client {
 	struct list_head reserved_buffers;
 	unsigned need_reserved_buffers;
 
+	unsigned minimum_buffers;
+
 	struct hlist_head *cache_hash;
 	wait_queue_head_t free_buffer_wait;
 
@@ -861,8 +863,8 @@ static void __get_memory_limit(struct dm
 	buffers = dm_bufio_cache_size_per_client >>
 		  (c->sectors_per_block_bits + SECTOR_SHIFT);
 
-	if (buffers < DM_BUFIO_MIN_BUFFERS)
-		buffers = DM_BUFIO_MIN_BUFFERS;
+	if (buffers < c->minimum_buffers)
+		buffers = c->minimum_buffers;
 
 	*limit_buffers = buffers;
 	*threshold_buffers = buffers * DM_BUFIO_WRITEBACK_PERCENT / 100;
@@ -1376,6 +1378,12 @@ void dm_bufio_forget(struct dm_bufio_cli
 }
 EXPORT_SYMBOL(dm_bufio_forget);
 
+void dm_bufio_set_minimum_buffers(struct dm_bufio_client *c, unsigned n)
+{
+	c->minimum_buffers = n;
+}
+EXPORT_SYMBOL(dm_bufio_set_minimum_buffers);
+
 unsigned dm_bufio_get_block_size(struct dm_bufio_client *c)
 {
 	return c->block_size;
@@ -1572,6 +1580,8 @@ struct dm_bufio_client *dm_bufio_client_
 	INIT_LIST_HEAD(&c->reserved_buffers);
 	c->need_reserved_buffers = reserved_buffers;
 
+	c->minimum_buffers = DM_BUFIO_MIN_BUFFERS;
+
 	init_waitqueue_head(&c->free_buffer_wait);
 	c->async_write_error = 0;
 
Index: linux-3.13-rc7/drivers/md/dm-bufio.h
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-bufio.h	2014-01-11 18:06:53.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-bufio.h	2014-01-11 18:09:13.000000000 +0100
@@ -115,6 +115,11 @@ void dm_bufio_release_move(struct dm_buf
  */
 void dm_bufio_forget(struct dm_bufio_client *c, sector_t block);
 
+/*
+ * Set the minimum number of buffers before cleanup happens.
+ */
+void dm_bufio_set_minimum_buffers(struct dm_bufio_client *c, unsigned n);
+
 unsigned dm_bufio_get_block_size(struct dm_bufio_client *c);
 sector_t dm_bufio_get_device_size(struct dm_bufio_client *c);
 sector_t dm_bufio_get_block_number(struct dm_buffer *b);

[PATCH 4/7] dm-snapshot: use GFP_KERNEL when initializing the snapshot

[Mikulas Patocka]

The list of initial exceptions is loaded in the target constructor. We are
allowed to allocate memory with GFP_KERNEL at this point. So, change
alloc_completed_exception to use GFP_KERNEL when being called from the
constructor.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: linux-3.13-rc7/drivers/md/dm-snap.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-snap.c	2014-01-09 20:42:51.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-snap.c	2014-01-09 20:43:41.000000000 +0100
@@ -610,12 +610,12 @@ static struct dm_exception *dm_lookup_ex
 	return NULL;
 }
 
-static struct dm_exception *alloc_completed_exception(void)
+static struct dm_exception *alloc_completed_exception(gfp_t gfp)
 {
 	struct dm_exception *e;
 
-	e = kmem_cache_alloc(exception_cache, GFP_NOIO);
-	if (!e)
+	e = kmem_cache_alloc(exception_cache, gfp);
+	if (!e && gfp == GFP_NOIO)
 		e = kmem_cache_alloc(exception_cache, GFP_ATOMIC);
 
 	return e;
@@ -697,7 +697,7 @@ static int dm_add_exception(void *contex
 	struct dm_snapshot *s = context;
 	struct dm_exception *e;
 
-	e = alloc_completed_exception();
+	e = alloc_completed_exception(GFP_KERNEL);
 	if (!e)
 		return -ENOMEM;
 
@@ -1405,7 +1405,7 @@ static void pending_complete(struct dm_s
 		goto out;
 	}
 
-	e = alloc_completed_exception();
+	e = alloc_completed_exception(GFP_NOIO);
 	if (!e) {
 		down_write(&s->lock);
 		__invalidate_snapshot(s, -ENOMEM);

[PATCH 5/7] dm-snapshot: make ps->area explicit

[Mikulas Patocka]

This patch changes the functions get_exception, read_exception and
insert_exceptions so that the pointer ps->area is given as an argument.

This patch doesn't change any functionality, but it refactors the code for
the next patch.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap-persistent.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Index: linux-3.13-rc7/drivers/md/dm-snap-persistent.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-snap-persistent.c	2014-01-09 23:37:59.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-snap-persistent.c	2014-01-09 23:40:22.000000000 +0100
@@ -401,17 +401,18 @@ static int write_header(struct pstore *p
 /*
  * Access functions for the disk exceptions, these do the endian conversions.
  */
-static struct disk_exception *get_exception(struct pstore *ps, uint32_t index)
+static struct disk_exception *get_exception(struct pstore *ps, void *ps_area,
+					    uint32_t index)
 {
 	BUG_ON(index >= ps->exceptions_per_area);
 
-	return ((struct disk_exception *) ps->area) + index;
+	return ((struct disk_exception *) ps_area) + index;
 }
 
-static void read_exception(struct pstore *ps,
+static void read_exception(struct pstore *ps, void *ps_area,
 			   uint32_t index, struct core_exception *result)
 {
-	struct disk_exception *de = get_exception(ps, index);
+	struct disk_exception *de = get_exception(ps, ps_area, index);
 
 	/* copy it */
 	result->old_chunk = le64_to_cpu(de->old_chunk);
@@ -421,7 +422,7 @@ static void read_exception(struct pstore
 static void write_exception(struct pstore *ps,
 			    uint32_t index, struct core_exception *e)
 {
-	struct disk_exception *de = get_exception(ps, index);
+	struct disk_exception *de = get_exception(ps, ps->area, index);
 
 	/* copy it */
 	de->old_chunk = cpu_to_le64(e->old_chunk);
@@ -430,7 +431,7 @@ static void write_exception(struct pstor
 
 static void clear_exception(struct pstore *ps, uint32_t index)
 {
-	struct disk_exception *de = get_exception(ps, index);
+	struct disk_exception *de = get_exception(ps, ps->area, index);
 
 	/* clear it */
 	de->old_chunk = 0;
@@ -442,7 +443,7 @@ static void clear_exception(struct pstor
  * 'full' is filled in to indicate if the area has been
  * filled.
  */
-static int insert_exceptions(struct pstore *ps,
+static int insert_exceptions(struct pstore *ps, void *ps_area,
 			     int (*callback)(void *callback_context,
 					     chunk_t old, chunk_t new),
 			     void *callback_context,
@@ -456,7 +457,7 @@ static int insert_exceptions(struct psto
 	*full = 1;
 
 	for (i = 0; i < ps->exceptions_per_area; i++) {
-		read_exception(ps, i, &e);
+		read_exception(ps, ps_area, i, &e);
 
 		/*
 		 * If the new_chunk is pointing at the start of
@@ -503,7 +504,8 @@ static int read_exceptions(struct pstore
 		if (r)
 			return r;
 
-		r = insert_exceptions(ps, callback, callback_context, &full);
+		r = insert_exceptions(ps, ps->area, callback, callback_context,
+				      &full);
 		if (r)
 			return r;
 	}
@@ -733,7 +735,7 @@ static int persistent_prepare_merge(stru
 		ps->current_committed = ps->exceptions_per_area;
 	}
 
-	read_exception(ps, ps->current_committed - 1, &ce);
+	read_exception(ps, ps->area, ps->current_committed - 1, &ce);
 	*last_old_chunk = ce.old_chunk;
 	*last_new_chunk = ce.new_chunk;
 
@@ -743,8 +745,8 @@ static int persistent_prepare_merge(stru
 	 */
 	for (nr_consecutive = 1; nr_consecutive < ps->current_committed;
 	     nr_consecutive++) {
-		read_exception(ps, ps->current_committed - 1 - nr_consecutive,
-			       &ce);
+		read_exception(ps, ps->area,
+			       ps->current_committed - 1 - nr_consecutive, &ce);
 		if (ce.old_chunk != *last_old_chunk - nr_consecutive ||
 		    ce.new_chunk != *last_new_chunk - nr_consecutive)
 			break;

[PATCH 6/7] dm-snapshot: use dm-bufio

[Mikulas Patocka]

Use dm-bufio for initial loading of the exceptions.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap-persistent.c |   39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

Index: linux-3.13-rc7/drivers/md/dm-snap-persistent.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-snap-persistent.c	2014-01-10 03:08:36.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-snap-persistent.c	2014-01-10 03:40:56.000000000 +0100
@@ -13,6 +13,7 @@
 #include <linux/export.h>
 #include <linux/slab.h>
 #include <linux/dm-io.h>
+#include "dm-bufio.h"
 
 #define DM_MSG_PREFIX "persistent snapshot"
 #define DM_CHUNK_SIZE_DEFAULT_SECTORS 32	/* 16KB */
@@ -494,27 +495,51 @@ static int read_exceptions(struct pstore
 			   void *callback_context)
 {
 	int r, full = 1;
+	struct dm_bufio_client *client;
+
+	client = dm_bufio_client_create(dm_snap_cow(ps->store->snap)->bdev,
+					ps->store->chunk_size << SECTOR_SHIFT,
+					1, 0, NULL, NULL);
+
+	if (IS_ERR(client))
+		return PTR_ERR(client);
 
 	/*
 	 * Keeping reading chunks and inserting exceptions until
 	 * we find a partially full area.
 	 */
 	for (ps->current_area = 0; full; ps->current_area++) {
-		r = area_io(ps, READ);
-		if (r)
-			return r;
+		struct dm_buffer *bp;
+		void *area;
+		chunk_t chunk = area_location(ps, ps->current_area);
+
+		area = dm_bufio_read(client, chunk, &bp);
+		if (unlikely(IS_ERR(area))) {
+			r = PTR_ERR(area);
+			goto ret_destroy_bufio;
+		}
 
-		r = insert_exceptions(ps, ps->area, callback, callback_context,
+		r = insert_exceptions(ps, area, callback, callback_context,
 				      &full);
-		if (r)
-			return r;
+
+		dm_bufio_release(bp);
+
+		dm_bufio_forget(client, chunk);
+
+		if (unlikely(r))
+			goto ret_destroy_bufio;
 	}
 
 	ps->current_area--;
 
 	skip_metadata(ps);
 
-	return 0;
+	r = 0;
+
+ret_destroy_bufio:
+	dm_bufio_client_destroy(client);
+
+	return r;
 }
 
 static struct pstore *get_info(struct dm_exception_store *store)

[PATCH 7/7] dm-snapshot: use bufio prefetch

[Mikulas Patocka]

This patch modifies dm-snapshot so that it prefetches the buffers when
loading the exceptions.

The number of buffers read ahead is specified in the DM_PREFETCH_CHUNKS
macro.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-snap-persistent.c |   23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Index: linux-3.13-rc7/drivers/md/dm-snap-persistent.c
===================================================================
--- linux-3.13-rc7.orig/drivers/md/dm-snap-persistent.c	2014-01-11 18:10:34.000000000 +0100
+++ linux-3.13-rc7/drivers/md/dm-snap-persistent.c	2014-01-11 18:12:53.000000000 +0100
@@ -18,6 +18,8 @@
 #define DM_MSG_PREFIX "persistent snapshot"
 #define DM_CHUNK_SIZE_DEFAULT_SECTORS 32	/* 16KB */
 
+#define DM_PREFETCH_CHUNKS		12
+
 /*-----------------------------------------------------------------
  * Persistent snapshots, by persistent we mean that the snapshot
  * will survive a reboot.
@@ -496,6 +498,7 @@ static int read_exceptions(struct pstore
 {
 	int r, full = 1;
 	struct dm_bufio_client *client;
+	chunk_t prefetch_area = 0;
 
 	client = dm_bufio_client_create(dm_snap_cow(ps->store->snap)->bdev,
 					ps->store->chunk_size << SECTOR_SHIFT,
@@ -504,6 +507,8 @@ static int read_exceptions(struct pstore
 	if (IS_ERR(client))
 		return PTR_ERR(client);
 
+	dm_bufio_set_minimum_buffers(client, DM_PREFETCH_CHUNKS + 1);
+
 	/*
 	 * Keeping reading chunks and inserting exceptions until
 	 * we find a partially full area.
@@ -511,7 +516,23 @@ static int read_exceptions(struct pstore
 	for (ps->current_area = 0; full; ps->current_area++) {
 		struct dm_buffer *bp;
 		void *area;
-		chunk_t chunk = area_location(ps, ps->current_area);
+		chunk_t chunk;
+
+		if (unlikely(prefetch_area < ps->current_area))
+			prefetch_area = ps->current_area;
+
+		if (DM_PREFETCH_CHUNKS) do {
+			chunk_t pf_chunk = area_location(ps, prefetch_area);
+			if (unlikely(pf_chunk >= dm_bufio_get_device_size(client)))
+				break;
+			if (unlikely(!dm_bufio_prefetch(client, pf_chunk, 1)))
+				break;
+			prefetch_area++;
+			if (unlikely(!prefetch_area))
+				break;
+		} while (prefetch_area <= ps->current_area + DM_PREFETCH_CHUNKS);
+
+		chunk = area_location(ps, ps->current_area);
 
 		area = dm_bufio_read(client, chunk, &bp);
 		if (unlikely(IS_ERR(area))) {

Re: [PATCH 7/7] dm-snapshot: use bufio prefetch

[Mike Snitzer]

On Sat, Jan 11 2014 at 12:32pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> This patch modifies dm-snapshot so that it prefetches the buffers when
> loading the exceptions.
> 
> The number of buffers read ahead is specified in the DM_PREFETCH_CHUNKS
> macro.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-snap-persistent.c |   23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> Index: linux-3.13-rc7/drivers/md/dm-snap-persistent.c
> ===================================================================
> --- linux-3.13-rc7.orig/drivers/md/dm-snap-persistent.c	2014-01-11 18:10:34.000000000 +0100
> +++ linux-3.13-rc7/drivers/md/dm-snap-persistent.c	2014-01-11 18:12:53.000000000 +0100
> @@ -18,6 +18,8 @@
>  #define DM_MSG_PREFIX "persistent snapshot"
>  #define DM_CHUNK_SIZE_DEFAULT_SECTORS 32	/* 16KB */
>  
> +#define DM_PREFETCH_CHUNKS		12
> +

The patch header should speak to why 12 is adequate.  Your 0th patch
header said we should do further testing to arrive at a good value.  I
think a follow-on patch should make the 'prefetch_chunks' configurable.

The dm_exception_store structure has a 4 byte hole before the context
member.  Making prefetch_chunks configurable (either from table load or
dm-snapshot module param) would help future-proof this code.

>  /*-----------------------------------------------------------------
>   * Persistent snapshots, by persistent we mean that the snapshot
>   * will survive a reboot.
> @@ -496,6 +498,7 @@ static int read_exceptions(struct pstore
>  {
>  	int r, full = 1;
>  	struct dm_bufio_client *client;
> +	chunk_t prefetch_area = 0;
>  
>  	client = dm_bufio_client_create(dm_snap_cow(ps->store->snap)->bdev,
>  					ps->store->chunk_size << SECTOR_SHIFT,
> @@ -504,6 +507,8 @@ static int read_exceptions(struct pstore
>  	if (IS_ERR(client))
>  		return PTR_ERR(client);
>  
> +	dm_bufio_set_minimum_buffers(client, DM_PREFETCH_CHUNKS + 1);
> +
>  	/*
>  	 * Keeping reading chunks and inserting exceptions until
>  	 * we find a partially full area.
> @@ -511,7 +516,23 @@ static int read_exceptions(struct pstore
>  	for (ps->current_area = 0; full; ps->current_area++) {
>  		struct dm_buffer *bp;
>  		void *area;
> -		chunk_t chunk = area_location(ps, ps->current_area);
> +		chunk_t chunk;
> +
> +		if (unlikely(prefetch_area < ps->current_area))
> +			prefetch_area = ps->current_area;
> +
> +		if (DM_PREFETCH_CHUNKS) do {
> +			chunk_t pf_chunk = area_location(ps, prefetch_area);
> +			if (unlikely(pf_chunk >= dm_bufio_get_device_size(client)))
> +				break;
> +			if (unlikely(!dm_bufio_prefetch(client, pf_chunk, 1)))
> +				break;
> +			prefetch_area++;
> +			if (unlikely(!prefetch_area))
> +				break;
> +		} while (prefetch_area <= ps->current_area + DM_PREFETCH_CHUNKS);
> +
> +		chunk = area_location(ps, ps->current_area);
>  
>  		area = dm_bufio_read(client, chunk, &bp);
>  		if (unlikely(IS_ERR(area))) {
> 

I factored this code out to a bufio_prefetch_chunks() method, I'll share
it as v2 in reply to your original patch.

Re: [PATCH 7/7] dm-snapshot: use bufio prefetch

[Mikulas Patocka]

On Mon, 13 Jan 2014, Mike Snitzer wrote:

> The patch header should speak to why 12 is adequate.  Your 0th patch
> header said we should do further testing to arrive at a good value.

12 is optimal on my system (dm-loop on filesystem on 15k SCSI disk).

> I think a follow-on patch should make the 'prefetch_chunks' configurable.

I think the user wouldn't configure it anyway.

Find a configuration scenario where this request for prefetch came from 
and find the optimal value for that scenario.

> The dm_exception_store structure has a 4 byte hole before the context
> member.  Making prefetch_chunks configurable (either from table load or
> dm-snapshot module param) would help future-proof this code.

Mikulas

[PATCH v2 7/7] dm snapshot: use bufio prefetch

[Mike Snitzer]

From: Mikulas Patocka <mpatocka@redhat.com>

This patch modifies dm-snapshot so that it prefetches the buffers when
loading the exceptions.

The number of chunk-sized buffers read ahead is specified in the
DM_PREFETCH_CHUNKS macro.  The current default for DM_PREFETCH_CHUNKS
(12) may need to be adjusted to improve performance on different types
of storage -- a future patch should make this configurable.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-snap-persistent.c |   34 +++++++++++++++++++++++++++++++++-
 1 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
index 1692750..ea45b3b 100644
--- a/drivers/md/dm-snap-persistent.c
+++ b/drivers/md/dm-snap-persistent.c
@@ -18,6 +18,8 @@
 #define DM_MSG_PREFIX "persistent snapshot"
 #define DM_CHUNK_SIZE_DEFAULT_SECTORS 32	/* 16KB */
 
+#define DM_PREFETCH_CHUNKS		12
+
 /*-----------------------------------------------------------------
  * Persistent snapshots, by persistent we mean that the snapshot
  * will survive a reboot.
@@ -490,6 +492,30 @@ static int insert_exceptions(struct pstore *ps, void *ps_area,
 	return 0;
 }
 
+static void bufio_prefetch_chunks(struct dm_bufio_client *client,
+				  struct pstore *ps)
+{
+	chunk_t prefetch_area = 0;
+	chunk_t pf_chunk;
+
+	if (!DM_PREFETCH_CHUNKS)
+		return;
+
+	if (prefetch_area < ps->current_area)
+		prefetch_area = ps->current_area;
+
+	do {
+		pf_chunk = area_location(ps, prefetch_area);
+		if (unlikely(pf_chunk >= dm_bufio_get_device_size(client)))
+			break;
+		if (unlikely(!dm_bufio_prefetch(client, pf_chunk, 1)))
+			break;
+		prefetch_area++;
+		if (unlikely(!prefetch_area))
+			break;
+	} while (prefetch_area <= ps->current_area + DM_PREFETCH_CHUNKS);
+}
+
 static int read_exceptions(struct pstore *ps,
 			   int (*callback)(void *callback_context, chunk_t old,
 					   chunk_t new),
@@ -505,6 +531,8 @@ static int read_exceptions(struct pstore *ps,
 	if (IS_ERR(client))
 		return PTR_ERR(client);
 
+	dm_bufio_set_minimum_buffers(client, DM_PREFETCH_CHUNKS + 1);
+
 	/*
 	 * Keeping reading chunks and inserting exceptions until
 	 * we find a partially full area.
@@ -512,7 +540,11 @@ static int read_exceptions(struct pstore *ps,
 	for (ps->current_area = 0; full; ps->current_area++) {
 		struct dm_buffer *bp;
 		void *area;
-		chunk_t chunk = area_location(ps, ps->current_area);
+		chunk_t chunk;
+
+		bufio_prefetch_chunks(client, ps);
+
+		chunk = area_location(ps, ps->current_area);
 
 		area = dm_bufio_read(client, chunk, &bp);
 		if (unlikely(IS_ERR(area))) {
-- 
1.7.4.4

Re: [PATCH v2 7/7] dm snapshot: use bufio prefetch

[Mikulas Patocka]

No.

This changed patch inefficiently loops in bufio_prefetch_chunks for each 
buffer that is read.

Mikulas


On Mon, 13 Jan 2014, Mike Snitzer wrote:

> From: Mikulas Patocka <mpatocka@redhat.com>
> 
> This patch modifies dm-snapshot so that it prefetches the buffers when
> loading the exceptions.
> 
> The number of chunk-sized buffers read ahead is specified in the
> DM_PREFETCH_CHUNKS macro.  The current default for DM_PREFETCH_CHUNKS
> (12) may need to be adjusted to improve performance on different types
> of storage -- a future patch should make this configurable.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> ---
>  drivers/md/dm-snap-persistent.c |   34 +++++++++++++++++++++++++++++++++-
>  1 files changed, 33 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
> index 1692750..ea45b3b 100644
> --- a/drivers/md/dm-snap-persistent.c
> +++ b/drivers/md/dm-snap-persistent.c
> @@ -18,6 +18,8 @@
>  #define DM_MSG_PREFIX "persistent snapshot"
>  #define DM_CHUNK_SIZE_DEFAULT_SECTORS 32	/* 16KB */
>  
> +#define DM_PREFETCH_CHUNKS		12
> +
>  /*-----------------------------------------------------------------
>   * Persistent snapshots, by persistent we mean that the snapshot
>   * will survive a reboot.
> @@ -490,6 +492,30 @@ static int insert_exceptions(struct pstore *ps, void *ps_area,
>  	return 0;
>  }
>  
> +static void bufio_prefetch_chunks(struct dm_bufio_client *client,
> +				  struct pstore *ps)
> +{
> +	chunk_t prefetch_area = 0;
> +	chunk_t pf_chunk;
> +
> +	if (!DM_PREFETCH_CHUNKS)
> +		return;
> +
> +	if (prefetch_area < ps->current_area)
> +		prefetch_area = ps->current_area;
> +
> +	do {
> +		pf_chunk = area_location(ps, prefetch_area);
> +		if (unlikely(pf_chunk >= dm_bufio_get_device_size(client)))
> +			break;
> +		if (unlikely(!dm_bufio_prefetch(client, pf_chunk, 1)))
> +			break;
> +		prefetch_area++;
> +		if (unlikely(!prefetch_area))
> +			break;
> +	} while (prefetch_area <= ps->current_area + DM_PREFETCH_CHUNKS);
> +}
> +
>  static int read_exceptions(struct pstore *ps,
>  			   int (*callback)(void *callback_context, chunk_t old,
>  					   chunk_t new),
> @@ -505,6 +531,8 @@ static int read_exceptions(struct pstore *ps,
>  	if (IS_ERR(client))
>  		return PTR_ERR(client);
>  
> +	dm_bufio_set_minimum_buffers(client, DM_PREFETCH_CHUNKS + 1);
> +
>  	/*
>  	 * Keeping reading chunks and inserting exceptions until
>  	 * we find a partially full area.
> @@ -512,7 +540,11 @@ static int read_exceptions(struct pstore *ps,
>  	for (ps->current_area = 0; full; ps->current_area++) {
>  		struct dm_buffer *bp;
>  		void *area;
> -		chunk_t chunk = area_location(ps, ps->current_area);
> +		chunk_t chunk;
> +
> +		bufio_prefetch_chunks(client, ps);
> +
> +		chunk = area_location(ps, ps->current_area);
>  
>  		area = dm_bufio_read(client, chunk, &bp);
>  		if (unlikely(IS_ERR(area))) {
> -- 
> 1.7.4.4
> 

Re: [PATCH v2 7/7] dm snapshot: use bufio prefetch

[Mike Snitzer]

On Mon, Jan 13 2014 at  5:00pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> No.
> 
> This changed patch inefficiently loops in bufio_prefetch_chunks for each 
> buffer that is read.

Yeah, I overlooked the importance of preserving prefetch_area (which
explains the unlikely you had).  But my intent was to make your code
less "special"; your if (DM_PREFETCH_CHUNKS) do { } while() code to
avoid the extra indentation is pretty ugly.

Given your current code, DM_PREFETCH_CHUNKS is always 12 so why not just
remove the check?

Re: [PATCH v2 7/7] dm snapshot: use bufio prefetch

[Mikulas Patocka]

On Mon, 13 Jan 2014, Mike Snitzer wrote:

> On Mon, Jan 13 2014 at  5:00pm -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > No.
> > 
> > This changed patch inefficiently loops in bufio_prefetch_chunks for each 
> > buffer that is read.
> 
> Yeah, I overlooked the importance of preserving prefetch_area (which
> explains the unlikely you had).  But my intent was to make your code
> less "special"; your if (DM_PREFETCH_CHUNKS) do { } while() code to
> avoid the extra indentation is pretty ugly.
> 
> Given your current code, DM_PREFETCH_CHUNKS is always 12 so why not just
> remove the check?

If someone ever changes it to a variable, the condition is there avoid 
unneeded call to dm_bufio_prefetch. The compiler optimizes out the 
constant expression, so it doesn't matter that it's there.

Mikulas

Re: [PATCH v2 7/7] dm snapshot: use bufio prefetch

[Mike Snitzer]

On Mon, Jan 13 2014 at  6:45pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Mon, 13 Jan 2014, Mike Snitzer wrote:
> 
> > On Mon, Jan 13 2014 at  5:00pm -0500,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > No.
> > > 
> > > This changed patch inefficiently loops in bufio_prefetch_chunks for each 
> > > buffer that is read.
> > 
> > Yeah, I overlooked the importance of preserving prefetch_area (which
> > explains the unlikely you had).  But my intent was to make your code
> > less "special"; your if (DM_PREFETCH_CHUNKS) do { } while() code to
> > avoid the extra indentation is pretty ugly.
> > 
> > Given your current code, DM_PREFETCH_CHUNKS is always 12 so why not just
> > remove the check?
> 
> If someone ever changes it to a variable, the condition is there avoid 
> unneeded call to dm_bufio_prefetch. The compiler optimizes out the 
> constant expression, so it doesn't matter that it's there.

How about move the conditional inside the do { } ?

I really dislike the style you've used on this, is there anywhere else
in the kernel that does this?

Re: [PATCH v2 7/7] dm snapshot: use bufio prefetch

[Mikulas Patocka]

On Mon, 13 Jan 2014, Mike Snitzer wrote:

> On Mon, Jan 13 2014 at  6:45pm -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > 
> > 
> > On Mon, 13 Jan 2014, Mike Snitzer wrote:
> > 
> > > On Mon, Jan 13 2014 at  5:00pm -0500,
> > > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > > 
> > > > No.
> > > > 
> > > > This changed patch inefficiently loops in bufio_prefetch_chunks for each 
> > > > buffer that is read.
> > > 
> > > Yeah, I overlooked the importance of preserving prefetch_area (which
> > > explains the unlikely you had).  But my intent was to make your code
> > > less "special"; your if (DM_PREFETCH_CHUNKS) do { } while() code to
> > > avoid the extra indentation is pretty ugly.
> > > 
> > > Given your current code, DM_PREFETCH_CHUNKS is always 12 so why not just
> > > remove the check?
> > 
> > If someone ever changes it to a variable, the condition is there avoid 
> > unneeded call to dm_bufio_prefetch. The compiler optimizes out the 
> > constant expression, so it doesn't matter that it's there.
> 
> How about move the conditional inside the do { } ?
> 
> I really dislike the style you've used on this, is there anywhere else
> in the kernel that does this?

grep -r 'if (.*) do {' *

shows

arch/hexagon/include/asm/cmpxchg.h:     if (size != 4) do { asm volatile("brkpt;\n"); } while (1);
drivers/media/usb/tm6000/tm6000-i2c.c:#define i2c_dprintk(lvl, fmt, args...) if (i2c_debug >= lvl) do { \
drivers/net/wireless/airo.c:    if (rc == SUCCESS) do {
drivers/net/wireless/airo.c:    if (rc == SUCCESS) do {
include/linux/zutil.h:        if (k != 0) do {
lib/lzo/lzo1x_compress.c:                               if (t > 0) do {
lib/lzo/lzo1x_compress.c:               if (t >= 16) do {
lib/lzo/lzo1x_compress.c:               if (t > 0) do {
lib/zlib_deflate/deftree.c:    if (s->last_lit != 0) do {

I don't think it's that problematic.

Mikulas

Re: [PATCH 1/7] dm-bufio: return success or failure on prefetch

[Mike Snitzer]

On Sat, Jan 11 2014 at 12:29pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> This patch makes dm_bufio_prefetch return 1 on success and 0 on failure.
> A prefetch failure may happen in case of temporary memory shortage.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-bufio.c |    8 ++++++--
>  drivers/md/dm-bufio.h |    4 ++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> Index: linux-3.13-rc7/drivers/md/dm-bufio.c
> ===================================================================
> --- linux-3.13-rc7.orig/drivers/md/dm-bufio.c	2014-01-09 16:11:31.000000000 +0100
> +++ linux-3.13-rc7/drivers/md/dm-bufio.c	2014-01-09 16:16:43.000000000 +0100
> @@ -1068,10 +1068,11 @@ void *dm_bufio_new(struct dm_bufio_clien
>  }
>  EXPORT_SYMBOL_GPL(dm_bufio_new);
>  
> -void dm_bufio_prefetch(struct dm_bufio_client *c,
> +int dm_bufio_prefetch(struct dm_bufio_client *c,
>  		       sector_t block, unsigned n_blocks)
>  {
>  	struct blk_plug plug;
> +	int success = 1;
>  
>  	LIST_HEAD(write_list);
>  

I'd prefer to see a bool used here.  Or an actual error code like
-ENOMEM.

> @@ -1104,13 +1105,16 @@ void dm_bufio_prefetch(struct dm_bufio_c
>  			if (!n_blocks)
>  				goto flush_plug;
>  			dm_bufio_lock(c);
> -		}
> +		} else
> +			success = 0;

Why do you continue to do work if a prefetch failed?  Why not return
early here?

Re: [PATCH 1/7] dm-bufio: return success or failure on prefetch

[Mikulas Patocka]

On Mon, 13 Jan 2014, Mike Snitzer wrote:

> On Sat, Jan 11 2014 at 12:29pm -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > This patch makes dm_bufio_prefetch return 1 on success and 0 on failure.
> > A prefetch failure may happen in case of temporary memory shortage.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > ---
> >  drivers/md/dm-bufio.c |    8 ++++++--
> >  drivers/md/dm-bufio.h |    4 ++--
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > Index: linux-3.13-rc7/drivers/md/dm-bufio.c
> > ===================================================================
> > --- linux-3.13-rc7.orig/drivers/md/dm-bufio.c	2014-01-09 16:11:31.000000000 +0100
> > +++ linux-3.13-rc7/drivers/md/dm-bufio.c	2014-01-09 16:16:43.000000000 +0100
> > @@ -1068,10 +1068,11 @@ void *dm_bufio_new(struct dm_bufio_clien
> >  }
> >  EXPORT_SYMBOL_GPL(dm_bufio_new);
> >  
> > -void dm_bufio_prefetch(struct dm_bufio_client *c,
> > +int dm_bufio_prefetch(struct dm_bufio_client *c,
> >  		       sector_t block, unsigned n_blocks)
> >  {
> >  	struct blk_plug plug;
> > +	int success = 1;
> >  
> >  	LIST_HEAD(write_list);
> >  
> 
> I'd prefer to see a bool used here.  Or an actual error code like
> -ENOMEM.
> 
> > @@ -1104,13 +1105,16 @@ void dm_bufio_prefetch(struct dm_bufio_c
> >  			if (!n_blocks)
> >  				goto flush_plug;
> >  			dm_bufio_lock(c);
> > -		}
> > +		} else
> > +			success = 0;
> 
> Why do you continue to do work if a prefetch failed?  Why not return
> early here?

I realized that the patch for the return values was wrong (it would return 
0 if some buffers were already prefetched).

And it's not really needed, if dm_bufio_prefetch doesn't do prefetch 
because of memory shortage, we can do nothing as dm_bufio_read will read 
that buffer anyway.

So, I am sending next 6-patch series, it's the same without the first 
patch.

Mikulas