[PATCH 0/5] block: introduce helpers for allocating io buffer from slab

[PATCH 0/5] block: introduce helpers for allocating io buffer from slab

[Ming Lei]

Hi,

Filesystems may allocate io buffer from slab, and use this buffer to
submit bio. This way may break storage drivers if they have special
requirement on DMA alignment.

The patch 1 adds one warning if the io buffer isn't aligned to DMA
alignment.

The 2nd & 3rd patches make DMA alignment as stacked limit.

The 4th patch introduces helpers for allocating io buffer from slab,
and DMA alignment is respected on this allocation.

The 5th patch converts xfs to use the introduced helpers for allocating
io buffer from slab.

See previous discussion on this topic:

https://marc.info/?t=153734857500004&r=1&w=2

Thanks,
Ming

Ming Lei (5):
  block: warn on un-aligned DMA IO buffer
  block: move .dma_alignment into q->limits
  block: make dma_alignment as stacked limit
  block: introduce helpers for allocating IO buffers from slab
  xfs: use block layer helpers to allocate io buffer from slab

 block/Makefile              |   3 +-
 block/blk-core.c            |   2 +
 block/blk-merge.c           |   2 +
 block/blk-sec-buf.c         | 144 ++++++++++++++++++++++++++++++++++++++++++++
 block/blk-settings.c        |  89 +++++++++++++++------------
 fs/xfs/xfs_buf.c            |  28 ++++++++-
 fs/xfs/xfs_super.c          |  13 +++-
 include/linux/blk-sec-buf.h |  43 +++++++++++++
 include/linux/blkdev.h      |   9 ++-
 9 files changed, 287 insertions(+), 46 deletions(-)
 create mode 100644 block/blk-sec-buf.c
 create mode 100644 include/linux/blk-sec-buf.h

Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: xfs@vger.kernel.org
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Matthew Wilcox <willy@infradead.org>


-- 
2.9.5

[PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Ming Lei]

Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
for pass-through request, and it isn't done for normal IO request.

Given the check has to be done on each bvec, it isn't efficient to add the
check in generic_make_request_checks().

