Skip to contentSkip to conversation

dm-snapshot prefetch patches

PATCH In review Mikulas Patocka 2 participants 18 messages Jan 11, 2014 – Jan 14, 2014 Device mapper

lore
Share & tools
lore

Mikulas Patocka

author maintainer
#0 lore raw

cover dm-snapshot prefetch patches

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

author maintainer
#1 lore raw

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

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 <[email protected]>
---
 drivers/md/dm-bufio.c |    8 ++++++--
 drivers/md/dm-bufio.h |    4 ++--
 2 files changed, 8 insertions(+), 4 deletions(-)
drivers/md/dm-bufio.c +6 -2
Index: linux-3.13-rc7/drivers/md/dm-bufio.c
===================================================================
@@ -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);
drivers/md/dm-bufio.h +2 -2
Index: linux-3.13-rc7/drivers/md/dm-bufio.h
===================================================================
@@ -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
author maintainer
#2 lore raw

PATCH 2/7 dm-bufio: introduce dm_bufio_forget

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

Signed-off-by Mikulas Patocka <[email protected]>
---
 drivers/md/dm-bufio.c |   22 ++++++++++++++++++++++
 drivers/md/dm-bufio.h |    7 +++++++
 2 files changed, 29 insertions(+)
drivers/md/dm-bufio.c +22 -0
Index: linux-3.13-rc7/drivers/md/dm-bufio.c
===================================================================
@@ -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;
drivers/md/dm-bufio.h +7 -0
Index: linux-3.13-rc7/drivers/md/dm-bufio.h
===================================================================
@@ -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);
author maintainer
#3 lore raw

PATCH 3/7 dm-bufio: introduce dm_bufio_set_minimum_buffers

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 <[email protected]>
---
 drivers/md/dm-bufio.c |   14 ++++++++++++--
 drivers/md/dm-bufio.h |    5 +++++
 2 files changed, 17 insertions(+), 2 deletions(-)
drivers/md/dm-bufio.c +12 -2
Index: linux-3.13-rc7/drivers/md/dm-bufio.c
===================================================================
@@ -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;
drivers/md/dm-bufio.h +5 -0
Index: linux-3.13-rc7/drivers/md/dm-bufio.h
===================================================================
@@ -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);
author maintainer
#4 lore raw

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

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 <[email protected]>
---
 drivers/md/dm-snap.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/md/dm-snap.c +5 -5
Index: linux-3.13-rc7/drivers/md/dm-snap.c
===================================================================
@@ -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);
author maintainer
#5 lore raw

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

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 <[email protected]>
---
 drivers/md/dm-snap-persistent.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)
drivers/md/dm-snap-persistent.c +14 -12
Index: linux-3.13-rc7/drivers/md/dm-snap-persistent.c
===================================================================
@@ -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;
author maintainer
#6 lore raw

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

Use dm-bufio for initial loading of the exceptions.

Signed-off-by Mikulas Patocka <[email protected]>
---
 drivers/md/dm-snap-persistent.c |   39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)
drivers/md/dm-snap-persistent.c +32 -7
Index: linux-3.13-rc7/drivers/md/dm-snap-persistent.c
===================================================================
@@ -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)
author maintainer
#7 lore raw

PATCH 7/7 dm-snapshot: use bufio prefetch

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 <[email protected]>
---
 drivers/md/dm-snap-persistent.c |   23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
drivers/md/dm-snap-persistent.c +22 -1
Index: linux-3.13-rc7/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.

