[PATCH] dm-bufio

[PATCH] dm-bufio

[Mikulas Patocka]

Hi

This is a patch for dm-bufio.

Changes:
* fixed a bug in i/o submission, introduced by someone who changed the 
code
* simplified some constructs
* more likely/unlikely hints
* dm-bufio.h moved from drivers/md to include/linux
* put cond_resched() to loops (it was there originally and then someone 
deleted it)

Mikulas

---
 drivers/md/Kconfig       |    2 
 drivers/md/dm-bufio.c    |   95 ++++++++++++++++++++++++----------------
 drivers/md/dm-bufio.h    |  110 -----------------------------------------------
 include/linux/dm-bufio.h |  110 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 169 insertions(+), 148 deletions(-)

Index: linux-3.1-rc3-fast/drivers/md/Kconfig
===================================================================
--- linux-3.1-rc3-fast.orig/drivers/md/Kconfig	2011-10-14 20:56:45.000000000 +0200
+++ linux-3.1-rc3-fast/drivers/md/Kconfig	2011-10-14 20:56:46.000000000 +0200
@@ -209,7 +209,7 @@ config DM_DEBUG
 	  If unsure, say N.
 
 config DM_BUFIO
-       tristate
+       tristate "Buffered IO"
        depends on BLK_DEV_DM && EXPERIMENTAL
        ---help---
 	 This interface allows you to do buffered I/O on a device and acts
Index: linux-3.1-rc3-fast/drivers/md/dm-bufio.c
===================================================================
--- linux-3.1-rc3-fast.orig/drivers/md/dm-bufio.c	2011-10-14 20:56:45.000000000 +0200
+++ linux-3.1-rc3-fast/drivers/md/dm-bufio.c	2011-10-14 20:57:10.000000000 +0200
@@ -167,6 +167,12 @@ static void dm_bufio_unlock(struct dm_bu
 	mutex_unlock(&c->lock);
 }
 
+#ifdef CONFIG_PREEMPT
+#define dm_bufio_cond_resched()		do { } while (0)
+#else
+#define dm_bufio_cond_resched()		cond_resched()
+#endif
+
 /*----------------------------------------------------------------*/
 
 /* Default cache size --- available memory divided by the ratio */