This patch addes one WARN in blk_queue_split() for capturing this issue.

Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: xfs@vger.kernel.org
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-merge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 42a46744c11b..d2dbd508cb6d 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -174,6 +174,8 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
 	const unsigned max_sectors = get_max_io_size(q, bio);
 
 	bio_for_each_segment(bv, bio, iter) {
+		WARN_ON_ONCE(queue_dma_alignment(q) & bv.bv_offset);
+
 		/*
 		 * If the queue doesn't support SG gaps and adding this
 		 * offset would create a gap, disallow it.
-- 
2.9.5

[PATCH 2/5] block: move .dma_alignment into q->limits

[Ming Lei]

Turns out q->dma_alignement should be stack limit because now bvec table
is immutalbe, the underlying queue's dma alignment has to be perceptible
by stack driver, so IO buffer can be allocated as dma aligned before
adding to bio.

So this patch moves .dma_alignment into q->limits and prepares for
making it as one stacked limit.

Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: xfs@vger.kernel.org
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-settings.c   | 6 +++---
 include/linux/blkdev.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index ffd459969689..cf9cd241dc16 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -830,7 +830,7 @@ EXPORT_SYMBOL(blk_queue_virt_boundary);
  **/
 void blk_queue_dma_alignment(struct request_queue *q, int mask)
 {
-	q->dma_alignment = mask;
+	q->limits.dma_alignment = mask;
 }
 EXPORT_SYMBOL(blk_queue_dma_alignment);
 
@@ -852,8 +852,8 @@ void blk_queue_update_dma_alignment(struct request_queue *q, int mask)
 {
 	BUG_ON(mask > PAGE_SIZE);
 
-	if (mask > q->dma_alignment)
-		q->dma_alignment = mask;
+	if (mask > q->limits.dma_alignment)
+		q->limits.dma_alignment = mask;
 }
 EXPORT_SYMBOL(blk_queue_update_dma_alignment);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 61207560e826..be938a31bc2e 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -366,6 +366,7 @@ struct queue_limits {
 	unsigned long		seg_boundary_mask;
 	unsigned long		virt_boundary_mask;
 
+	unsigned int		dma_alignment;
 	unsigned int		max_hw_sectors;
 	unsigned int		max_dev_sectors;
 	unsigned int		chunk_sectors;
@@ -561,7 +562,6 @@ struct request_queue {
 	unsigned int		dma_drain_size;
 	void			*dma_drain_buffer;
 	unsigned int		dma_pad_mask;
-	unsigned int		dma_alignment;
 
 	struct blk_queue_tag	*queue_tags;
 
@@ -1617,7 +1617,7 @@ static inline unsigned int bdev_zone_sectors(struct block_device *bdev)
 
 static inline int queue_dma_alignment(struct request_queue *q)
 {
-	return q ? q->dma_alignment : 511;
+	return q ? q->limits.dma_alignment : 511;
 }
 
 static inline int blk_rq_aligned(struct request_queue *q, unsigned long addr,
-- 
2.9.5

[PATCH 3/5] block: make dma_alignment as stacked limit

[Ming Lei]

This patch converts .dma_alignment into stacked limit, so the stack
driver may get updated with underlying dma alignment, and allocate
IO buffer as queue DMA aligned.

Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: xfs@vger.kernel.org
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-settings.c | 89 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 39 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index cf9cd241dc16..aef4510a99b6 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -525,6 +525,54 @@ void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b)
 EXPORT_SYMBOL(blk_queue_stack_limits);
 
 /**
+ * blk_queue_dma_alignment - set dma length and memory alignment
+ * @q:     the request queue for the device
+ * @mask:  alignment mask
+ *
+ * description:
+ *    set required memory and length alignment for direct dma transactions.
+ *    this is used when building direct io requests for the queue.
+ *
+ **/
+void blk_queue_dma_alignment(struct request_queue *q, int mask)
+{
+	q->limits.dma_alignment = mask;
+}
+EXPORT_SYMBOL(blk_queue_dma_alignment);
+
+static int __blk_queue_update_dma_alignment(struct queue_limits *t, int mask)
+{
+	BUG_ON(mask >= PAGE_SIZE);
+
+	if (mask > t->dma_alignment)
+		return mask;
+	else
+		return t->dma_alignment;
+}
+
+/**
+ * blk_queue_update_dma_alignment - update dma length and memory alignment
+ * @q:     the request queue for the device
+ * @mask:  alignment mask
+ *
+ * description:
+ *    update required memory and length alignment for direct dma transactions.
+ *    If the requested alignment is larger than the current alignment, then
+ *    the current queue alignment is updated to the new value, otherwise it
+ *    is left alone.  The design of this is to allow multiple objects
+ *    (driver, device, transport etc) to set their respective
+ *    alignments without having them interfere.
+ *
+ **/
+void blk_queue_update_dma_alignment(struct request_queue *q, int mask)
+{
+	q->limits.dma_alignment =
+		__blk_queue_update_dma_alignment(&q->limits, mask);
+}
+EXPORT_SYMBOL(blk_queue_update_dma_alignment);
+
+
+/**
  * blk_stack_limits - adjust queue_limits for stacked devices
  * @t:	the stacking driver limits (top device)
  * @b:  the underlying queue limits (bottom, component device)
@@ -563,6 +611,8 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
 					    b->seg_boundary_mask);
 	t->virt_boundary_mask = min_not_zero(t->virt_boundary_mask,
 					    b->virt_boundary_mask);
+	t->dma_alignment = __blk_queue_update_dma_alignment(t,
+							    b->dma_alignment);
 
 	t->max_segments = min_not_zero(t->max_segments, b->max_segments);
 	t->max_discard_segments = min_not_zero(t->max_discard_segments,
@@ -818,45 +868,6 @@ void blk_queue_virt_boundary(struct request_queue *q, unsigned long mask)
 }
 EXPORT_SYMBOL(blk_queue_virt_boundary);
 
-/**
- * blk_queue_dma_alignment - set dma length and memory alignment
- * @q:     the request queue for the device
- * @mask:  alignment mask
- *
- * description:
- *    set required memory and length alignment for direct dma transactions.
- *    this is used when building direct io requests for the queue.
- *
- **/
-void blk_queue_dma_alignment(struct request_queue *q, int mask)
-{
-	q->limits.dma_alignment = mask;
-}
-EXPORT_SYMBOL(blk_queue_dma_alignment);
-
-/**
- * blk_queue_update_dma_alignment - update dma length and memory alignment
- * @q:     the request queue for the device
- * @mask:  alignment mask
- *
- * description:
- *    update required memory and length alignment for direct dma transactions.
- *    If the requested alignment is larger than the current alignment, then
- *    the current queue alignment is updated to the new value, otherwise it
- *    is left alone.  The design of this is to allow multiple objects
- *    (driver, device, transport etc) to set their respective
- *    alignments without having them interfere.
- *
- **/
-void blk_queue_update_dma_alignment(struct request_queue *q, int mask)
-{
-	BUG_ON(mask > PAGE_SIZE);
-
-	if (mask > q->limits.dma_alignment)
-		q->limits.dma_alignment = mask;
-}
-EXPORT_SYMBOL(blk_queue_update_dma_alignment);
-
 void blk_queue_flush_queueable(struct request_queue *q, bool queueable)
 {
 	if (queueable)
-- 
2.9.5

[PATCH 4/5] block: introduce helpers for allocating IO buffers from slab

[Ming Lei]

One big issue is that the allocated buffer from slab has to respect
the queue DMA alignment limit.

This patch supports to create one kmem_cache for less-than PAGE_SIZE
allocation, and makes sure that the allocation is aligned with queue
DMA alignment.

For >= PAGE_SIZE allocation, it should be done via buddy directly.

Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: xfs@vger.kernel.org
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/Makefile              |   3 +-
 block/blk-core.c            |   2 +
 block/blk-sec-buf.c         | 144 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blk-sec-buf.h |  43 +++++++++++++
 include/linux/blkdev.h      |   5 ++
 5 files changed, 196 insertions(+), 1 deletion(-)
 create mode 100644 block/blk-sec-buf.c
 create mode 100644 include/linux/blk-sec-buf.h

diff --git a/block/Makefile b/block/Makefile
index 27eac600474f..74f3ed6ef954 100644
--- a/block/Makefile
+++ b/block/Makefile
@@ -9,7 +9,8 @@ obj-$(CONFIG_BLOCK) := bio.o elevator.o blk-core.o blk-tag.o blk-sysfs.o \
 			blk-lib.o blk-mq.o blk-mq-tag.o blk-stat.o \
 			blk-mq-sysfs.o blk-mq-cpumap.o blk-mq-sched.o ioctl.o \
 			genhd.o partition-generic.o ioprio.o \
-			badblocks.o partitions/ blk-rq-qos.o
+			badblocks.o partitions/ blk-rq-qos.o \
+			blk-sec-buf.o
 
 obj-$(CONFIG_BOUNCE)		+= bounce.o
 obj-$(CONFIG_BLK_SCSI_REQUEST)	+= scsi_ioctl.o
diff --git a/block/blk-core.c b/block/blk-core.c
index cdfabc5646da..02fe17dd5e67 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1079,6 +1079,8 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id,
 	if (blkcg_init_queue(q))
 		goto fail_ref;
 
+	mutex_init(&q->blk_sec_buf_slabs_mutex);
+
 	return q;
 
 fail_ref:
diff --git a/block/blk-sec-buf.c b/block/blk-sec-buf.c
new file mode 100644
index 000000000000..2842a913a3d1
--- /dev/null
+++ b/block/blk-sec-buf.c
@@ -0,0 +1,144 @@
+/*
+ * Sector size level IO buffer allocation helpers for less-than PAGE_SIZE
+ * allocation.
+ *
+ * Controllers may has DMA alignment requirement, meantime filesystem or
+ * other upper layer component may allocate IO buffer via slab and submit
+ * bio with this buffer directly. Then DMA alignment limit can't be
+ * repectected.
+ *
+ * Create DMA aligned slab, and allocate this less-than PAGE_SIZE IO buffer
+ * from the created slab for above users.
+ *
+ * Copyright (C) 2018 Ming Lei <ming.lei@redhat.com>
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/blk-sec-buf.h>
+
+static void __blk_destroy_sec_buf_slabs(struct blk_sec_buf_slabs *slabs)
+{
+	int i;
+
+	if (!slabs)
+		return;
+
+	for (i = 0; i < BLK_NR_SEC_BUF_SLAB; i++)
+		kmem_cache_destroy(slabs->slabs[i]);
+	kfree(slabs);
+}
+
+void blk_destroy_sec_buf_slabs(struct request_queue *q)
+{
+	mutex_lock(&q->blk_sec_buf_slabs_mutex);
+	if (q->sec_buf_slabs && !--q->sec_buf_slabs->ref_cnt) {
+		__blk_destroy_sec_buf_slabs(q->sec_buf_slabs);
+		q->sec_buf_slabs = NULL;
+	}
+	mutex_unlock(&q->blk_sec_buf_slabs_mutex);
+}
+EXPORT_SYMBOL_GPL(blk_destroy_sec_buf_slabs);
+
+int blk_create_sec_buf_slabs(char *name, struct request_queue *q)
+{
+	struct blk_sec_buf_slabs *slabs;
+	char *slab_name;
+	int i;
+	int nr_slabs = BLK_NR_SEC_BUF_SLAB;
+	int ret = -ENOMEM;
+
+	/* No need to create kmem_cache if kmalloc is fine */
+	if (!q || queue_dma_alignment(q) < ARCH_KMALLOC_MINALIGN)
+		return 0;
+
+	slab_name = kmalloc(strlen(name) + 5, GFP_KERNEL);
+	if (!slab_name)
+		return ret;
+
+	mutex_lock(&q->blk_sec_buf_slabs_mutex);
+	if (q->sec_buf_slabs) {
+		q->sec_buf_slabs->ref_cnt++;
+		ret = 0;
+		goto out;
+	}
+
+	slabs = kzalloc(sizeof(*slabs), GFP_KERNEL);
+	if (!slabs)
+		goto out;
+
+	for (i = 0; i < nr_slabs; i++) {
+		int size = (i == nr_slabs - 1) ? PAGE_SIZE - 512 : (i + 1) << 9;
+
+		sprintf(slab_name, "%s-%d", name, i);
+		slabs->slabs[i] = kmem_cache_create(slab_name, size,
+				queue_dma_alignment(q) + 1,
+				SLAB_PANIC, NULL);
+		if (!slabs->slabs[i])
+			goto fail;
+	}
+
+	slabs->ref_cnt = 1;
+	q->sec_buf_slabs = slabs;
+	ret = 0;
+	goto out;
+
+ fail:
+	__blk_destroy_sec_buf_slabs(slabs);
+ out:
+	mutex_unlock(&q->blk_sec_buf_slabs_mutex);
+	kfree(slab_name);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(blk_create_sec_buf_slabs);
+
+void *blk_alloc_sec_buf(struct request_queue *q, int size, gfp_t flags)
+{
+	int i;
+
+	/* We only serve less-than PAGE_SIZE alloction */
+	if (size >= PAGE_SIZE)
+		return NULL;
+
+	/*
+	 * Fallback to kmalloc if no queue is provided, or kmalloc is
+	 * enough to respect the queue dma alignment
+	 */
+	if (!q || queue_dma_alignment(q) < ARCH_KMALLOC_MINALIGN)
+		return kmalloc(size, flags);
+
+	if (WARN_ON_ONCE(!q->sec_buf_slabs))
+		return NULL;
+
+	i = round_up(size, 512) >> 9;
+	i = i < BLK_NR_SEC_BUF_SLAB ? i : BLK_NR_SEC_BUF_SLAB;
+
+	return kmem_cache_alloc(q->sec_buf_slabs->slabs[i - 1], flags);
+}
+EXPORT_SYMBOL_GPL(blk_alloc_sec_buf);
+
+void blk_free_sec_buf(struct request_queue *q, void *buf, int size)
+{
+	int i;
+
+	/* We only serve less-than PAGE_SIZE alloction */
+	if (size >= PAGE_SIZE)
+		return;
+
+	/*
+	 * Fallback to kmalloc if no queue is provided, or kmalloc is
+	 * enough to respect the queue dma alignment
+	 */
+	if (!q || queue_dma_alignment(q) < ARCH_KMALLOC_MINALIGN) {
+		kfree(buf);
+		return;
+	}
+
+	if (WARN_ON_ONCE(!q->sec_buf_slabs))
+		return;
+
+	i = round_up(size, 512) >> 9;
+	i = i < BLK_NR_SEC_BUF_SLAB ? i : BLK_NR_SEC_BUF_SLAB;
+
+	kmem_cache_free(q->sec_buf_slabs->slabs[i - 1], buf);
+}
+EXPORT_SYMBOL_GPL(blk_free_sec_buf);
diff --git a/include/linux/blk-sec-buf.h b/include/linux/blk-sec-buf.h
new file mode 100644
index 000000000000..dc81d8fc0d68
--- /dev/null
+++ b/include/linux/blk-sec-buf.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_BLK_SEC_BUF_H
+#define _LINUX_BLK_SEC_BUF_H
+
+#include <linux/slab.h>
+#include <linux/blkdev.h>
+
+#define  BLK_NR_SEC_BUF_SLAB   ((PAGE_SIZE >> 9) > 128 ? 128 : (PAGE_SIZE >> 9))
+struct blk_sec_buf_slabs {
+	int ref_cnt;
+	struct kmem_cache *slabs[BLK_NR_SEC_BUF_SLAB];
+};
+
+int blk_create_sec_buf_slabs(char *name, struct request_queue *q);
+void blk_destroy_sec_buf_slabs(struct request_queue *q);
+
+void *blk_alloc_sec_buf(struct request_queue *q, int size, gfp_t flags);
+void blk_free_sec_buf(struct request_queue *q, void *buf, int size);
+
+static inline int bdev_create_sec_buf_slabs(struct block_device *bdev)
+{
+	char *name = bdev->bd_disk ? bdev->bd_disk->disk_name : "unknown";
+
+	return blk_create_sec_buf_slabs(name, bdev->bd_queue);
+}
+
+static inline void bdev_destroy_sec_buf_slabs(struct block_device *bdev)
+{
+	blk_destroy_sec_buf_slabs(bdev->bd_queue);
+}
+
+static inline void *bdev_alloc_sec_buf(struct block_device *bdev, int size,
+		gfp_t flags)
+{
+	return blk_alloc_sec_buf(bdev->bd_queue, size, flags);
+}
+static inline void bdev_free_sec_buf(struct block_device *bdev, void *buf,
+		int size)
+{
+	blk_free_sec_buf(bdev->bd_queue, buf, size);
+}
+
+#endif
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index be938a31bc2e..30f5324d1f95 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -27,6 +27,7 @@
 #include <linux/percpu-refcount.h>
 #include <linux/scatterlist.h>
 #include <linux/blkzoned.h>
+#include <linux/blk-sec-buf.h>
 
 struct module;
 struct scsi_ioctl_command;
@@ -523,6 +524,10 @@ struct request_queue {
 	 */
 	gfp_t			bounce_gfp;
 
+	/* for allocate less-than PAGE_SIZE io buffer */
+	struct blk_sec_buf_slabs *sec_buf_slabs;
+	struct mutex		blk_sec_buf_slabs_mutex;
+
 	/*
 	 * protects queue structures from reentrancy. ->__queue_lock should
 	 * _never_ be used directly, it is queue private. always use
-- 
2.9.5

[PATCH 5/5] xfs: use block layer helpers to allocate io buffer from slab

[Ming Lei]

XFS may use kmalloc() to allocate io buffer, this way may not respect
request queue's DMA alignment limit, and cause data corruption.

This patch uses the introduced block layer helpers to allocate this
kind of io buffer, and makes sure that DMA alignment is respected.

Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>
Cc: Darrick J. Wong <darrick.wong@oracle.com>
Cc: xfs@vger.kernel.org
Cc: Dave Chinner <dchinner@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 fs/xfs/xfs_buf.c   | 28 +++++++++++++++++++++++++---
 fs/xfs/xfs_super.c | 13 ++++++++++++-
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index e839907e8492..fabee5e1706b 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -314,12 +314,34 @@ xfs_buf_free(
 			__free_page(page);
 		}
 	} else if (bp->b_flags & _XBF_KMEM)
-		kmem_free(bp->b_addr);
+		bdev_free_sec_buf(bp->b_target->bt_bdev, bp->b_addr,
+				BBTOB(bp->b_length));
 	_xfs_buf_free_pages(bp);
 	xfs_buf_free_maps(bp);
 	kmem_zone_free(xfs_buf_zone, bp);
 }
 
+void *
+xfs_buf_allocate_memory_from_slab(xfs_buf_t *bp, int size)
+{
+	int	retries = 0;
+	gfp_t	lflags = kmem_flags_convert(KM_NOFS);
+	void	*ptr;
+
+	do {
+		ptr = bdev_alloc_sec_buf(bp->b_target->bt_bdev, size, lflags);
+		if (ptr)
+			return ptr;
+		if (!(++retries % 100))
+			xfs_err(NULL,
+	"%s(%u) possible memory allocation deadlock size %u in %s (mode:0x%x)",
+				current->comm, current->pid,
+				(unsigned int)size, __func__, lflags);
+		congestion_wait(BLK_RW_ASYNC, HZ/50);
+	} while (1);
+
+}
+
 /*
  * Allocates all the pages for buffer in question and builds it's page list.
  */
@@ -342,7 +364,7 @@ xfs_buf_allocate_memory(
 	 */
 	size = BBTOB(bp->b_length);
 	if (size < PAGE_SIZE) {
-		bp->b_addr = kmem_alloc(size, KM_NOFS);
+		bp->b_addr = xfs_buf_allocate_memory_from_slab(bp, size);
 		if (!bp->b_addr) {
 			/* low memory - use alloc_page loop instead */
 			goto use_alloc_page;
@@ -351,7 +373,7 @@ xfs_buf_allocate_memory(
 		if (((unsigned long)(bp->b_addr + size - 1) & PAGE_MASK) !=
 		    ((unsigned long)bp->b_addr & PAGE_MASK)) {
 			/* b_addr spans two pages - use alloc_page instead */
-			kmem_free(bp->b_addr);
+			bdev_free_sec_buf(bp->b_target->bt_bdev, bp->b_addr, size);
 			bp->b_addr = NULL;
 			goto use_alloc_page;
 		}
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 207ee302b1bb..026cdae3aa4f 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -664,6 +664,10 @@ xfs_blkdev_get(
 		xfs_warn(mp, "Invalid device [%s], error=%d", name, error);
 	}
 
+	error = bdev_create_sec_buf_slabs(*bdevp);
+	if (error)
+		blkdev_put(*bdevp, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
+
 	return error;
 }
 
@@ -671,8 +675,10 @@ STATIC void
 xfs_blkdev_put(
 	struct block_device	*bdev)
 {
-	if (bdev)
+	if (bdev) {
+		bdev_destroy_sec_buf_slabs(bdev);
 		blkdev_put(bdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
+	}
 }
 
 void
@@ -706,6 +712,8 @@ xfs_close_devices(
 	}
 	xfs_free_buftarg(mp->m_ddev_targp);
 	fs_put_dax(dax_ddev);
+
+	bdev_destroy_sec_buf_slabs(mp->m_super->s_bdev);
 }
 
 /*
@@ -774,6 +782,9 @@ xfs_open_devices(
 		mp->m_logdev_targp = mp->m_ddev_targp;
 	}
 
+	if (bdev_create_sec_buf_slabs(ddev))
+		goto out_free_rtdev_targ;
+
 	return 0;
 
  out_free_rtdev_targ:
-- 
2.9.5

Re: [PATCH 0/5] block: introduce helpers for allocating io buffer from slab

[Matthew Wilcox]

On Thu, Oct 18, 2018 at 09:18:12PM +0800, Ming Lei wrote:
> Hi,
> 
> Filesystems may allocate io buffer from slab, and use this buffer to
> submit bio. This way may break storage drivers if they have special
> requirement on DMA alignment.

Before we go down this road, could we have a discussion about what
hardware actually requires this?  Storage has this weird assumption that
I/Os must be at least 512 byte aligned in memory, and I don't know where
this idea comes from.  Network devices can do arbitrary byte alignment.
Even USB controllers can do arbitrary byte alignment.  Sure, performance
is going to suck and there are definite risks on some architectures
with doing IOs that are sub-cacheline aligned, but why is storage such a
special snowflake that we assume that host controllers are only capable
of doing 512-byte aligned DMAs?

I just dragged out the NCR53c810 data sheet from 1993, and it's capable of
doing arbitrary alignment of DMA.  NVMe is capable of 4-byte aligned DMA.
What hardware needs this 512 byte alignment?

Re: [PATCH 0/5] block: introduce helpers for allocating io buffer from slab

[Christoph Hellwig]

On Thu, Oct 18, 2018 at 07:03:42AM -0700, Matthew Wilcox wrote:
> Before we go down this road, could we have a discussion about what
> hardware actually requires this?  Storage has this weird assumption that
> I/Os must be at least 512 byte aligned in memory, and I don't know where
> this idea comes from.  Network devices can do arbitrary byte alignment.
> Even USB controllers can do arbitrary byte alignment.  Sure, performance
> is going to suck and there are definite risks on some architectures
> with doing IOs that are sub-cacheline aligned, but why is storage such a
> special snowflake that we assume that host controllers are only capable
> of doing 512-byte aligned DMAs?

Actually most storage controllers requires 4-byte alignment, but there is
a significant subset that requires 512-byte alignment.

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Jens Axboe]

On 10/18/18 7:18 AM, Ming Lei wrote:
> Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
> for pass-through request, and it isn't done for normal IO request.
> 
> Given the check has to be done on each bvec, it isn't efficient to add the
> check in generic_make_request_checks().
> 
> This patch addes one WARN in blk_queue_split() for capturing this issue.

I don't want to do this, because then we are forever doomed to
have something that fully loops a bio at submission time. I
absolutely hate the splitting we have and the need for it,
hopefully it can go away for a subset of IOs at some point.

In many ways, this seems to be somewhat of a made-up problem, I don't
recall a single bug report for something like this over decades of
working with the IO stack. 512b alignment restrictions for DMA seems
absolutely insane. I know people claim they exist, but clearly that
isn't a hard requirement or we would have been boned years ago.

-- 
Jens Axboe

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Christoph Hellwig]

> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 42a46744c11b..d2dbd508cb6d 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -174,6 +174,8 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,
>  	const unsigned max_sectors = get_max_io_size(q, bio);
>  
>  	bio_for_each_segment(bv, bio, iter) {
> +		WARN_ON_ONCE(queue_dma_alignment(q) & bv.bv_offset);

I'd write this the other way around, although I have no good argument
for that.

Otherwise this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/5] block: move .dma_alignment into q->limits

[Christoph Hellwig]

On Thu, Oct 18, 2018 at 09:18:14PM +0800, Ming Lei wrote:
> Turns out q->dma_alignement should be stack limit because now bvec table
> is immutalbe, the underlying queue's dma alignment has to be perceptible
> by stack driver, so IO buffer can be allocated as dma aligned before
> adding to bio.
> 
> So this patch moves .dma_alignment into q->limits and prepares for
> making it as one stacked limit.

Looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/5] block: make dma_alignment as stacked limit

[Christoph Hellwig]

On Thu, Oct 18, 2018 at 09:18:15PM +0800, Ming Lei wrote:
> This patch converts .dma_alignment into stacked limit, so the stack
> driver may get updated with underlying dma alignment, and allocate
> IO buffer as queue DMA aligned.
> 
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>
> Cc: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: xfs@vger.kernel.org
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-settings.c | 89 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index cf9cd241dc16..aef4510a99b6 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -525,6 +525,54 @@ void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b)
>  EXPORT_SYMBOL(blk_queue_stack_limits);
>  
>  /**
> + * blk_queue_dma_alignment - set dma length and memory alignment
> + * @q:     the request queue for the device
> + * @mask:  alignment mask
> + *
> + * description:
> + *    set required memory and length alignment for direct dma transactions.
> + *    this is used when building direct io requests for the queue.
> + *
> + **/
> +void blk_queue_dma_alignment(struct request_queue *q, int mask)
> +{
> +	q->limits.dma_alignment = mask;
> +}
> +EXPORT_SYMBOL(blk_queue_dma_alignment);

Any good reason not to keep these functions where they were before?

> +
> +static int __blk_queue_update_dma_alignment(struct queue_limits *t, int mask)
> +{
> +	BUG_ON(mask >= PAGE_SIZE);
> +
> +	if (mask > t->dma_alignment)
> +		return mask;
> +	else
> +		return t->dma_alignment;

I think this function could just be replaced with a:

	max(t->dma_alignment, mask);

Re: [PATCH 4/5] block: introduce helpers for allocating IO buffers from slab

[Christoph Hellwig]

This all seems quite complicated.

I think the interface we'd want is more one that has a little
cache of a single page in the queue, and a little bitmap which
sub-page size blocks of it are used.

Something like (pseudo code minus locking):

void *blk_alloc_sector_buffer(struct block_device *bdev, gfp_t gfp)
{
	unsigned block_size = block_size(bdev);

	if (blocksize >= PAGE_SIZE)
		return (void *)__get_free_pages(gfp, get_order(blocksize));

	if (bdev->fragment_cache_page) {
		[ <find fragment in bdev->fragment_cache_page using
		  e.g. bitmap and return if found]
	}

	bdev->fragment_cache_page = (void *)__get_free_page(gfp);
	goto find_again;
}

note that the locking also is important, preferably we'd be able to do
something lockless.

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Christoph Hellwig]

On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote:
> On 10/18/18 7:18 AM, Ming Lei wrote:
> > Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
> > for pass-through request, and it isn't done for normal IO request.
> > 
> > Given the check has to be done on each bvec, it isn't efficient to add the
> > check in generic_make_request_checks().
> > 
> > This patch addes one WARN in blk_queue_split() for capturing this issue.
> 
> I don't want to do this, because then we are forever doomed to
> have something that fully loops a bio at submission time. I
> absolutely hate the splitting we have and the need for it,
> hopefully it can go away for a subset of IOs at some point.

It is just a WARN_ON - no one should rely on it, but it is a good
debug aid.

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Jens Axboe]

On 10/18/18 8:43 AM, Christoph Hellwig wrote:
> On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote:
>> On 10/18/18 7:18 AM, Ming Lei wrote:
>>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
>>> for pass-through request, and it isn't done for normal IO request.
>>>
>>> Given the check has to be done on each bvec, it isn't efficient to add the
>>> check in generic_make_request_checks().
>>>
>>> This patch addes one WARN in blk_queue_split() for capturing this issue.
>>
>> I don't want to do this, because then we are forever doomed to
>> have something that fully loops a bio at submission time. I
>> absolutely hate the splitting we have and the need for it,
>> hopefully it can go away for a subset of IOs at some point.
> 
> It is just a WARN_ON - no one should rely on it, but it is a good
> debug aid.

And then we'll have it start triggering on random things because
some drivers set random limits, that don't reflect reality at
all... We've basically had this setting that some drivers set,
but that we don't really look at except for mapping in user
data. Those should be audited first before adding something like
this.

-- 
Jens Axboe

Re: [PATCH 0/5] block: introduce helpers for allocating io buffer from slab

[Matthew Wilcox]

On Thu, Oct 18, 2018 at 04:05:51PM +0200, Christoph Hellwig wrote:
> On Thu, Oct 18, 2018 at 07:03:42AM -0700, Matthew Wilcox wrote:
> > Before we go down this road, could we have a discussion about what
> > hardware actually requires this?  Storage has this weird assumption that
> > I/Os must be at least 512 byte aligned in memory, and I don't know where
> > this idea comes from.  Network devices can do arbitrary byte alignment.
> > Even USB controllers can do arbitrary byte alignment.  Sure, performance
> > is going to suck and there are definite risks on some architectures
> > with doing IOs that are sub-cacheline aligned, but why is storage such a
> > special snowflake that we assume that host controllers are only capable
> > of doing 512-byte aligned DMAs?
> 
> Actually most storage controllers requires 4-byte alignment, but there is
> a significant subset that requires 512-byte alignment.

Can you name one that does require 512-byte alignment, preferably still
in use?  Or even >4-byte alignment.  I just checked AHCI and that requires
only 2-byte alignment.

I have reason to believe that these are uncommon because of the feedback
we got in the NVMe committee after releasing 1.0 which required 4-byte
alignment from people whining that they just couldn't guarantee 4-byte
alignment in their host devices and they absolutely needed to have no
alignment requirements (!)

Re: [PATCH 4/5] block: introduce helpers for allocating IO buffers from slab

[Matthew Wilcox]

On Thu, Oct 18, 2018 at 04:42:07PM +0200, Christoph Hellwig wrote:
> This all seems quite complicated.
> 
> I think the interface we'd want is more one that has a little
> cache of a single page in the queue, and a little bitmap which
> sub-page size blocks of it are used.
> 
> Something like (pseudo code minus locking):
> 
> void *blk_alloc_sector_buffer(struct block_device *bdev, gfp_t gfp)
> {
> 	unsigned block_size = block_size(bdev);
> 
> 	if (blocksize >= PAGE_SIZE)
> 		return (void *)__get_free_pages(gfp, get_order(blocksize));
> 
> 	if (bdev->fragment_cache_page) {
> 		[ <find fragment in bdev->fragment_cache_page using
> 		  e.g. bitmap and return if found]
> 	}
> 
> 	bdev->fragment_cache_page = (void *)__get_free_page(gfp);
> 	goto find_again;
> }

This looks a lot like page_frag_alloc() except I think page_frag_alloc()
may be more efficient.

Re: [PATCH 0/5] block: introduce helpers for allocating io buffer from slab

[Christoph Hellwig]

On Thu, Oct 18, 2018 at 08:06:05AM -0700, Matthew Wilcox wrote:
> Can you name one that does require 512-byte alignment, preferably still
> in use?  Or even >4-byte alignment.  I just checked AHCI and that requires
> only 2-byte alignment.

Xen-blkfront, rsxx, various SD/MMC card readers for example.

> I have reason to believe that these are uncommon because of the feedback
> we got in the NVMe committee after releasing 1.0 which required 4-byte
> alignment from people whining that they just couldn't guarantee 4-byte
> alignment in their host devices and they absolutely needed to have no
> alignment requirements (!)

See how things turned - after NVMe SGLs followed the no alignment
rule enough controller vendors rebelled so that NVMe 1.3 has an option
of SGL support only if 4-byte aligned.

Re: [PATCH 4/5] block: introduce helpers for allocating IO buffers from slab

[Christoph Hellwig]

On Thu, Oct 18, 2018 at 08:11:23AM -0700, Matthew Wilcox wrote:
> On Thu, Oct 18, 2018 at 04:42:07PM +0200, Christoph Hellwig wrote:
> > This all seems quite complicated.
> > 
> > I think the interface we'd want is more one that has a little
> > cache of a single page in the queue, and a little bitmap which
> > sub-page size blocks of it are used.
> > 
> > Something like (pseudo code minus locking):
> > 
> > void *blk_alloc_sector_buffer(struct block_device *bdev, gfp_t gfp)
> > {
> > 	unsigned block_size = block_size(bdev);
> > 
> > 	if (blocksize >= PAGE_SIZE)
> > 		return (void *)__get_free_pages(gfp, get_order(blocksize));
> > 
> > 	if (bdev->fragment_cache_page) {
> > 		[ <find fragment in bdev->fragment_cache_page using
> > 		  e.g. bitmap and return if found]
> > 	}
> > 
> > 	bdev->fragment_cache_page = (void *)__get_free_page(gfp);
> > 	goto find_again;
> > }
> 
> This looks a lot like page_frag_alloc() except I think page_frag_alloc()
> may be more efficient.

Oh, nice. Sounds like XFS should just use page_frag_alloc.  I'll give
it a spin.

Re: [PATCH 0/5] block: introduce helpers for allocating io buffer from slab

[Bart Van Assche]

On Thu, 2018-10-18 at 07:03 -0700, Matthew Wilcox wrote:
> On Thu, Oct 18, 2018 at 09:18:12PM +0800, Ming Lei wrote:
> > Filesystems may allocate io buffer from slab, and use this buffer to
> > submit bio. This way may break storage drivers if they have special
> > requirement on DMA alignment.
> 
> Before we go down this road, could we have a discussion about what
> hardware actually requires this?  Storage has this weird assumption that
> I/Os must be at least 512 byte aligned in memory, and I don't know where
> this idea comes from.  Network devices can do arbitrary byte alignment.
> Even USB controllers can do arbitrary byte alignment.  Sure, performance
> is going to suck and there are definite risks on some architectures
> with doing IOs that are sub-cacheline aligned, but why is storage such a
> special snowflake that we assume that host controllers are only capable
> of doing 512-byte aligned DMAs?
> 
> I just dragged out the NCR53c810 data sheet from 1993, and it's capable of
> doing arbitrary alignment of DMA.  NVMe is capable of 4-byte aligned DMA.
> What hardware needs this 512 byte alignment?

How about starting with modifying the queue_dma_alignment() function? The
current implementation of that function is as follows:

static inline int queue_dma_alignment(struct request_queue *q)
{
	return q ? q->dma_alignment : 511;
}

In other words, for block drivers that do not set the DMA alignment
explicitly it is assumed that these drivers need 512 byte alignment. I think
the "512 byte alignment as default" was introduced in 2002. From Thomas
Gleixner's history tree, commit ad519c6902fb:

+static inline int queue_dma_alignment(request_queue_t *q)
+{
+       int retval = 511;
+
+       if (q && q->dma_alignment)
+               retval = q->dma_alignment;
+
+       return retval;
+}
+
+static inline int bdev_dma_aligment(struct block_device *bdev)
+{
+       return queue_dma_alignment(bdev_get_queue(bdev));
+}
+

Bart.

Re: [PATCH 0/5] block: introduce helpers for allocating io buffer from slab

[Bart Van Assche]

On Thu, 2018-10-18 at 07:03 -0700, Matthew Wilcox wrote:
> On Thu, Oct 18, 2018 at 09:18:12PM +0800, Ming Lei wrote:
> > Filesystems may allocate io buffer from slab, and use this buffer to
> > submit bio. This way may break storage drivers if they have special
> > requirement on DMA alignment.
> 
> Before we go down this road, could we have a discussion about what
> hardware actually requires this?  Storage has this weird assumption that
> I/Os must be at least 512 byte aligned in memory, and I don't know where
> this idea comes from.  Network devices can do arbitrary byte alignment.
> Even USB controllers can do arbitrary byte alignment.  Sure, performance
> is going to suck and there are definite risks on some architectures
> with doing IOs that are sub-cacheline aligned, but why is storage such a
> special snowflake that we assume that host controllers are only capable
> of doing 512-byte aligned DMAs?
> 
> I just dragged out the NCR53c810 data sheet from 1993, and it's capable of
> doing arbitrary alignment of DMA.  NVMe is capable of 4-byte aligned DMA.
> What hardware needs this 512 byte alignment?

How about starting with modifying the queue_dma_alignment() function? The
current implementation of that function is as follows:

static inline int queue_dma_alignment(struct request_queue *q)
{
	return q ? q->dma_alignment : 511;
}

In other words, for block drivers that do not set the DMA alignment
explicitly it is assumed that these drivers need 512 byte alignment. I think
the "512 byte alignment as default" was introduced in 2002. From Thomas
Gleixner's history tree, commit ad519c6902fb:

+static inline int queue_dma_alignment(request_queue_t *q)
+{
+       int retval = 511;
+
+       if (q && q->dma_alignment)
+               retval = q->dma_alignment;
+
+       return retval;
+}
+
+static inline int bdev_dma_aligment(struct block_device *bdev)
+{
+       return queue_dma_alignment(bdev_get_queue(bdev));
+}
+

Bart.

Re: [PATCH 2/5] block: move .dma_alignment into q->limits

[Bart Van Assche]

On Thu, 2018-10-18 at 21:18 +0800, Ming Lei wrote:
> Turns out q->dma_alignement should be stack limit because now bvec table
               ^^^^^^^^^^^^^^
               dma_alignment?
> is immutalbe, the underlying queue's dma alignment has to be perceptible
     ^^^^^^^^^                                                 ^^^^^^^^^^^
     immutable?                                                observable?
> by stack driver, so IO buffer can be allocated as dma aligned before
     ^^^^^
     stacked?

Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 2/5] block: move .dma_alignment into q->limits

[Bart Van Assche]

On Thu, 2018-10-18 at 21:18 +0800, Ming Lei wrote:
> Turns out q->dma_alignement should be stack limit because now bvec table
               ^^^^^^^^^^^^^^
               dma_alignment?
> is immutalbe, the underlying queue's dma alignment has to be perceptible
     ^^^^^^^^^                                                 ^^^^^^^^^^^
     immutable?                                                observable?
> by stack driver, so IO buffer can be allocated as dma aligned before
     ^^^^^
     stacked?

Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Ming Lei]

On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote:
> On 10/18/18 7:18 AM, Ming Lei wrote:
> > Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
> > for pass-through request, and it isn't done for normal IO request.
> > 
> > Given the check has to be done on each bvec, it isn't efficient to add the
> > check in generic_make_request_checks().
> > 
> > This patch addes one WARN in blk_queue_split() for capturing this issue.
> 
> I don't want to do this, because then we are forever doomed to
> have something that fully loops a bio at submission time. I
> absolutely hate the splitting we have and the need for it,
> hopefully it can go away for a subset of IOs at some point.
> 
> In many ways, this seems to be somewhat of a made-up problem, I don't
> recall a single bug report for something like this over decades of
> working with the IO stack. 512b alignment restrictions for DMA seems
> absolutely insane. I know people claim they exist, but clearly that
> isn't a hard requirement or we would have been boned years ago.

There are still some drivers with this requirement:

drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, sdev->sector_size - 1);
drivers/ata/pata_macio.c:812:           blk_queue_update_dma_alignment(sdev->request_queue, 31);
drivers/ata/pata_macio.c:827:           blk_queue_update_dma_alignment(sdev->request_queue, 15);
drivers/block/ps3disk.c:470:    blk_queue_dma_alignment(queue, dev->blk_size-1);
drivers/block/rsxx/dev.c:282:           blk_queue_dma_alignment(card->queue, blk_size - 1);
drivers/block/xen-blkfront.c:957:       blk_queue_dma_alignment(rq, 511);
drivers/ide/ide-cd.c:1512:      blk_queue_dma_alignment(q, 31);
drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment (sdev->request_queue, 512 - 1);
drivers/staging/rts5208/rtsx.c:94:      blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
drivers/usb/image/microtek.c:329:       blk_queue_dma_alignment(s->request_queue, (512 - 1));
drivers/usb/storage/scsiglue.c:92:      blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
drivers/usb/storage/uas.c:818:  blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));


Thanks,
Ming

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Jens Axboe]

On 10/18/18 7:28 PM, Ming Lei wrote:
> On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote:
>> On 10/18/18 7:18 AM, Ming Lei wrote:
>>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
>>> for pass-through request, and it isn't done for normal IO request.
>>>
>>> Given the check has to be done on each bvec, it isn't efficient to add the
>>> check in generic_make_request_checks().
>>>
>>> This patch addes one WARN in blk_queue_split() for capturing this issue.
>>
>> I don't want to do this, because then we are forever doomed to
>> have something that fully loops a bio at submission time. I
>> absolutely hate the splitting we have and the need for it,
>> hopefully it can go away for a subset of IOs at some point.
>>
>> In many ways, this seems to be somewhat of a made-up problem, I don't
>> recall a single bug report for something like this over decades of
>> working with the IO stack. 512b alignment restrictions for DMA seems
>> absolutely insane. I know people claim they exist, but clearly that
>> isn't a hard requirement or we would have been boned years ago.
> 
> There are still some drivers with this requirement:
> 
> drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, sdev->sector_size - 1);
> drivers/ata/pata_macio.c:812:           blk_queue_update_dma_alignment(sdev->request_queue, 31);
> drivers/ata/pata_macio.c:827:           blk_queue_update_dma_alignment(sdev->request_queue, 15);
> drivers/block/ps3disk.c:470:    blk_queue_dma_alignment(queue, dev->blk_size-1);
> drivers/block/rsxx/dev.c:282:           blk_queue_dma_alignment(card->queue, blk_size - 1);
> drivers/block/xen-blkfront.c:957:       blk_queue_dma_alignment(rq, 511);
> drivers/ide/ide-cd.c:1512:      blk_queue_dma_alignment(q, 31);
> drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment (sdev->request_queue, 512 - 1);
> drivers/staging/rts5208/rtsx.c:94:      blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
> drivers/usb/image/microtek.c:329:       blk_queue_dma_alignment(s->request_queue, (512 - 1));
> drivers/usb/storage/scsiglue.c:92:      blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
> drivers/usb/storage/uas.c:818:  blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));