@@ -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))) {
#8 lore raw

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

Quoted 17 lines · 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 <[email protected]>
---
 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
===================================================================

drivers/md/dm-snap-persistent.c line 22

Quoted 5 lines
drivers/md/dm-snap-persistent.c +2 -0
@@ -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.

Quoted 36 lines
drivers/md/dm-snap-persistent.c +13 -1
 /*-----------------------------------------------------------------
  * 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)))

drivers/md/dm-snap-persistent.c static int read_exceptions(struct pstore · line 538

Quoted 10 lines
drivers/md/dm-snap-persistent.c +7 -0
+				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.

author maintainer
#9 lore raw

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

Quoted 2 lines · Mike Snitzer

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).

Quoted 1 line

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.

Quoted 3 lines

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

#10 lore raw

PATCH v2 7/7 dm snapshot: use bufio prefetch

In reply to Mikulas Patocka

From: Mikulas Patocka <[email protected]>

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 <[email protected]>
Signed-off-by Mike Snitzer <[email protected]>
---
 drivers/md/dm-snap-persistent.c |   34 +++++++++++++++++++++++++++++++++-
 1 files changed, 33 insertions(+), 1 deletions(-)
drivers/md/dm-snap-persistent.c +33 -1
index 1692750..ea45b3b 100644
@@ -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))) {
Signature · 1 line
1.7.4.4
author maintainer
#11 lore raw

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

No.

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

Mikulas

Quoted 85 lines · Mike Snitzer

From: Mikulas Patocka <[email protected]>

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 <[email protected]>
Signed-off-by Mike Snitzer <[email protected]>
---
 drivers/md/dm-snap-persistent.c |   34 +++++++++++++++++++++++++++++++++-
 1 files changed, 33 insertions(+), 1 deletions(-)
drivers/md/dm-snap-persistent.c +33 -1
index 1692750..ea45b3b 100644
@@ -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))) {
Signature · 1 line
1.7.4.4
#12 lore raw

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

Quoted 4 lines · Mikulas Patocka

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?

author maintainer
#13 lore raw

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

Quoted 8 lines · Mike Snitzer
Quoted 4 lines · Mikulas Patocka

No.

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

Quoted 7 lines

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

#14 lore raw

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

Quoted 17 lines · Mikulas Patocka
Quoted 13 lines · Mike Snitzer
Quoted 4 lines · Mikulas Patocka

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.

Quoted 6 lines
Quoted 2 lines

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?

author maintainer
#15 lore raw

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

Quoted 23 lines · Mike Snitzer
Quoted 20 lines · Mikulas Patocka
Quoted 15 lines · Mike Snitzer
Quoted 4 lines · Mikulas Patocka

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?

Quoted 8 lines
Quoted 3 lines

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

#16 lore raw

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

In reply to Mikulas Patocka
Quoted 17 lines · 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 <[email protected]>
---
 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
===================================================================

drivers/md/dm-bufio.c +0 -0
@@ -1068,10 +1068,11 @@ void *dm_bufio_new(struct dm_bufio_clien
 }
 EXPORT_SYMBOL_GPL(dm_bufio_new);

drivers/md/dm-bufio.c void *dm_bufio_new(struct dm_bufio_clien · line 1078

Quoted 10 lines
drivers/md/dm-bufio.c +2 -1
 
-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.

drivers/md/dm-bufio.c void dm_bufio_prefetch(struct dm_bufio_c · line 1109

Quoted 7 lines
drivers/md/dm-bufio.c +2 -1
@@ -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?

author maintainer
#17 lore raw

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

Quoted 34 lines · Mike Snitzer
Quoted 17 lines · 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 <[email protected]>
---
 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
===================================================================

drivers/md/dm-bufio.c +0 -0
@@ -1068,10 +1068,11 @@ void *dm_bufio_new(struct dm_bufio_clien
 }
 EXPORT_SYMBOL_GPL(dm_bufio_new);

drivers/md/dm-bufio.c void *dm_bufio_new(struct dm_bufio_clien · line 1078

Quoted 10 lines
drivers/md/dm-bufio.c +2 -1
 
-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.

Quoted 10 lines

drivers/md/dm-bufio.c void dm_bufio_prefetch(struct dm_bufio_c · line 1109

Quoted 7 lines
drivers/md/dm-bufio.c +2 -1
@@ -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