@@ -470,17 +476,18 @@ static void use_inline_bio(struct dm_buf
 	ptr = b->data;
 	len = b->c->block_size;
 
-	if (len >= PAGE_SIZE)
+	if (likely(len >= PAGE_SIZE))
 		BUG_ON((unsigned long)ptr & (PAGE_SIZE - 1));
 	else
 		BUG_ON((unsigned long)ptr & (len - 1));
 
 	do {
-		if (!bio_add_page(&b->bio, virt_to_page(ptr),
-				  len < PAGE_SIZE ? len : PAGE_SIZE,
-				  virt_to_phys(ptr) & (PAGE_SIZE - 1))) {
+		if (unlikely(!bio_add_page(&b->bio, virt_to_page(ptr),
+					len < PAGE_SIZE ? len : PAGE_SIZE,
+					virt_to_phys(ptr) & (PAGE_SIZE - 1)))) {
 			BUG_ON(b->c->block_size <= PAGE_SIZE);
 			use_dmio(b, rw, block, end_io);
+			return;
 		}
 
 		len -= PAGE_SIZE;
@@ -493,8 +500,10 @@ static void use_inline_bio(struct dm_buf
 static void submit_io(struct dm_buffer *b, int rw, sector_t block,
 		      bio_end_io_t *end_io)
 {
-	if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
-	    b->data_mode != DATA_MODE_VMALLOC)
+	if (rw == WRITE && b->c->write_callback)
+		b->c->write_callback(b);
+	if (likely(b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE) &&
+	    likely(b->data_mode != DATA_MODE_VMALLOC))
 		use_inline_bio(b, rw, block, end_io);
 	else
 		use_dmio(b, rw, block, end_io);
@@ -514,7 +523,7 @@ static void write_endio(struct bio *bio,
 {
 	struct dm_buffer *b = container_of(bio, struct dm_buffer, bio);
 	b->write_error = error;
-	if (error) {
+	if (unlikely(error)) {
 		struct dm_bufio_client *c = b->c;
 		(void)cmpxchg(&c->async_write_error, 0, error);
 	}
@@ -550,8 +559,6 @@ static void __write_dirty_buffer(struct 
 	clear_bit(B_DIRTY, &b->state);
 	wait_on_bit_lock(&b->state, B_WRITING,
 			 do_io_schedule, TASK_UNINTERRUPTIBLE);
-	if (b->c->write_callback)
-		b->c->write_callback(b);
 	submit_io(b, WRITE, b->block, write_endio);
 }
 
@@ -572,7 +579,7 @@ static void __make_buffer_clean(struct d
 
 /*
  * Find some buffer that is not held by anybody, clean it, unlink it and
- * return it.  If "wait" is zero, try less hard and don't block.
+ * return it.
  */
 static struct dm_buffer *__get_unclaimed_buffer(struct dm_bufio_client *c)
 {
@@ -585,6 +592,7 @@ static struct dm_buffer *__get_unclaimed
 			__unlink_buffer(b);
 			return b;
 		}
+		dm_bufio_cond_resched();
 	}
 
 	list_for_each_entry_reverse(b, &c->lru[LIST_DIRTY], lru_list) {
@@ -594,6 +602,7 @@ static struct dm_buffer *__get_unclaimed
 			__unlink_buffer(b);
 			return b;
 		}
+		dm_bufio_cond_resched();
 	}
 	return NULL;
 }
@@ -636,18 +645,21 @@ static struct dm_buffer *__alloc_buffer_
 		 * This is useful for debugging. When we set cache size to 1,
 		 * no new buffers are allocated at all.
 		 */
-		if (dm_bufio_cache_size_latch != 1) {
+		if (likely(dm_bufio_cache_size_latch != 1)) {
 			/*
-			 * dm-bufio is resistant to allocation failures (it just keeps
-			 * one buffer reserved in cases all the allocations fail).
+			 * dm-bufio is resistant to allocation failures (it
+			 * just keeps one buffer reserved in cases all the
+			 * allocations fail).
 			 * So set flags to not try too hard:
 			 *	GFP_NOIO: don't recurse into the I/O layer
-			 *	__GFP_NORETRY: don't retry and rather return failure
+			 *	__GFP_NORETRY: don't retry and rather return
+			 *			failure
 			 *	__GFP_NOMEMALLOC: don't use emergency reserves
-			 *	__GFP_NOWARN: don't print a warning in case of failure
+			 *	__GFP_NOWARN: don't print a warning in case of
+			 *			failure
 			 */
 			b = alloc_buffer(c, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
-			if (b)
+			if (likely(b != NULL))
 				return b;
 		}
 
@@ -670,7 +682,7 @@ static struct dm_buffer *__alloc_buffer_
 static struct dm_buffer *__alloc_buffer_wait(struct dm_bufio_client *c)
 {
 	struct dm_buffer *b = __alloc_buffer_wait_no_callback(c);
-	if (b && c->alloc_callback)
+	if (c->alloc_callback)
 		c->alloc_callback(b);
 	return b;
 }
@@ -682,7 +694,7 @@ static void __free_buffer_wake(struct dm
 {
 	struct dm_bufio_client *c = b->c;
 
-	if (c->need_reserved_buffers) {
+	if (unlikely(c->need_reserved_buffers != 0)) {
 		list_add(&b->lru_list, &c->reserved_buffers);
 		c->need_reserved_buffers--;
 	} else
@@ -704,6 +716,7 @@ static void __write_dirty_buffers_async(
 		if (no_wait && test_bit(B_WRITING, &b->state))
 			return;
 		__write_dirty_buffer(b);
+		dm_bufio_cond_resched();
 	}
 }
 
@@ -716,7 +729,7 @@ static void __get_memory_limit(struct dm
 {
 	unsigned long buffers;
 
-	if (dm_bufio_cache_size != dm_bufio_cache_size_latch) {
+	if (unlikely(dm_bufio_cache_size != dm_bufio_cache_size_latch)) {
 		mutex_lock(&dm_bufio_clients_lock);
 		__cache_size_refresh();
 		mutex_unlock(&dm_bufio_clients_lock);
@@ -724,7 +737,7 @@ 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)
+	if (unlikely(buffers < DM_BUFIO_MIN_BUFFERS))
 		buffers = DM_BUFIO_MIN_BUFFERS;
 	*limit_buffers = buffers;
 	*threshold_buffers = buffers * DM_BUFIO_WRITEBACK_PERCENT / 100;
@@ -747,6 +760,7 @@ static void __check_watermark(struct dm_
 		if (!b)
 			return;
 		__free_buffer_wake(b);
+		dm_bufio_cond_resched();
 	}
 	if (c->n_buffers[LIST_DIRTY] > threshold_buffers)
 		__write_dirty_buffers_async(c, 1);
@@ -758,8 +772,9 @@ static struct dm_buffer *__find(struct d
 	struct dm_buffer *b;
 	struct hlist_node *hn;
 	hlist_for_each_entry(b, hn, &c->cache_hash[DM_BUFIO_HASH(block)], hash_list) {
-		if (b->block == block)
+		if (likely(b->block == block))
 			return b;
+		dm_bufio_cond_resched();
 	}
 
 	return NULL;
@@ -775,12 +790,13 @@ enum new_flag {
 	NF_GET = 2
 };
 
-static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
-				     struct dm_buffer **bp, int *need_submit)
+static void read_endio(struct bio *bio, int error);
+
+static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block,
+				     enum new_flag nf, struct dm_buffer **bp)
 {
 	struct dm_buffer *b, *new_b = NULL;
 
-	*need_submit = 0;
 	b = __find(c, block);
 	if (b) {
 		b->hold_count++;
@@ -821,7 +837,9 @@ static struct dm_buffer *__bufio_new(str
 	}
 
 	b->state = 1 << B_READING;
-	*need_submit = 1;
+
+	submit_io(b, READ, b->block, read_endio);
+
 	return b;
 }
 
@@ -849,22 +867,18 @@ static void read_endio(struct bio *bio, 
 static void *new_read(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
 		      struct dm_buffer **bp)
 {
-	int need_submit;
 	struct dm_buffer *b;
 
 	dm_bufio_lock(c);
-	b = __bufio_new(c, block, nf, bp, &need_submit);
+	b = __bufio_new(c, block, nf, bp);
 	dm_bufio_unlock(c);
 
-	if (!b || IS_ERR(b))
+	if (unlikely(!b) || unlikely(IS_ERR(b)))
 		return b;
 	else {
-		if (need_submit)
-			submit_io(b, READ, b->block, read_endio);
-
 		wait_on_bit(&b->state, B_READING,
 			    do_io_schedule, TASK_UNINTERRUPTIBLE);
-		if (b->read_error != 0) {
+		if (unlikely(b->read_error != 0)) {
 			int error = b->read_error;
 			dm_bufio_release(b);
 			return ERR_PTR(error);
@@ -904,7 +918,7 @@ void dm_bufio_release(struct dm_buffer *
 	BUG_ON(test_bit(B_READING, &b->state));
 	BUG_ON(!b->hold_count);
 	b->hold_count--;
-	if (!b->hold_count) {
+	if (likely(!b->hold_count)) {
 		wake_up(&c->free_buffer_wait);
 
 		/*
@@ -912,7 +926,7 @@ void dm_bufio_release(struct dm_buffer *
 		 * to be written, free the buffer. There is no point in caching
 		 * invalid buffer.
 		 */
-		if ((b->read_error || b->write_error) &&
+		if (unlikely((b->read_error | b->write_error) != 0) &&
 		    !test_bit(B_WRITING, &b->state) &&
 		    !test_bit(B_DIRTY, &b->state)) {
 			__unlink_buffer(b);
@@ -999,15 +1013,19 @@ again:
 		 * someone is doing some writes simultaneously with us --- in
 		 * this case, stop dropping the lock.
 		 */
+		dm_bufio_cond_resched();
 		if (dropped_lock)
 			goto again;
 	}
 	wake_up(&c->free_buffer_wait);
 	dm_bufio_unlock(c);
 
-	a = xchg(&c->async_write_error, 0);
+	if (likely(!c->async_write_error))
+		a = 0;
+	else
+		a = xchg(&c->async_write_error, 0);
 	f = dm_bufio_issue_flush(c);
-	if (a)
+	if (unlikely(a != 0))
 		return a;
 	return f;
 }
@@ -1071,7 +1089,7 @@ retry:
 	BUG_ON(!b->hold_count);
 	BUG_ON(test_bit(B_READING, &b->state));
 	__write_dirty_buffer(b);
-	if (b->hold_count == 1) {
+	if (likely(b->hold_count == 1)) {
 		wait_on_bit(&b->state, B_WRITING,
 			    do_io_schedule, TASK_UNINTERRUPTIBLE);
 		set_bit(B_DIRTY, &b->state);
@@ -1193,6 +1211,7 @@ static void __scan(struct dm_bufio_clien
 				if (!--nr_to_scan)
 					return;
 			}
+			dm_bufio_cond_resched();
 		}
 	}
 }
@@ -1406,9 +1425,11 @@ static void cleanup_old_buffers(void)
 				       struct dm_buffer, lru_list);
 			if (__cleanup_old_buffer(b, 0, max_age))
 				break;
+			dm_bufio_cond_resched();
 		}
 
 		dm_bufio_unlock(c);
+		dm_bufio_cond_resched();
 	}
 	mutex_unlock(&dm_bufio_clients_lock);
 }
Index: linux-3.1-rc3-fast/drivers/md/dm-bufio.h
===================================================================
--- linux-3.1-rc3-fast.orig/drivers/md/dm-bufio.h	2011-10-14 20:56:45.000000000 +0200
+++ /dev/null	1970-01-01 00:00:00.000000000 +0000
@@ -1,110 +0,0 @@
-/*
- * Copyright (C) 2009-2011 2011 Red Hat, Inc.
- *
- * This file is released under the GPL.
- */
-
-#ifndef DM_BUFIO_H
-#define DM_BUFIO_H
-
-#include <linux/blkdev.h>
-#include <linux/types.h>
-
-/*----------------------------------------------------------------*/
-
-struct dm_bufio_client;
-struct dm_buffer;
-
-/*
- * Create a buffered IO cache on a given device
- */
-struct dm_bufio_client *
-dm_bufio_client_create(struct block_device *bdev, unsigned block_size,
-		       unsigned reserved_buffers, unsigned aux_size,
-		       void (*alloc_callback)(struct dm_buffer *),
-		       void (*write_callback)(struct dm_buffer *));
-
-/*
- * Release a buffered IO cache.
- */
-void dm_bufio_client_destroy(struct dm_bufio_client *c);
-
-/*
- * WARNING: to avoid deadlocks, these conditions are observed:
- *
- * - At most one thread can hold at most "reserved_buffers" simultaneously.
- * - Each other threads can hold at most one buffer.
- * - Threads which call only dm_bufio_get can hold unlimited number of
- *   buffers.
- */
-
-/*
- * Read a given block from disk. Returns pointer to data.  Returns a
- * pointer to dm_buffer that can be used to release the buffer or to make
- * it dirty.
- */
-void *dm_bufio_read(struct dm_bufio_client *c, sector_t block,
-		    struct dm_buffer **bp);
-
-/*
- * Like dm_bufio_read, but return buffer from cache, don't read
- * it. If the buffer is not in the cache, return NULL.
- */
-void *dm_bufio_get(struct dm_bufio_client *c, sector_t block,
-		   struct dm_buffer **bp);
-
-/*
- * Like dm_bufio_read, but don't read anything from the disk.  It is
- * expected that the caller initializes the buffer and marks it dirty.
- */
-void *dm_bufio_new(struct dm_bufio_client *c, sector_t block,
-		   struct dm_buffer **bp);
-
-/*
- * Release a reference obtained with dm_bufio_{read,get,new}. The data
- * pointer and dm_buffer pointer is no longer valid after this call.
- */
-void dm_bufio_release(struct dm_buffer *b);
-
-/*
- * Mark a buffer dirty. It should be called after the buffer is modified.
- *
- * In case of memory pressure, the buffer may be written after
- * dm_bufio_mark_buffer_dirty, but before dm_bufio_write_dirty_buffers.  So
- * dm_bufio_write_dirty_buffers guarantees that the buffer is on-disk but
- * the actual writing may occur earlier.
- */
-void dm_bufio_mark_buffer_dirty(struct dm_buffer *b);
-
-/*
- * Initiate writing of dirty buffers, without waiting for completion.
- */
-void dm_bufio_write_dirty_buffers_async(struct dm_bufio_client *c);
-
-/*
- * Write all dirty buffers. Guarantees that all dirty buffers created prior
- * to this call are on disk when this call exits.
- */
-int dm_bufio_write_dirty_buffers(struct dm_bufio_client *c);
-
-/*
- * Send an empty write barrier to the device to flush hardware disk cache.
- */
-int dm_bufio_issue_flush(struct dm_bufio_client *c);
-
-/*
- * Like dm_bufio_release but also move the buffer to the new
- * block. dm_bufio_write_dirty_buffers is needed to commit the new block.
- */
-void dm_bufio_release_move(struct dm_buffer *b, sector_t new_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);
-void *dm_bufio_get_block_data(struct dm_buffer *b);
-void *dm_bufio_get_aux_data(struct dm_buffer *b);
-struct dm_bufio_client *dm_bufio_get_client(struct dm_buffer *b);
-
-/*----------------------------------------------------------------*/
-
-#endif
Index: linux-3.1-rc3-fast/include/linux/dm-bufio.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-3.1-rc3-fast/include/linux/dm-bufio.h	2011-10-14 20:56:46.000000000 +0200
@@ -0,0 +1,110 @@
+/*
+ * Copyright (C) 2009-2011 2011 Red Hat, Inc.
+ *
+ * This file is released under the GPL.
+ */
+
+#ifndef DM_BUFIO_H
+#define DM_BUFIO_H
+
+#include <linux/blkdev.h>
+#include <linux/types.h>
+
+/*----------------------------------------------------------------*/
+
+struct dm_bufio_client;
+struct dm_buffer;
+
+/*
+ * Create a buffered IO cache on a given device
+ */
+struct dm_bufio_client *
+dm_bufio_client_create(struct block_device *bdev, unsigned block_size,
+		       unsigned reserved_buffers, unsigned aux_size,
+		       void (*alloc_callback)(struct dm_buffer *),
+		       void (*write_callback)(struct dm_buffer *));
+
+/*
+ * Release a buffered IO cache.
+ */
+void dm_bufio_client_destroy(struct dm_bufio_client *c);
+
+/*
+ * WARNING: to avoid deadlocks, these conditions are observed:
+ *
+ * - At most one thread can hold at most "reserved_buffers" simultaneously.
+ * - Each other threads can hold at most one buffer.
+ * - Threads which call only dm_bufio_get can hold unlimited number of
+ *   buffers.
+ */
+
+/*
+ * Read a given block from disk. Returns pointer to data.  Returns a
+ * pointer to dm_buffer that can be used to release the buffer or to make
+ * it dirty.
+ */
+void *dm_bufio_read(struct dm_bufio_client *c, sector_t block,
+		    struct dm_buffer **bp);
+
+/*
+ * Like dm_bufio_read, but return buffer from cache, don't read
+ * it. If the buffer is not in the cache, return NULL.
+ */
+void *dm_bufio_get(struct dm_bufio_client *c, sector_t block,
+		   struct dm_buffer **bp);
+
+/*
+ * Like dm_bufio_read, but don't read anything from the disk.  It is
+ * expected that the caller initializes the buffer and marks it dirty.
+ */
+void *dm_bufio_new(struct dm_bufio_client *c, sector_t block,
+		   struct dm_buffer **bp);
+
+/*
+ * Release a reference obtained with dm_bufio_{read,get,new}. The data
+ * pointer and dm_buffer pointer is no longer valid after this call.
+ */
+void dm_bufio_release(struct dm_buffer *b);
+
+/*
+ * Mark a buffer dirty. It should be called after the buffer is modified.
+ *
+ * In case of memory pressure, the buffer may be written after
+ * dm_bufio_mark_buffer_dirty, but before dm_bufio_write_dirty_buffers.  So
+ * dm_bufio_write_dirty_buffers guarantees that the buffer is on-disk but
+ * the actual writing may occur earlier.
+ */
+void dm_bufio_mark_buffer_dirty(struct dm_buffer *b);
+
+/*
+ * Initiate writing of dirty buffers, without waiting for completion.
+ */
+void dm_bufio_write_dirty_buffers_async(struct dm_bufio_client *c);
+
+/*
+ * Write all dirty buffers. Guarantees that all dirty buffers created prior
+ * to this call are on disk when this call exits.
+ */
+int dm_bufio_write_dirty_buffers(struct dm_bufio_client *c);
+
+/*
+ * Send an empty write barrier to the device to flush hardware disk cache.
+ */
+int dm_bufio_issue_flush(struct dm_bufio_client *c);
+
+/*
+ * Like dm_bufio_release but also move the buffer to the new
+ * block. dm_bufio_write_dirty_buffers is needed to commit the new block.
+ */
+void dm_bufio_release_move(struct dm_buffer *b, sector_t new_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);
+void *dm_bufio_get_block_data(struct dm_buffer *b);
+void *dm_bufio_get_aux_data(struct dm_buffer *b);
+struct dm_bufio_client *dm_bufio_get_client(struct dm_buffer *b);
+
+/*----------------------------------------------------------------*/
+
+#endif

Re: [PATCH] dm-bufio

[Joe Thornber]

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> Hi
> 
> This is a patch for dm-bufio.
> 
> Changes:
> * fixed a bug in i/o submission, introduced by someone who changed the 
> code

Could you point me at the specific part of this patch that does this
please?

> * simplified some constructs
> * more likely/unlikely hints

They clutter the code, and have been used without discrimination.  What
is the point of using it on a slow path?

> * dm-bufio.h moved from drivers/md to include/linux

Who outside dm do you expect to use this?

> * put cond_resched() to loops (it was there originally and then someone 
> deleted it)

If you're going to use cond_resched() at least do so a little more
intelligently than putting it in _every_ loop.  For instance you call it on
every iteration of a sweep through the hash table.  The call to
cond_resched will take more time than the loop body.  At least make a
change so it's only done every n'th iteration.


Can I also point out that I asked you to make a lot of these changes
over a year ago.  You've only got yourself to blame if 'someone' does
it for you.

- Someone




> 
> Mikulas
> 
> ---
>  drivers/md/Kconfig       |    2 
>  drivers/md/dm-bufio.c    |   95 ++++++++++++++++++++++++----------------
>  drivers/md/dm-bufio.h    |  110 -----------------------------------------------
>  include/linux/dm-bufio.h |  110 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 169 insertions(+), 148 deletions(-)
> 
> Index: linux-3.1-rc3-fast/drivers/md/Kconfig
> ===================================================================
> --- linux-3.1-rc3-fast.orig/drivers/md/Kconfig	2011-10-14 20:56:45.000000000 +0200
> +++ linux-3.1-rc3-fast/drivers/md/Kconfig	2011-10-14 20:56:46.000000000 +0200
> @@ -209,7 +209,7 @@ config DM_DEBUG
>  	  If unsure, say N.
>  
>  config DM_BUFIO
> -       tristate
> +       tristate "Buffered IO"
>         depends on BLK_DEV_DM && EXPERIMENTAL
>         ---help---
>  	 This interface allows you to do buffered I/O on a device and acts
> Index: linux-3.1-rc3-fast/drivers/md/dm-bufio.c
> ===================================================================
> --- linux-3.1-rc3-fast.orig/drivers/md/dm-bufio.c	2011-10-14 20:56:45.000000000 +0200
> +++ linux-3.1-rc3-fast/drivers/md/dm-bufio.c	2011-10-14 20:57:10.000000000 +0200
> @@ -167,6 +167,12 @@ static void dm_bufio_unlock(struct dm_bu
>  	mutex_unlock(&c->lock);
>  }
>  
> +#ifdef CONFIG_PREEMPT
> +#define dm_bufio_cond_resched()		do { } while (0)
> +#else
> +#define dm_bufio_cond_resched()		cond_resched()
> +#endif
> +
>  /*----------------------------------------------------------------*/
>  
>  /* Default cache size --- available memory divided by the ratio */
> @@ -470,17 +476,18 @@ static void use_inline_bio(struct dm_buf
>  	ptr = b->data;
>  	len = b->c->block_size;
>  
> -	if (len >= PAGE_SIZE)
> +	if (likely(len >= PAGE_SIZE))
>  		BUG_ON((unsigned long)ptr & (PAGE_SIZE - 1));
>  	else
>  		BUG_ON((unsigned long)ptr & (len - 1));
>  
>  	do {
> -		if (!bio_add_page(&b->bio, virt_to_page(ptr),
> -				  len < PAGE_SIZE ? len : PAGE_SIZE,
> -				  virt_to_phys(ptr) & (PAGE_SIZE - 1))) {
> +		if (unlikely(!bio_add_page(&b->bio, virt_to_page(ptr),
> +					len < PAGE_SIZE ? len : PAGE_SIZE,
> +					virt_to_phys(ptr) & (PAGE_SIZE - 1)))) {
>  			BUG_ON(b->c->block_size <= PAGE_SIZE);
>  			use_dmio(b, rw, block, end_io);
> +			return;
>  		}
>  
>  		len -= PAGE_SIZE;
> @@ -493,8 +500,10 @@ static void use_inline_bio(struct dm_buf
>  static void submit_io(struct dm_buffer *b, int rw, sector_t block,
>  		      bio_end_io_t *end_io)
>  {
> -	if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
> -	    b->data_mode != DATA_MODE_VMALLOC)
> +	if (rw == WRITE && b->c->write_callback)
> +		b->c->write_callback(b);
> +	if (likely(b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE) &&
> +	    likely(b->data_mode != DATA_MODE_VMALLOC))
>  		use_inline_bio(b, rw, block, end_io);
>  	else
>  		use_dmio(b, rw, block, end_io);
> @@ -514,7 +523,7 @@ static void write_endio(struct bio *bio,
>  {
>  	struct dm_buffer *b = container_of(bio, struct dm_buffer, bio);
>  	b->write_error = error;
> -	if (error) {
> +	if (unlikely(error)) {
>  		struct dm_bufio_client *c = b->c;
>  		(void)cmpxchg(&c->async_write_error, 0, error);
>  	}
> @@ -550,8 +559,6 @@ static void __write_dirty_buffer(struct 
>  	clear_bit(B_DIRTY, &b->state);
>  	wait_on_bit_lock(&b->state, B_WRITING,
>  			 do_io_schedule, TASK_UNINTERRUPTIBLE);
> -	if (b->c->write_callback)
> -		b->c->write_callback(b);
>  	submit_io(b, WRITE, b->block, write_endio);
>  }
>  
> @@ -572,7 +579,7 @@ static void __make_buffer_clean(struct d
>  
>  /*
>   * Find some buffer that is not held by anybody, clean it, unlink it and
> - * return it.  If "wait" is zero, try less hard and don't block.
> + * return it.
>   */
>  static struct dm_buffer *__get_unclaimed_buffer(struct dm_bufio_client *c)
>  {
> @@ -585,6 +592,7 @@ static struct dm_buffer *__get_unclaimed
>  			__unlink_buffer(b);
>  			return b;
>  		}
> +		dm_bufio_cond_resched();
>  	}
>  
>  	list_for_each_entry_reverse(b, &c->lru[LIST_DIRTY], lru_list) {
> @@ -594,6 +602,7 @@ static struct dm_buffer *__get_unclaimed
>  			__unlink_buffer(b);
>  			return b;
>  		}
> +		dm_bufio_cond_resched();
>  	}
>  	return NULL;
>  }
> @@ -636,18 +645,21 @@ static struct dm_buffer *__alloc_buffer_
>  		 * This is useful for debugging. When we set cache size to 1,
>  		 * no new buffers are allocated at all.
>  		 */
> -		if (dm_bufio_cache_size_latch != 1) {
> +		if (likely(dm_bufio_cache_size_latch != 1)) {
>  			/*
> -			 * dm-bufio is resistant to allocation failures (it just keeps
> -			 * one buffer reserved in cases all the allocations fail).
> +			 * dm-bufio is resistant to allocation failures (it
> +			 * just keeps one buffer reserved in cases all the
> +			 * allocations fail).
>  			 * So set flags to not try too hard:
>  			 *	GFP_NOIO: don't recurse into the I/O layer
> -			 *	__GFP_NORETRY: don't retry and rather return failure
> +			 *	__GFP_NORETRY: don't retry and rather return
> +			 *			failure
>  			 *	__GFP_NOMEMALLOC: don't use emergency reserves
> -			 *	__GFP_NOWARN: don't print a warning in case of failure
> +			 *	__GFP_NOWARN: don't print a warning in case of
> +			 *			failure
>  			 */
>  			b = alloc_buffer(c, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
> -			if (b)
> +			if (likely(b != NULL))
>  				return b;
>  		}
>  
> @@ -670,7 +682,7 @@ static struct dm_buffer *__alloc_buffer_
>  static struct dm_buffer *__alloc_buffer_wait(struct dm_bufio_client *c)
>  {
>  	struct dm_buffer *b = __alloc_buffer_wait_no_callback(c);
> -	if (b && c->alloc_callback)
> +	if (c->alloc_callback)
>  		c->alloc_callback(b);
>  	return b;
>  }
> @@ -682,7 +694,7 @@ static void __free_buffer_wake(struct dm
>  {
>  	struct dm_bufio_client *c = b->c;
>  
> -	if (c->need_reserved_buffers) {
> +	if (unlikely(c->need_reserved_buffers != 0)) {
>  		list_add(&b->lru_list, &c->reserved_buffers);
>  		c->need_reserved_buffers--;
>  	} else
> @@ -704,6 +716,7 @@ static void __write_dirty_buffers_async(
>  		if (no_wait && test_bit(B_WRITING, &b->state))
>  			return;
>  		__write_dirty_buffer(b);
> +		dm_bufio_cond_resched();
>  	}
>  }
>  
> @@ -716,7 +729,7 @@ static void __get_memory_limit(struct dm
>  {
>  	unsigned long buffers;
>  
> -	if (dm_bufio_cache_size != dm_bufio_cache_size_latch) {
> +	if (unlikely(dm_bufio_cache_size != dm_bufio_cache_size_latch)) {
>  		mutex_lock(&dm_bufio_clients_lock);
>  		__cache_size_refresh();
>  		mutex_unlock(&dm_bufio_clients_lock);
> @@ -724,7 +737,7 @@ 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)
> +	if (unlikely(buffers < DM_BUFIO_MIN_BUFFERS))
>  		buffers = DM_BUFIO_MIN_BUFFERS;
>  	*limit_buffers = buffers;
>  	*threshold_buffers = buffers * DM_BUFIO_WRITEBACK_PERCENT / 100;
> @@ -747,6 +760,7 @@ static void __check_watermark(struct dm_
>  		if (!b)
>  			return;
>  		__free_buffer_wake(b);
> +		dm_bufio_cond_resched();
>  	}
>  	if (c->n_buffers[LIST_DIRTY] > threshold_buffers)
>  		__write_dirty_buffers_async(c, 1);
> @@ -758,8 +772,9 @@ static struct dm_buffer *__find(struct d
>  	struct dm_buffer *b;
>  	struct hlist_node *hn;
>  	hlist_for_each_entry(b, hn, &c->cache_hash[DM_BUFIO_HASH(block)], hash_list) {
> -		if (b->block == block)
> +		if (likely(b->block == block))
>  			return b;
> +		dm_bufio_cond_resched();
>  	}
>  
>  	return NULL;
> @@ -775,12 +790,13 @@ enum new_flag {
>  	NF_GET = 2
>  };
>  
> -static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
> -				     struct dm_buffer **bp, int *need_submit)
> +static void read_endio(struct bio *bio, int error);
> +
> +static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block,
> +				     enum new_flag nf, struct dm_buffer **bp)
>  {
>  	struct dm_buffer *b, *new_b = NULL;
>  
> -	*need_submit = 0;
>  	b = __find(c, block);
>  	if (b) {
>  		b->hold_count++;
> @@ -821,7 +837,9 @@ static struct dm_buffer *__bufio_new(str
>  	}
>  
>  	b->state = 1 << B_READING;
> -	*need_submit = 1;
> +
> +	submit_io(b, READ, b->block, read_endio);
> +
>  	return b;
>  }
>  
> @@ -849,22 +867,18 @@ static void read_endio(struct bio *bio, 
>  static void *new_read(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
>  		      struct dm_buffer **bp)
>  {
> -	int need_submit;
>  	struct dm_buffer *b;
>  
>  	dm_bufio_lock(c);
> -	b = __bufio_new(c, block, nf, bp, &need_submit);
> +	b = __bufio_new(c, block, nf, bp);
>  	dm_bufio_unlock(c);
>  
> -	if (!b || IS_ERR(b))
> +	if (unlikely(!b) || unlikely(IS_ERR(b)))
>  		return b;
>  	else {
> -		if (need_submit)
> -			submit_io(b, READ, b->block, read_endio);
> -
>  		wait_on_bit(&b->state, B_READING,
>  			    do_io_schedule, TASK_UNINTERRUPTIBLE);
> -		if (b->read_error != 0) {
> +		if (unlikely(b->read_error != 0)) {
>  			int error = b->read_error;
>  			dm_bufio_release(b);
>  			return ERR_PTR(error);
> @@ -904,7 +918,7 @@ void dm_bufio_release(struct dm_buffer *
>  	BUG_ON(test_bit(B_READING, &b->state));
>  	BUG_ON(!b->hold_count);
>  	b->hold_count--;
> -	if (!b->hold_count) {
> +	if (likely(!b->hold_count)) {
>  		wake_up(&c->free_buffer_wait);
>  
>  		/*
> @@ -912,7 +926,7 @@ void dm_bufio_release(struct dm_buffer *
>  		 * to be written, free the buffer. There is no point in caching
>  		 * invalid buffer.
>  		 */
> -		if ((b->read_error || b->write_error) &&
> +		if (unlikely((b->read_error | b->write_error) != 0) &&
>  		    !test_bit(B_WRITING, &b->state) &&
>  		    !test_bit(B_DIRTY, &b->state)) {
>  			__unlink_buffer(b);
> @@ -999,15 +1013,19 @@ again:
>  		 * someone is doing some writes simultaneously with us --- in
>  		 * this case, stop dropping the lock.
>  		 */
> +		dm_bufio_cond_resched();
>  		if (dropped_lock)
>  			goto again;
>  	}
>  	wake_up(&c->free_buffer_wait);
>  	dm_bufio_unlock(c);
>  
> -	a = xchg(&c->async_write_error, 0);
> +	if (likely(!c->async_write_error))
> +		a = 0;
> +	else
> +		a = xchg(&c->async_write_error, 0);
>  	f = dm_bufio_issue_flush(c);
> -	if (a)
> +	if (unlikely(a != 0))
>  		return a;
>  	return f;
>  }
> @@ -1071,7 +1089,7 @@ retry:
>  	BUG_ON(!b->hold_count);
>  	BUG_ON(test_bit(B_READING, &b->state));
>  	__write_dirty_buffer(b);
> -	if (b->hold_count == 1) {
> +	if (likely(b->hold_count == 1)) {
>  		wait_on_bit(&b->state, B_WRITING,
>  			    do_io_schedule, TASK_UNINTERRUPTIBLE);
>  		set_bit(B_DIRTY, &b->state);
> @@ -1193,6 +1211,7 @@ static void __scan(struct dm_bufio_clien
>  				if (!--nr_to_scan)
>  					return;
>  			}
> +			dm_bufio_cond_resched();
>  		}
>  	}
>  }
> @@ -1406,9 +1425,11 @@ static void cleanup_old_buffers(void)
>  				       struct dm_buffer, lru_list);
>  			if (__cleanup_old_buffer(b, 0, max_age))
>  				break;
> +			dm_bufio_cond_resched();
>  		}
>  
>  		dm_bufio_unlock(c);
> +		dm_bufio_cond_resched();
>  	}
>  	mutex_unlock(&dm_bufio_clients_lock);
>  }
> Index: linux-3.1-rc3-fast/drivers/md/dm-bufio.h
> ===================================================================
> --- linux-3.1-rc3-fast.orig/drivers/md/dm-bufio.h	2011-10-14 20:56:45.000000000 +0200
> +++ /dev/null	1970-01-01 00:00:00.000000000 +0000
> @@ -1,110 +0,0 @@
> -/*
> - * Copyright (C) 2009-2011 2011 Red Hat, Inc.
> - *
> - * This file is released under the GPL.
> - */
> -
> -#ifndef DM_BUFIO_H
> -#define DM_BUFIO_H
> -
> -#include <linux/blkdev.h>
> -#include <linux/types.h>
> -
> -/*----------------------------------------------------------------*/
> -
> -struct dm_bufio_client;
> -struct dm_buffer;
> -
> -/*
> - * Create a buffered IO cache on a given device
> - */
> -struct dm_bufio_client *
> -dm_bufio_client_create(struct block_device *bdev, unsigned block_size,
> -		       unsigned reserved_buffers, unsigned aux_size,
> -		       void (*alloc_callback)(struct dm_buffer *),
> -		       void (*write_callback)(struct dm_buffer *));
> -
> -/*
> - * Release a buffered IO cache.
> - */
> -void dm_bufio_client_destroy(struct dm_bufio_client *c);
> -
> -/*
> - * WARNING: to avoid deadlocks, these conditions are observed:
> - *
> - * - At most one thread can hold at most "reserved_buffers" simultaneously.
> - * - Each other threads can hold at most one buffer.
> - * - Threads which call only dm_bufio_get can hold unlimited number of
> - *   buffers.
> - */
> -
> -/*
> - * Read a given block from disk. Returns pointer to data.  Returns a
> - * pointer to dm_buffer that can be used to release the buffer or to make
> - * it dirty.
> - */
> -void *dm_bufio_read(struct dm_bufio_client *c, sector_t block,
> -		    struct dm_buffer **bp);
> -
> -/*
> - * Like dm_bufio_read, but return buffer from cache, don't read
> - * it. If the buffer is not in the cache, return NULL.
> - */
> -void *dm_bufio_get(struct dm_bufio_client *c, sector_t block,
> -		   struct dm_buffer **bp);
> -
> -/*
> - * Like dm_bufio_read, but don't read anything from the disk.  It is
> - * expected that the caller initializes the buffer and marks it dirty.
> - */
> -void *dm_bufio_new(struct dm_bufio_client *c, sector_t block,
> -		   struct dm_buffer **bp);
> -
> -/*
> - * Release a reference obtained with dm_bufio_{read,get,new}. The data
> - * pointer and dm_buffer pointer is no longer valid after this call.
> - */
> -void dm_bufio_release(struct dm_buffer *b);
> -
> -/*
> - * Mark a buffer dirty. It should be called after the buffer is modified.
> - *
> - * In case of memory pressure, the buffer may be written after
> - * dm_bufio_mark_buffer_dirty, but before dm_bufio_write_dirty_buffers.  So
> - * dm_bufio_write_dirty_buffers guarantees that the buffer is on-disk but
> - * the actual writing may occur earlier.
> - */
> -void dm_bufio_mark_buffer_dirty(struct dm_buffer *b);
> -
> -/*
> - * Initiate writing of dirty buffers, without waiting for completion.
> - */
> -void dm_bufio_write_dirty_buffers_async(struct dm_bufio_client *c);
> -
> -/*
> - * Write all dirty buffers. Guarantees that all dirty buffers created prior
> - * to this call are on disk when this call exits.
> - */
> -int dm_bufio_write_dirty_buffers(struct dm_bufio_client *c);
> -
> -/*
> - * Send an empty write barrier to the device to flush hardware disk cache.
> - */
> -int dm_bufio_issue_flush(struct dm_bufio_client *c);
> -
> -/*
> - * Like dm_bufio_release but also move the buffer to the new
> - * block. dm_bufio_write_dirty_buffers is needed to commit the new block.
> - */
> -void dm_bufio_release_move(struct dm_buffer *b, sector_t new_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);
> -void *dm_bufio_get_block_data(struct dm_buffer *b);
> -void *dm_bufio_get_aux_data(struct dm_buffer *b);
> -struct dm_bufio_client *dm_bufio_get_client(struct dm_buffer *b);
> -
> -/*----------------------------------------------------------------*/
> -
> -#endif
> Index: linux-3.1-rc3-fast/include/linux/dm-bufio.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-3.1-rc3-fast/include/linux/dm-bufio.h	2011-10-14 20:56:46.000000000 +0200
> @@ -0,0 +1,110 @@
> +/*
> + * Copyright (C) 2009-2011 2011 Red Hat, Inc.
> + *
> + * This file is released under the GPL.
> + */
> +
> +#ifndef DM_BUFIO_H
> +#define DM_BUFIO_H
> +
> +#include <linux/blkdev.h>
> +#include <linux/types.h>
> +
> +/*----------------------------------------------------------------*/
> +
> +struct dm_bufio_client;
> +struct dm_buffer;
> +
> +/*
> + * Create a buffered IO cache on a given device
> + */
> +struct dm_bufio_client *
> +dm_bufio_client_create(struct block_device *bdev, unsigned block_size,
> +		       unsigned reserved_buffers, unsigned aux_size,
> +		       void (*alloc_callback)(struct dm_buffer *),
> +		       void (*write_callback)(struct dm_buffer *));
> +
> +/*
> + * Release a buffered IO cache.
> + */
> +void dm_bufio_client_destroy(struct dm_bufio_client *c);
> +
> +/*
> + * WARNING: to avoid deadlocks, these conditions are observed:
> + *
> + * - At most one thread can hold at most "reserved_buffers" simultaneously.
> + * - Each other threads can hold at most one buffer.
> + * - Threads which call only dm_bufio_get can hold unlimited number of
> + *   buffers.
> + */
> +
> +/*
> + * Read a given block from disk. Returns pointer to data.  Returns a
> + * pointer to dm_buffer that can be used to release the buffer or to make
> + * it dirty.
> + */
> +void *dm_bufio_read(struct dm_bufio_client *c, sector_t block,
> +		    struct dm_buffer **bp);
> +
> +/*
> + * Like dm_bufio_read, but return buffer from cache, don't read
> + * it. If the buffer is not in the cache, return NULL.
> + */
> +void *dm_bufio_get(struct dm_bufio_client *c, sector_t block,
> +		   struct dm_buffer **bp);
> +
> +/*
> + * Like dm_bufio_read, but don't read anything from the disk.  It is
> + * expected that the caller initializes the buffer and marks it dirty.
> + */
> +void *dm_bufio_new(struct dm_bufio_client *c, sector_t block,
> +		   struct dm_buffer **bp);
> +
> +/*
> + * Release a reference obtained with dm_bufio_{read,get,new}. The data
> + * pointer and dm_buffer pointer is no longer valid after this call.
> + */
> +void dm_bufio_release(struct dm_buffer *b);
> +
> +/*
> + * Mark a buffer dirty. It should be called after the buffer is modified.
> + *
> + * In case of memory pressure, the buffer may be written after
> + * dm_bufio_mark_buffer_dirty, but before dm_bufio_write_dirty_buffers.  So
> + * dm_bufio_write_dirty_buffers guarantees that the buffer is on-disk but
> + * the actual writing may occur earlier.
> + */
> +void dm_bufio_mark_buffer_dirty(struct dm_buffer *b);
> +
> +/*
> + * Initiate writing of dirty buffers, without waiting for completion.
> + */
> +void dm_bufio_write_dirty_buffers_async(struct dm_bufio_client *c);
> +
> +/*
> + * Write all dirty buffers. Guarantees that all dirty buffers created prior
> + * to this call are on disk when this call exits.
> + */
> +int dm_bufio_write_dirty_buffers(struct dm_bufio_client *c);
> +
> +/*
> + * Send an empty write barrier to the device to flush hardware disk cache.
> + */
> +int dm_bufio_issue_flush(struct dm_bufio_client *c);
> +
> +/*
> + * Like dm_bufio_release but also move the buffer to the new
> + * block. dm_bufio_write_dirty_buffers is needed to commit the new block.
> + */
> +void dm_bufio_release_move(struct dm_buffer *b, sector_t new_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);
> +void *dm_bufio_get_block_data(struct dm_buffer *b);
> +void *dm_bufio_get_aux_data(struct dm_buffer *b);
> +struct dm_bufio_client *dm_bufio_get_client(struct dm_buffer *b);
> +
> +/*----------------------------------------------------------------*/
> +
> +#endif

Re: [PATCH] dm-bufio

[Joe Thornber]

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> --- linux-3.1-rc3-fast.orig/drivers/md/Kconfig	2011-10-14 20:56:45.000000000 +0200
> +++ linux-3.1-rc3-fast/drivers/md/Kconfig	2011-10-14 20:56:46.000000000 +0200
> @@ -209,7 +209,7 @@ config DM_DEBUG
>  	  If unsure, say N.
>  
>  config DM_BUFIO
> -       tristate
> +       tristate "Buffered IO"
>         depends on BLK_DEV_DM && EXPERIMENTAL
>         ---help---
>  	 This interface allows you to do buffered I/O on a device and acts

This makes bufio appear in 'menuconfig' and friends.  Why do you want this user visible?

Re: [PATCH] dm-bufio

[Joe Thornber]

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
>  			use_dmio(b, rw, block, end_io);
> +			return;

ACK

Re: [PATCH] dm-bufio

[Joe Thornber]

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> @@ -572,7 +579,7 @@ static void __make_buffer_clean(struct d
>  
>  /*
>   * Find some buffer that is not held by anybody, clean it, unlink it and
> - * return it.  If "wait" is zero, try less hard and don't block.
> + * return it.
>   */

ACK

Re: [PATCH] dm-bufio

[Joe Thornber]

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> -static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
> -				     struct dm_buffer **bp, int *need_submit)
> +static void read_endio(struct bio *bio, int error);
> +
> +static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block,
> +				     enum new_flag nf, struct dm_buffer **bp)
>  {
>  	struct dm_buffer *b, *new_b = NULL;
>  
> -	*need_submit = 0;
>  	b = __find(c, block);
>  	if (b) {
>  		b->hold_count++;
> @@ -821,7 +837,9 @@ static struct dm_buffer *__bufio_new(str
>  	}
>  
>  	b->state = 1 << B_READING;
> -	*need_submit = 1;
> +
> +	submit_io(b, READ, b->block, read_endio);
> +
>  	return b;
>  }
>  
> @@ -849,22 +867,18 @@ static void read_endio(struct bio *bio, 
>  static void *new_read(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
>  		      struct dm_buffer **bp)
>  {
> -	int need_submit;
>  	struct dm_buffer *b;
>  
>  	dm_bufio_lock(c);
> -	b = __bufio_new(c, block, nf, bp, &need_submit);
> +	b = __bufio_new(c, block, nf, bp);
>  	dm_bufio_unlock(c);
>  
> 	if (!b || IS_ERR(b))
>  		return b;
>  	else {
> -		if (need_submit)
> -			submit_io(b, READ, b->block, read_endio);
> -
>  		wait_on_bit(&b->state, B_READING,
>  			    do_io_schedule, TASK_UNINTERRUPTIBLE);


This change means submit_io(), which can block, will be called with
the client lock held.  I don't see this as an improvement.  NACK.

Re: [PATCH] dm-bufio

[Joe Thornber]

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> @@ -670,7 +682,7 @@ static struct dm_buffer *__alloc_buffer_
>  static struct dm_buffer *__alloc_buffer_wait(struct dm_bufio_client *c)
>  {
>  	struct dm_buffer *b = __alloc_buffer_wait_no_callback(c);
> -	if (b && c->alloc_callback)
> +	if (c->alloc_callback)
>  		c->alloc_callback(b);
>  	return b;
>  }

ACK.

Re: [PATCH] dm-bufio

[Joe Thornber]

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
@@ -493,8 +500,10 @@ static void use_inline_bio(struct dm_buf
 static void submit_io(struct dm_buffer *b, int rw, sector_t block,
                      bio_end_io_t *end_io)
 {
-       if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
-           b->data_mode != DATA_MODE_VMALLOC)
+       if (rw == WRITE && b->c->write_callback)
+               b->c->write_callback(b);
        if (likely(b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE) &&
            likely(b->data_mode != DATA_MODE_VMALLOC))
                use_inline_bio(b, rw, block, end_io);
        else
                use_dmio(b, rw, block, end_io);
@@ -550,8 +559,6 @@ static void __write_dirty_buffer(struct
        clear_bit(B_DIRTY, &b->state);
        wait_on_bit_lock(&b->state, B_WRITING,
                         do_io_schedule, TASK_UNINTERRUPTIBLE);
-       if (b->c->write_callback)
-               b->c->write_callback(b);
        submit_io(b, WRITE, b->block, write_endio);
 }


This doesn't seem an improvement.  Except ... it changes the behaviour
of dm_bufio_release_move().  So was there a preexisting bug in
dm_bufio_release_move() that you're trying to fix with this patch?

Re: [PATCH] dm-bufio

[Joe Thornber]

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
@@ -999,15 +1013,19 @@ again:
                 * someone is doing some writes simultaneously with us --- in
                 * this case, stop dropping the lock.
                 */
                if (dropped_lock)
                        goto again;
        }
        wake_up(&c->free_buffer_wait);
        dm_bufio_unlock(c);

-       a = xchg(&c->async_write_error, 0);
+       if (likely(!c->async_write_error))
+               a = 0;
+       else
+               a = xchg(&c->async_write_error, 0);
        f = dm_bufio_issue_flush(c);


I didn't change this.  Is it also fixing a preexisting issue?

Re: [PATCH] dm-bufio

[Joe Thornber]

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> Hi
> 
> This is a patch for dm-bufio.

I've merged all I'm going to at this point and pushed to thin-dev.

If you put together a patch for the cond_resched stuff I'll take that;
providing you don't call it in a tight loop like here:

> @@ -758,8 +772,9 @@ static struct dm_buffer *__find(struct d
>  	struct dm_buffer *b;
>  	struct hlist_node *hn;
>  	hlist_for_each_entry(b, hn, &c->cache_hash[DM_BUFIO_HASH(block)], hash_list) {
> -		if (b->block == block)
> +		if (likely(b->block == block))
>  			return b;
> +		dm_bufio_cond_resched();
>  	}


I'm not really interested in the likely/unlikely annotations.

- Joe

Re: dm-bufio

[Mike Snitzer]

On Mon, Oct 17 2011 at  6:08am -0400,
Joe Thornber <thornber@redhat.com> wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > Hi
> > 
> > This is a patch for dm-bufio.
> > 
> > Changes:
...
> > * dm-bufio.h moved from drivers/md to include/linux
> 
> Who outside dm do you expect to use this?

External DM target modules may have an interest (not great justification
but...).

Re: [PATCH] dm-bufio

[Mikulas Patocka]

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> @@ -999,15 +1013,19 @@ again:
>                  * someone is doing some writes simultaneously with us --- in
>                  * this case, stop dropping the lock.
>                  */
>                 if (dropped_lock)
>                         goto again;
>         }
>         wake_up(&c->free_buffer_wait);
>         dm_bufio_unlock(c);
> 
> -       a = xchg(&c->async_write_error, 0);
> +       if (likely(!c->async_write_error))
> +               a = 0;
> +       else
> +               a = xchg(&c->async_write_error, 0);
>         f = dm_bufio_issue_flush(c);
> 
> 
> I didn't change this.  Is it also fixing a preexisting issue?

It doesn't fix anything. It just bypasses xchg for the most common case.

Mikulas

Re: [PATCH] dm-bufio

[Mikulas Patocka]

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > -static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
> > -				     struct dm_buffer **bp, int *need_submit)
> > +static void read_endio(struct bio *bio, int error);
> > +
> > +static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block,
> > +				     enum new_flag nf, struct dm_buffer **bp)
> >  {
> >  	struct dm_buffer *b, *new_b = NULL;
> >  
> > -	*need_submit = 0;
> >  	b = __find(c, block);
> >  	if (b) {
> >  		b->hold_count++;
> > @@ -821,7 +837,9 @@ static struct dm_buffer *__bufio_new(str
> >  	}
> >  
> >  	b->state = 1 << B_READING;
> > -	*need_submit = 1;
> > +
> > +	submit_io(b, READ, b->block, read_endio);
> > +
> >  	return b;
> >  }
> >  
> > @@ -849,22 +867,18 @@ static void read_endio(struct bio *bio, 
> >  static void *new_read(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
> >  		      struct dm_buffer **bp)
> >  {
> > -	int need_submit;
> >  	struct dm_buffer *b;
> >  
> >  	dm_bufio_lock(c);
> > -	b = __bufio_new(c, block, nf, bp, &need_submit);
> > +	b = __bufio_new(c, block, nf, bp);
> >  	dm_bufio_unlock(c);
> >  
> > 	if (!b || IS_ERR(b))
> >  		return b;
> >  	else {
> > -		if (need_submit)
> > -			submit_io(b, READ, b->block, read_endio);
> > -
> >  		wait_on_bit(&b->state, B_READING,
> >  			    do_io_schedule, TASK_UNINTERRUPTIBLE);
> 
> 
> This change means submit_io(), which can block, will be called with
> the client lock held.  I don't see this as an improvement.  NACK.

dm-bufio is designed with the possibility that submit_io is called inside 
the lock. There are other places that do it too. But it's true that it 
wasn't called in the lock before.

I think the best thing would be to delete those functions and use the code 
that was there before.

Mikulas

Re: [PATCH] dm-bufio

[Joe Thornber]

On Mon, Oct 17, 2011 at 09:47:05AM -0400, Mikulas Patocka wrote:
> I think the best thing would be to delete those functions and use the code
> that was there before.

I know you do.  However, Alasdair made it clear that he was not
willing to push the code as it was and we don't have time to argue.

Re: [PATCH] dm-bufio

[Mikulas Patocka]

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > Hi
> > 
> > This is a patch for dm-bufio.
> > 
> > Changes:
> > * fixed a bug in i/o submission, introduced by someone who changed the 
> > code
> 
> Could you point me at the specific part of this patch that does this
> please?

this:
                        BUG_ON(b->c->block_size <= PAGE_SIZE);
                        use_dmio(b, rw, block, end_io);
+                       return;


> > * simplified some constructs
> > * more likely/unlikely hints
> 
> They clutter the code, and have been used without discrimination.  What
> is the point of using it on a slow path?

I think I added them only to the possible hot paths.

> > * dm-bufio.h moved from drivers/md to include/linux
> 
> Who outside dm do you expect to use this?

I don't know, Alasdair said that he wants it there (like 
include/linux/dm-io.h for example). BTW dm-bufio has nothing to do with 
device mapper actually, so it can be used by any code.

> > * put cond_resched() to loops (it was there originally and then someone 
> > deleted it)
> 
> If you're going to use cond_resched() at least do so a little more
> intelligently than putting it in _every_ loop.  For instance you call it on
> every iteration of a sweep through the hash table.  The call to
> cond_resched will take more time than the loop body.  At least make a
> change so it's only done every n'th iteration.

I think it would be better to use
#ifndef CONFIG_PREEMPT
if (need_resched()) cond_resched();
#endif

need_resched() is inlined and translates to a single condition.

I don't know why Linux doesn't provide a macro for it, this would be 
useful far beyond dm code.

Mikulas

> Can I also point out that I asked you to make a lot of these changes
> over a year ago.  You've only got yourself to blame if 'someone' does
> it for you.
> 
> - Someone

Re: [PATCH] dm-bufio

[Mikulas Patocka]

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> @@ -493,8 +500,10 @@ static void use_inline_bio(struct dm_buf
>  static void submit_io(struct dm_buffer *b, int rw, sector_t block,
>                       bio_end_io_t *end_io)
>  {
> -       if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
> -           b->data_mode != DATA_MODE_VMALLOC)
> +       if (rw == WRITE && b->c->write_callback)
> +               b->c->write_callback(b);
>         if (likely(b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE) &&
>             likely(b->data_mode != DATA_MODE_VMALLOC))
>                 use_inline_bio(b, rw, block, end_io);
>         else
>                 use_dmio(b, rw, block, end_io);
> @@ -550,8 +559,6 @@ static void __write_dirty_buffer(struct
>         clear_bit(B_DIRTY, &b->state);
>         wait_on_bit_lock(&b->state, B_WRITING,
>                          do_io_schedule, TASK_UNINTERRUPTIBLE);
> -       if (b->c->write_callback)
> -               b->c->write_callback(b);
>         submit_io(b, WRITE, b->block, write_endio);
>  }
> 
> 
> This doesn't seem an improvement.  Except ... it changes the behaviour
> of dm_bufio_release_move().  So was there a preexisting bug in
> dm_bufio_release_move() that you're trying to fix with this patch?

The actual reason was to do this callback in dm_bufio_release_move() too 
--- just for consistency. (the user of dm_bufio_release_move() doesn't use 
write_callback anyway).

Mikulas

Re: [PATCH] dm-bufio

[Joe Thornber]

On Mon, Oct 17, 2011 at 10:05:26AM -0400, Mikulas Patocka wrote:
> 
> 
> On Mon, 17 Oct 2011, Joe Thornber wrote:
> 
> > On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > @@ -493,8 +500,10 @@ static void use_inline_bio(struct dm_buf
> >  static void submit_io(struct dm_buffer *b, int rw, sector_t block,
> >                       bio_end_io_t *end_io)
> >  {
> > -       if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
> > -           b->data_mode != DATA_MODE_VMALLOC)
> > +       if (rw == WRITE && b->c->write_callback)
> > +               b->c->write_callback(b);
> >         if (likely(b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE) &&
> >             likely(b->data_mode != DATA_MODE_VMALLOC))
> >                 use_inline_bio(b, rw, block, end_io);
> >         else
> >                 use_dmio(b, rw, block, end_io);
> > @@ -550,8 +559,6 @@ static void __write_dirty_buffer(struct
> >         clear_bit(B_DIRTY, &b->state);
> >         wait_on_bit_lock(&b->state, B_WRITING,
> >                          do_io_schedule, TASK_UNINTERRUPTIBLE);
> > -       if (b->c->write_callback)
> > -               b->c->write_callback(b);
> >         submit_io(b, WRITE, b->block, write_endio);
> >  }
> > 
> > 
> > This doesn't seem an improvement.  Except ... it changes the behaviour
> > of dm_bufio_release_move().  So was there a preexisting bug in
> > dm_bufio_release_move() that you're trying to fix with this patch?
> 
> The actual reason was to do this callback in dm_bufio_release_move() too 
> --- just for consistency. (the user of dm_bufio_release_move() doesn't use 
> write_callback anyway).

thinp uses dm_bufio_release_move() and write_callback.  So yes, this
is a bug fix.  I thought so and merged.

- Joe

Re: [PATCH] dm-bufio

[Joe Thornber]

On Mon, Oct 17, 2011 at 10:04:16AM -0400, Mikulas Patocka wrote:
> > If you're going to use cond_resched() at least do so a little more
> > intelligently than putting it in _every_ loop.  For instance you call it on
> > every iteration of a sweep through the hash table.  The call to
> > cond_resched will take more time than the loop body.  At least make a
> > change so it's only done every n'th iteration.
> 
> I think it would be better to use
> #ifndef CONFIG_PREEMPT
> if (need_resched()) cond_resched();
> #endif
> 
> need_resched() is inlined and translates to a single condition.

Yep, that would be fine.

> I don't know why Linux doesn't provide a macro for it, this would be 
> useful far beyond dm code.

Agreed, I was very surprised that cond_resched() expands out to a
function call rather than a test + fn call.

Get a patch together and I'll merge.

- Joe

Re: [PATCH] dm-bufio

[Mikulas Patocka]

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Mon, Oct 17, 2011 at 10:05:26AM -0400, Mikulas Patocka wrote:
> > 
> > 
> > On Mon, 17 Oct 2011, Joe Thornber wrote:
> > 
> > > On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > > @@ -493,8 +500,10 @@ static void use_inline_bio(struct dm_buf
> > >  static void submit_io(struct dm_buffer *b, int rw, sector_t block,
> > >                       bio_end_io_t *end_io)
> > >  {
> > > -       if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
> > > -           b->data_mode != DATA_MODE_VMALLOC)
> > > +       if (rw == WRITE && b->c->write_callback)
> > > +               b->c->write_callback(b);
> > >         if (likely(b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE) &&
> > >             likely(b->data_mode != DATA_MODE_VMALLOC))
> > >                 use_inline_bio(b, rw, block, end_io);
> > >         else
> > >                 use_dmio(b, rw, block, end_io);
> > > @@ -550,8 +559,6 @@ static void __write_dirty_buffer(struct
> > >         clear_bit(B_DIRTY, &b->state);
> > >         wait_on_bit_lock(&b->state, B_WRITING,
> > >                          do_io_schedule, TASK_UNINTERRUPTIBLE);
> > > -       if (b->c->write_callback)
> > > -               b->c->write_callback(b);
> > >         submit_io(b, WRITE, b->block, write_endio);
> > >  }
> > > 
> > > 
> > > This doesn't seem an improvement.  Except ... it changes the behaviour
> > > of dm_bufio_release_move().  So was there a preexisting bug in
> > > dm_bufio_release_move() that you're trying to fix with this patch?
> > 
> > The actual reason was to do this callback in dm_bufio_release_move() too 
> > --- just for consistency. (the user of dm_bufio_release_move() doesn't use 
> > write_callback anyway).
> 
> thinp uses dm_bufio_release_move() and write_callback.  So yes, this
> is a bug fix.  I thought so and merged.

BTW. it still uses the old block number in "prepare_for_write" callback. 
Do you use this block number somewhere? Should we link the buffer to the 
new block before the call and the link it back?

Mikulas

> - Joe
> 

Re: [PATCH] dm-bufio

[Mikulas Patocka]

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > Hi
> > 
> > This is a patch for dm-bufio.
> 
> I've merged all I'm going to at this point and pushed to thin-dev.

I think Alasdair currently holds the master version of this code. Or is it 
you? If we had three versions of the same code and don't know which one is 
the master, then there is a problem.

Mikulas

> If you put together a patch for the cond_resched stuff I'll take that;
> providing you don't call it in a tight loop like here:
> 
> > @@ -758,8 +772,9 @@ static struct dm_buffer *__find(struct d
> >  	struct dm_buffer *b;
> >  	struct hlist_node *hn;
> >  	hlist_for_each_entry(b, hn, &c->cache_hash[DM_BUFIO_HASH(block)], hash_list) {
> > -		if (b->block == block)
> > +		if (likely(b->block == block))
> >  			return b;
> > +		dm_bufio_cond_resched();
> >  	}
> 
> 
> I'm not really interested in the likely/unlikely annotations.
> 
> - Joe
> 

Re: [PATCH] dm-bufio

[Joe Thornber]

On Mon, Oct 17, 2011 at 12:22:13PM -0400, Mikulas Patocka wrote:
> > thinp uses dm_bufio_release_move() and write_callback.  So yes, this
> > is a bug fix.  I thought so and merged.
> 
> BTW. it still uses the old block number in "prepare_for_write" callback. 
> Do you use this block number somewhere? Should we link the buffer to the 
> new block before the call and the link it back?

Good catch, let me look into it.

- Joe

Re: [PATCH] dm-bufio

[Joe Thornber]

On Mon, Oct 17, 2011 at 12:24:00PM -0400, Mikulas Patocka wrote:
> On Mon, 17 Oct 2011, Joe Thornber wrote:
> 
> > On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > > Hi
> > > 
> > > This is a patch for dm-bufio.
> > 
> > I've merged all I'm going to at this point and pushed to thin-dev.
> 
> I think Alasdair currently holds the master version of this code. Or is it 
> you? If we had three versions of the same code and don't know which one is 
> the master, then there is a problem.

I consider my tree to be the master of thin-provisioning, and will be
very annoyed if anything gets pushed to linux-next that hasn't been
sent to me first.  dm-bufio is yours to maintain in the long term -
I'm just taking an interest because it's blocking acceptance of my
code.

Re: [PATCH] dm-bufio

[Joe Thornber]

On Mon, Oct 17, 2011 at 12:22:13PM -0400, Mikulas Patocka wrote:
> > thinp uses dm_bufio_release_move() and write_callback.  So yes, this
> > is a bug fix.  I thought so and merged.
> 
> BTW. it still uses the old block number in "prepare_for_write" callback. 
> Do you use this block number somewhere? Should we link the buffer to the 
> new block before the call and the link it back?

I double checked this; it's fine.  I use release_move as part of the
shadow operation in the transaction manager.  As such we know a write
lock is going to be grabbed on it straight away (which will dirty and
prompt a rewriting of the block nr).  Also in the event of a crash
before the next commit, the new block wont appear in the metadata at
all.

- Joe