Of course, I too can grep :-)

My point is that these settings might not match reality. And the
WARN_ON(), as implemented, is going to trigger on any device that
DOESN'T set the alignment, as Bart pointed out.

-- 
Jens Axboe

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Ming Lei]

On Thu, Oct 18, 2018 at 07:33:50PM -0600, Jens Axboe wrote:
> On 10/18/18 7:28 PM, Ming Lei wrote:
> > On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote:
> >> On 10/18/18 7:18 AM, Ming Lei wrote:
> >>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
> >>> for pass-through request, and it isn't done for normal IO request.
> >>>
> >>> Given the check has to be done on each bvec, it isn't efficient to add the
> >>> check in generic_make_request_checks().
> >>>
> >>> This patch addes one WARN in blk_queue_split() for capturing this issue.
> >>
> >> I don't want to do this, because then we are forever doomed to
> >> have something that fully loops a bio at submission time. I
> >> absolutely hate the splitting we have and the need for it,
> >> hopefully it can go away for a subset of IOs at some point.
> >>
> >> In many ways, this seems to be somewhat of a made-up problem, I don't
> >> recall a single bug report for something like this over decades of
> >> working with the IO stack. 512b alignment restrictions for DMA seems
> >> absolutely insane. I know people claim they exist, but clearly that
> >> isn't a hard requirement or we would have been boned years ago.
> > 
> > There are still some drivers with this requirement:
> > 
> > drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, sdev->sector_size - 1);
> > drivers/ata/pata_macio.c:812:           blk_queue_update_dma_alignment(sdev->request_queue, 31);
> > drivers/ata/pata_macio.c:827:           blk_queue_update_dma_alignment(sdev->request_queue, 15);
> > drivers/block/ps3disk.c:470:    blk_queue_dma_alignment(queue, dev->blk_size-1);
> > drivers/block/rsxx/dev.c:282:           blk_queue_dma_alignment(card->queue, blk_size - 1);
> > drivers/block/xen-blkfront.c:957:       blk_queue_dma_alignment(rq, 511);
> > drivers/ide/ide-cd.c:1512:      blk_queue_dma_alignment(q, 31);
> > drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment (sdev->request_queue, 512 - 1);
> > drivers/staging/rts5208/rtsx.c:94:      blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
> > drivers/usb/image/microtek.c:329:       blk_queue_dma_alignment(s->request_queue, (512 - 1));
> > drivers/usb/storage/scsiglue.c:92:      blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
> > drivers/usb/storage/uas.c:818:  blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
> 
> Of course, I too can grep :-)
> 
> My point is that these settings might not match reality. And the
> WARN_ON(), as implemented, is going to trigger on any device that
> DOESN'T set the alignment, as Bart pointed out.

It is just a WARN_ON_ONCE() which exactly shows something which need
further attention, then related people may take a look and we can move
on.

So I think it is correct thing to do.

Thanks,
Ming

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Jens Axboe]

On 10/18/18 7:39 PM, Ming Lei wrote:
> On Thu, Oct 18, 2018 at 07:33:50PM -0600, Jens Axboe wrote:
>> On 10/18/18 7:28 PM, Ming Lei wrote:
>>> On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote:
>>>> On 10/18/18 7:18 AM, Ming Lei wrote:
>>>>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
>>>>> for pass-through request, and it isn't done for normal IO request.
>>>>>
>>>>> Given the check has to be done on each bvec, it isn't efficient to add the
>>>>> check in generic_make_request_checks().
>>>>>
>>>>> This patch addes one WARN in blk_queue_split() for capturing this issue.
>>>>
>>>> I don't want to do this, because then we are forever doomed to
>>>> have something that fully loops a bio at submission time. I
>>>> absolutely hate the splitting we have and the need for it,
>>>> hopefully it can go away for a subset of IOs at some point.
>>>>
>>>> In many ways, this seems to be somewhat of a made-up problem, I don't
>>>> recall a single bug report for something like this over decades of
>>>> working with the IO stack. 512b alignment restrictions for DMA seems
>>>> absolutely insane. I know people claim they exist, but clearly that
>>>> isn't a hard requirement or we would have been boned years ago.
>>>
>>> There are still some drivers with this requirement:
>>>
>>> drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, sdev->sector_size - 1);
>>> drivers/ata/pata_macio.c:812:           blk_queue_update_dma_alignment(sdev->request_queue, 31);
>>> drivers/ata/pata_macio.c:827:           blk_queue_update_dma_alignment(sdev->request_queue, 15);
>>> drivers/block/ps3disk.c:470:    blk_queue_dma_alignment(queue, dev->blk_size-1);
>>> drivers/block/rsxx/dev.c:282:           blk_queue_dma_alignment(card->queue, blk_size - 1);
>>> drivers/block/xen-blkfront.c:957:       blk_queue_dma_alignment(rq, 511);
>>> drivers/ide/ide-cd.c:1512:      blk_queue_dma_alignment(q, 31);
>>> drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment (sdev->request_queue, 512 - 1);
>>> drivers/staging/rts5208/rtsx.c:94:      blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
>>> drivers/usb/image/microtek.c:329:       blk_queue_dma_alignment(s->request_queue, (512 - 1));
>>> drivers/usb/storage/scsiglue.c:92:      blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
>>> drivers/usb/storage/uas.c:818:  blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
>>
>> Of course, I too can grep :-)
>>
>> My point is that these settings might not match reality. And the
>> WARN_ON(), as implemented, is going to trigger on any device that
>> DOESN'T set the alignment, as Bart pointed out.
> 
> It is just a WARN_ON_ONCE() which exactly shows something which need
> further attention, then related people may take a look and we can move
> on.
> 
> So I think it is correct thing to do.

It most certainly is NOT the right thing to do, when we know that:

1) We currently have drivers setting an alignment that we don't meet
2) We have drivers not setting an alignment, and getting 512 by default
3) We have drivers setting an alignment that seems incorrect

It's something that can be debated once everything else has been
done, it's most certainly not something that should be done upfront
when we KNOW it'll trigger for cases. WARN_ON_ONCE() can be useful
to figure out if an invalid condition triggers, it's pointless to
add it for cases that we know exactly how they will trigger.

-- 
Jens Axboe

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Ming Lei]

On Thu, Oct 18, 2018 at 07:52:59PM -0600, Jens Axboe wrote:
> On 10/18/18 7:39 PM, Ming Lei wrote:
> > On Thu, Oct 18, 2018 at 07:33:50PM -0600, Jens Axboe wrote:
> >> On 10/18/18 7:28 PM, Ming Lei wrote:
> >>> On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote:
> >>>> On 10/18/18 7:18 AM, Ming Lei wrote:
> >>>>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
> >>>>> for pass-through request, and it isn't done for normal IO request.
> >>>>>
> >>>>> Given the check has to be done on each bvec, it isn't efficient to add the
> >>>>> check in generic_make_request_checks().
> >>>>>
> >>>>> This patch addes one WARN in blk_queue_split() for capturing this issue.
> >>>>
> >>>> I don't want to do this, because then we are forever doomed to
> >>>> have something that fully loops a bio at submission time. I
> >>>> absolutely hate the splitting we have and the need for it,
> >>>> hopefully it can go away for a subset of IOs at some point.
> >>>>
> >>>> In many ways, this seems to be somewhat of a made-up problem, I don't
> >>>> recall a single bug report for something like this over decades of
> >>>> working with the IO stack. 512b alignment restrictions for DMA seems
> >>>> absolutely insane. I know people claim they exist, but clearly that
> >>>> isn't a hard requirement or we would have been boned years ago.
> >>>
> >>> There are still some drivers with this requirement:
> >>>
> >>> drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, sdev->sector_size - 1);
> >>> drivers/ata/pata_macio.c:812:           blk_queue_update_dma_alignment(sdev->request_queue, 31);
> >>> drivers/ata/pata_macio.c:827:           blk_queue_update_dma_alignment(sdev->request_queue, 15);
> >>> drivers/block/ps3disk.c:470:    blk_queue_dma_alignment(queue, dev->blk_size-1);
> >>> drivers/block/rsxx/dev.c:282:           blk_queue_dma_alignment(card->queue, blk_size - 1);
> >>> drivers/block/xen-blkfront.c:957:       blk_queue_dma_alignment(rq, 511);
> >>> drivers/ide/ide-cd.c:1512:      blk_queue_dma_alignment(q, 31);
> >>> drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment (sdev->request_queue, 512 - 1);
> >>> drivers/staging/rts5208/rtsx.c:94:      blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
> >>> drivers/usb/image/microtek.c:329:       blk_queue_dma_alignment(s->request_queue, (512 - 1));
> >>> drivers/usb/storage/scsiglue.c:92:      blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
> >>> drivers/usb/storage/uas.c:818:  blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
> >>
> >> Of course, I too can grep :-)
> >>
> >> My point is that these settings might not match reality. And the
> >> WARN_ON(), as implemented, is going to trigger on any device that
> >> DOESN'T set the alignment, as Bart pointed out.
> > 
> > It is just a WARN_ON_ONCE() which exactly shows something which need
> > further attention, then related people may take a look and we can move
> > on.
> > 
> > So I think it is correct thing to do.
> 
> It most certainly is NOT the right thing to do, when we know that:
> 
> 1) We currently have drivers setting an alignment that we don't meet
> 2) We have drivers not setting an alignment, and getting 512 by default

The 512 default should have been removed given it isn't respected at
all in normal io path, but it is included from the beginning of 2.6.12

> 3) We have drivers setting an alignment that seems incorrect

Then WARN_ON() is helpful for both 1) and 2) after the default 512
limit is removed.

Thanks,
Ming

Re: [PATCH 1/5] block: warn on un-aligned DMA IO buffer

[Jens Axboe]

On 10/18/18 8:06 PM, Ming Lei wrote:
> On Thu, Oct 18, 2018 at 07:52:59PM -0600, Jens Axboe wrote:
>> On 10/18/18 7:39 PM, Ming Lei wrote:
>>> On Thu, Oct 18, 2018 at 07:33:50PM -0600, Jens Axboe wrote:
>>>> On 10/18/18 7:28 PM, Ming Lei wrote:
>>>>> On Thu, Oct 18, 2018 at 08:27:28AM -0600, Jens Axboe wrote:
>>>>>> On 10/18/18 7:18 AM, Ming Lei wrote:
>>>>>>> Now we only check if DMA IO buffer is aligned to queue_dma_alignment()
>>>>>>> for pass-through request, and it isn't done for normal IO request.
>>>>>>>
>>>>>>> Given the check has to be done on each bvec, it isn't efficient to add the
>>>>>>> check in generic_make_request_checks().
>>>>>>>
>>>>>>> This patch addes one WARN in blk_queue_split() for capturing this issue.
>>>>>>
>>>>>> I don't want to do this, because then we are forever doomed to
>>>>>> have something that fully loops a bio at submission time. I
>>>>>> absolutely hate the splitting we have and the need for it,
>>>>>> hopefully it can go away for a subset of IOs at some point.
>>>>>>
>>>>>> In many ways, this seems to be somewhat of a made-up problem, I don't
>>>>>> recall a single bug report for something like this over decades of
>>>>>> working with the IO stack. 512b alignment restrictions for DMA seems
>>>>>> absolutely insane. I know people claim they exist, but clearly that
>>>>>> isn't a hard requirement or we would have been boned years ago.
>>>>>
>>>>> There are still some drivers with this requirement:
>>>>>
>>>>> drivers/ata/libata-scsi.c:1308: blk_queue_update_dma_alignment(q, sdev->sector_size - 1);
>>>>> drivers/ata/pata_macio.c:812:           blk_queue_update_dma_alignment(sdev->request_queue, 31);
>>>>> drivers/ata/pata_macio.c:827:           blk_queue_update_dma_alignment(sdev->request_queue, 15);
>>>>> drivers/block/ps3disk.c:470:    blk_queue_dma_alignment(queue, dev->blk_size-1);
>>>>> drivers/block/rsxx/dev.c:282:           blk_queue_dma_alignment(card->queue, blk_size - 1);
>>>>> drivers/block/xen-blkfront.c:957:       blk_queue_dma_alignment(rq, 511);
>>>>> drivers/ide/ide-cd.c:1512:      blk_queue_dma_alignment(q, 31);
>>>>> drivers/message/fusion/mptscsih.c:2388: blk_queue_dma_alignment (sdev->request_queue, 512 - 1);
>>>>> drivers/staging/rts5208/rtsx.c:94:      blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
>>>>> drivers/usb/image/microtek.c:329:       blk_queue_dma_alignment(s->request_queue, (512 - 1));
>>>>> drivers/usb/storage/scsiglue.c:92:      blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
>>>>> drivers/usb/storage/uas.c:818:  blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
>>>>
>>>> Of course, I too can grep :-)
>>>>
>>>> My point is that these settings might not match reality. And the
>>>> WARN_ON(), as implemented, is going to trigger on any device that
>>>> DOESN'T set the alignment, as Bart pointed out.
>>>
>>> It is just a WARN_ON_ONCE() which exactly shows something which need
>>> further attention, then related people may take a look and we can move
>>> on.
>>>
>>> So I think it is correct thing to do.
>>
>> It most certainly is NOT the right thing to do, when we know that:
>>
>> 1) We currently have drivers setting an alignment that we don't meet
>> 2) We have drivers not setting an alignment, and getting 512 by default
> 
> The 512 default should have been removed given it isn't respected at
> all in normal io path, but it is included from the beginning of 2.6.12
> 
>> 3) We have drivers setting an alignment that seems incorrect
> 
> Then WARN_ON() is helpful for both 1) and 2) after the default 512
> limit is removed.

I'm not saying it's not useful (though even that is doubtful), I'm
saying it's exactly the wrong order. You cut the very paragraph where
I stated that. Drop this patch, focus on the other bits. Once that is
done, then we can debate whether it's useful or not. Right now it
definitely isn't.

-- 
Jens Axboe

Re: [PATCH 4/5] block: introduce helpers for allocating IO buffers from slab

[Ming Lei]

On Thu, Oct 18, 2018 at 05:22:19PM +0200, Christoph Hellwig wrote:
> On Thu, Oct 18, 2018 at 08:11:23AM -0700, Matthew Wilcox wrote:
> > On Thu, Oct 18, 2018 at 04:42:07PM +0200, Christoph Hellwig wrote:
> > > This all seems quite complicated.
> > > 
> > > I think the interface we'd want is more one that has a little
> > > cache of a single page in the queue, and a little bitmap which
> > > sub-page size blocks of it are used.
> > > 
> > > Something like (pseudo code minus locking):
> > > 
> > > void *blk_alloc_sector_buffer(struct block_device *bdev, gfp_t gfp)
> > > {
> > > 	unsigned block_size = block_size(bdev);
> > > 
> > > 	if (blocksize >= PAGE_SIZE)
> > > 		return (void *)__get_free_pages(gfp, get_order(blocksize));
> > > 
> > > 	if (bdev->fragment_cache_page) {
> > > 		[ <find fragment in bdev->fragment_cache_page using
> > > 		  e.g. bitmap and return if found]
> > > 	}
> > > 
> > > 	bdev->fragment_cache_page = (void *)__get_free_page(gfp);
> > > 	goto find_again;
> > > }
> > 
> > This looks a lot like page_frag_alloc() except I think page_frag_alloc()
> > may be more efficient.
> 
> Oh, nice. Sounds like XFS should just use page_frag_alloc.  I'll give
> it a spin.

XFS or other fs can use page_frag_alloc() directly, seems not necessary to
introduce this change in block layer any more given 512-aligned buffer
should be fine everywhere.

The only benefit to make it as block helper is that the offset or size
can be checked with q->dma_alignment.

Dave/Jens, do you think which way is better? Put allocation as block
helper or fs uses page_frag_alloc() directly for allocating 512*N-byte 
buffer(total size is less than PAGE_SIZE)?

Thanks,
Ming

Re: [PATCH 4/5] block: introduce helpers for allocating IO buffers from slab

[Jens Axboe]

On 10/18/18 8:53 PM, Ming Lei wrote:
> On Thu, Oct 18, 2018 at 05:22:19PM +0200, Christoph Hellwig wrote:
>> On Thu, Oct 18, 2018 at 08:11:23AM -0700, Matthew Wilcox wrote:
>>> On Thu, Oct 18, 2018 at 04:42:07PM +0200, Christoph Hellwig wrote:
>>>> This all seems quite complicated.
>>>>
>>>> I think the interface we'd want is more one that has a little
>>>> cache of a single page in the queue, and a little bitmap which
>>>> sub-page size blocks of it are used.
>>>>
>>>> Something like (pseudo code minus locking):
>>>>
>>>> void *blk_alloc_sector_buffer(struct block_device *bdev, gfp_t gfp)
>>>> {
>>>> 	unsigned block_size = block_size(bdev);
>>>>
>>>> 	if (blocksize >= PAGE_SIZE)
>>>> 		return (void *)__get_free_pages(gfp, get_order(blocksize));
>>>>
>>>> 	if (bdev->fragment_cache_page) {
>>>> 		[ <find fragment in bdev->fragment_cache_page using
>>>> 		  e.g. bitmap and return if found]
>>>> 	}
>>>>
>>>> 	bdev->fragment_cache_page = (void *)__get_free_page(gfp);
>>>> 	goto find_again;
>>>> }
>>>
>>> This looks a lot like page_frag_alloc() except I think page_frag_alloc()
>>> may be more efficient.
>>
>> Oh, nice. Sounds like XFS should just use page_frag_alloc.  I'll give
>> it a spin.
> 
> XFS or other fs can use page_frag_alloc() directly, seems not necessary to
> introduce this change in block layer any more given 512-aligned buffer
> should be fine everywhere.
> 
> The only benefit to make it as block helper is that the offset or size
> can be checked with q->dma_alignment.
> 
> Dave/Jens, do you think which way is better? Put allocation as block
> helper or fs uses page_frag_alloc() directly for allocating 512*N-byte 
> buffer(total size is less than PAGE_SIZE)?

I'd greatly prefer having the FS use that directly, seems kind of
pointless to provide an abstraction for that at that point.

-- 
Jens Axboe

Re: [PATCH 4/5] block: introduce helpers for allocating IO buffers from slab

[Dave Chinner]

On Fri, Oct 19, 2018 at 10:53:49AM +0800, Ming Lei wrote:
> On Thu, Oct 18, 2018 at 05:22:19PM +0200, Christoph Hellwig wrote:
> > On Thu, Oct 18, 2018 at 08:11:23AM -0700, Matthew Wilcox wrote:
> > > On Thu, Oct 18, 2018 at 04:42:07PM +0200, Christoph Hellwig wrote:
> > > > This all seems quite complicated.
> > > > 
> > > > I think the interface we'd want is more one that has a little
> > > > cache of a single page in the queue, and a little bitmap which
> > > > sub-page size blocks of it are used.
> > > > 
> > > > Something like (pseudo code minus locking):
> > > > 
> > > > void *blk_alloc_sector_buffer(struct block_device *bdev, gfp_t gfp)
> > > > {
> > > > 	unsigned block_size = block_size(bdev);
> > > > 
> > > > 	if (blocksize >= PAGE_SIZE)
> > > > 		return (void *)__get_free_pages(gfp, get_order(blocksize));
> > > > 
> > > > 	if (bdev->fragment_cache_page) {
> > > > 		[ <find fragment in bdev->fragment_cache_page using
> > > > 		  e.g. bitmap and return if found]
> > > > 	}
> > > > 
> > > > 	bdev->fragment_cache_page = (void *)__get_free_page(gfp);
> > > > 	goto find_again;
> > > > }
> > > 
> > > This looks a lot like page_frag_alloc() except I think page_frag_alloc()
> > > may be more efficient.
> > 
> > Oh, nice. Sounds like XFS should just use page_frag_alloc.  I'll give
> > it a spin.
> 
> XFS or other fs can use page_frag_alloc() directly, seems not necessary to
> introduce this change in block layer any more given 512-aligned buffer
> should be fine everywhere.
> 
> The only benefit to make it as block helper is that the offset or size
> can be checked with q->dma_alignment.
> 
> Dave/Jens, do you think which way is better? Put allocation as block
> helper or fs uses page_frag_alloc() directly for allocating 512*N-byte 
> buffer(total size is less than PAGE_SIZE)?

Cristoph has already said he's looking at using page_frag_alloc()
directly in XFS....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com