All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] brd: implement discard
@ 2022-09-16  8:58 ` Mikulas Patocka
  0 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  8:58 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

Hi

This series of patches implements discard, write_zeroes and secure_erase 
support for the brd driver.

Zdenek asked me to write it, because we use brd in the lvm2 testsuite and 
it would be benefical to run the testsuite with discard enabled in order 
to test discard handling.

This patch series should have no performance impact - it doesn't add any 
locks to the common I/O paths. It only extends rcu read region around 
lookup and reading or writing of a single page. Discarded pages are freed 
with "call_rcu" to make sure that if we mix discard with I/O, the I/O 
won't access freed memory.

Mikulas


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dm-devel] [PATCH 0/4] brd: implement discard
@ 2022-09-16  8:58 ` Mikulas Patocka
  0 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  8:58 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

Hi

This series of patches implements discard, write_zeroes and secure_erase 
support for the brd driver.

Zdenek asked me to write it, because we use brd in the lvm2 testsuite and 
it would be benefical to run the testsuite with discard enabled in order 
to test discard handling.

This patch series should have no performance impact - it doesn't add any 
locks to the common I/O paths. It only extends rcu read region around 
lookup and reading or writing of a single page. Discarded pages are freed 
with "call_rcu" to make sure that if we mix discard with I/O, the I/O 
won't access freed memory.

Mikulas
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/4] brd: make brd_insert_page return bool
  2022-09-16  8:58 ` [dm-devel] " Mikulas Patocka
@ 2022-09-16  8:59   ` Mikulas Patocka
  -1 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  8:59 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

brd_insert_page returns a pointer to struct page, however the pointer is
never used (it is only compared against NULL), so to clean-up the code, we
make it return bool.

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

---
 drivers/block/brd.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c
+++ linux-2.6/drivers/block/brd.c
@@ -78,11 +78,11 @@ static struct page *brd_lookup_page(stru
 }
 
 /*
- * Look up and return a brd's page for a given sector.
- * If one does not exist, allocate an empty page, and insert that. Then
- * return it.
+ * If the page exists, return true.
+ * If it does not exist, allocate an empty page, and insert that. Return true
+ * if the allocation was successful.
  */
-static struct page *brd_insert_page(struct brd_device *brd, sector_t sector)
+static bool brd_insert_page(struct brd_device *brd, sector_t sector)
 {
 	pgoff_t idx;
 	struct page *page;
@@ -90,7 +90,7 @@ static struct page *brd_insert_page(stru
 
 	page = brd_lookup_page(brd, sector);
 	if (page)
-		return page;
+		return true;
 
 	/*
 	 * Must use NOIO because we don't want to recurse back into the
@@ -99,11 +99,11 @@ static struct page *brd_insert_page(stru
 	gfp_flags = GFP_NOIO | __GFP_ZERO | __GFP_HIGHMEM;
 	page = alloc_page(gfp_flags);
 	if (!page)
-		return NULL;
+		return false;
 
 	if (radix_tree_preload(GFP_NOIO)) {
 		__free_page(page);
-		return NULL;
+		return false;
 	}
 
 	spin_lock(&brd->brd_lock);
@@ -121,7 +121,7 @@ static struct page *brd_insert_page(stru
 
 	radix_tree_preload_end();
 
-	return page;
+	return true;
 }
 
 /*


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dm-devel] [PATCH 1/4] brd: make brd_insert_page return bool
@ 2022-09-16  8:59   ` Mikulas Patocka
  0 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  8:59 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

brd_insert_page returns a pointer to struct page, however the pointer is
never used (it is only compared against NULL), so to clean-up the code, we
make it return bool.

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

---
 drivers/block/brd.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c
+++ linux-2.6/drivers/block/brd.c
@@ -78,11 +78,11 @@ static struct page *brd_lookup_page(stru
 }
 
 /*
- * Look up and return a brd's page for a given sector.
- * If one does not exist, allocate an empty page, and insert that. Then
- * return it.
+ * If the page exists, return true.
+ * If it does not exist, allocate an empty page, and insert that. Return true
+ * if the allocation was successful.
  */
-static struct page *brd_insert_page(struct brd_device *brd, sector_t sector)
+static bool brd_insert_page(struct brd_device *brd, sector_t sector)
 {
 	pgoff_t idx;
 	struct page *page;
@@ -90,7 +90,7 @@ static struct page *brd_insert_page(stru
 
 	page = brd_lookup_page(brd, sector);
 	if (page)
-		return page;
+		return true;
 
 	/*
 	 * Must use NOIO because we don't want to recurse back into the
@@ -99,11 +99,11 @@ static struct page *brd_insert_page(stru
 	gfp_flags = GFP_NOIO | __GFP_ZERO | __GFP_HIGHMEM;
 	page = alloc_page(gfp_flags);
 	if (!page)
-		return NULL;
+		return false;
 
 	if (radix_tree_preload(GFP_NOIO)) {
 		__free_page(page);
-		return NULL;
+		return false;
 	}
 
 	spin_lock(&brd->brd_lock);
@@ -121,7 +121,7 @@ static struct page *brd_insert_page(stru
 
 	radix_tree_preload_end();
 
-	return page;
+	return true;
 }
 
 /*

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 2/4] brd: extend the rcu regions to cover read and write
  2022-09-16  8:58 ` [dm-devel] " Mikulas Patocka
@ 2022-09-16  8:59   ` Mikulas Patocka
  -1 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  8:59 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

This patch extends the rcu regions, so that lookup followed by a read or
write of a page is done inside rcu read lock. This si be needed for the
following patch that enables discard.

Note that we also replace "BUG_ON(!page);" with "if (page) ..." in
copy_to_brd - the page may be NULL if write races with discard. In this
situation, the result is undefined, so we can actually skip the write
operation at all.

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

---
 drivers/block/brd.c |   50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c
+++ linux-2.6/drivers/block/brd.c
@@ -50,29 +50,15 @@ struct brd_device {
 
 /*
  * Look up and return a brd's page for a given sector.
+ * This must be called with the rcu lock held.
  */
 static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector)
 {
 	pgoff_t idx;
 	struct page *page;
 
-	/*
-	 * The page lifetime is protected by the fact that we have opened the
-	 * device node -- brd pages will never be deleted under us, so we
-	 * don't need any further locking or refcounting.
-	 *
-	 * This is strictly true for the radix-tree nodes as well (ie. we
-	 * don't actually need the rcu_read_lock()), however that is not a
-	 * documented feature of the radix-tree API so it is better to be
-	 * safe here (we don't have total exclusion from radix tree updates
-	 * here, only deletes).
-	 */
-	rcu_read_lock();
 	idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */
 	page = radix_tree_lookup(&brd->brd_pages, idx);
-	rcu_read_unlock();
-
-	BUG_ON(page && page->index != idx);
 
 	return page;
 }
@@ -88,7 +74,9 @@ static bool brd_insert_page(struct brd_d
 	struct page *page;
 	gfp_t gfp_flags;
 
+	rcu_read_lock();
 	page = brd_lookup_page(brd, sector);
+	rcu_read_unlock();
 	if (page)
 		return true;
 
@@ -198,23 +186,29 @@ static void copy_to_brd(struct brd_devic
 	size_t copy;
 
 	copy = min_t(size_t, n, PAGE_SIZE - offset);
-	page = brd_lookup_page(brd, sector);
-	BUG_ON(!page);
 
-	dst = kmap_atomic(page);
-	memcpy(dst + offset, src, copy);
-	kunmap_atomic(dst);
+	rcu_read_lock();
+	page = brd_lookup_page(brd, sector);
+	if (page) {
+		dst = kmap_atomic(page);
+		memcpy(dst + offset, src, copy);
+		kunmap_atomic(dst);
+	}
+	rcu_read_unlock();
 
 	if (copy < n) {
 		src += copy;
 		sector += copy >> SECTOR_SHIFT;
 		copy = n - copy;
-		page = brd_lookup_page(brd, sector);
-		BUG_ON(!page);
 
-		dst = kmap_atomic(page);
-		memcpy(dst, src, copy);
-		kunmap_atomic(dst);
+		rcu_read_lock();
+		page = brd_lookup_page(brd, sector);
+		if (page) {
+			dst = kmap_atomic(page);
+			memcpy(dst, src, copy);
+			kunmap_atomic(dst);
+		}
+		rcu_read_unlock();
 	}
 }
 
@@ -230,6 +224,8 @@ static void copy_from_brd(void *dst, str
 	size_t copy;
 
 	copy = min_t(size_t, n, PAGE_SIZE - offset);
+
+	rcu_read_lock();
 	page = brd_lookup_page(brd, sector);
 	if (page) {
 		src = kmap_atomic(page);
@@ -237,11 +233,14 @@ static void copy_from_brd(void *dst, str
 		kunmap_atomic(src);
 	} else
 		memset(dst, 0, copy);
+	rcu_read_unlock();
 
 	if (copy < n) {
 		dst += copy;
 		sector += copy >> SECTOR_SHIFT;
 		copy = n - copy;
+
+		rcu_read_lock();
 		page = brd_lookup_page(brd, sector);
 		if (page) {
 			src = kmap_atomic(page);
@@ -249,6 +248,7 @@ static void copy_from_brd(void *dst, str
 			kunmap_atomic(src);
 		} else
 			memset(dst, 0, copy);
+		rcu_read_unlock();
 	}
 }
 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dm-devel] [PATCH 2/4] brd: extend the rcu regions to cover read and write
@ 2022-09-16  8:59   ` Mikulas Patocka
  0 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  8:59 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

This patch extends the rcu regions, so that lookup followed by a read or
write of a page is done inside rcu read lock. This si be needed for the
following patch that enables discard.

Note that we also replace "BUG_ON(!page);" with "if (page) ..." in
copy_to_brd - the page may be NULL if write races with discard. In this
situation, the result is undefined, so we can actually skip the write
operation at all.

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

---
 drivers/block/brd.c |   50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c
+++ linux-2.6/drivers/block/brd.c
@@ -50,29 +50,15 @@ struct brd_device {
 
 /*
  * Look up and return a brd's page for a given sector.
+ * This must be called with the rcu lock held.
  */
 static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector)
 {
 	pgoff_t idx;
 	struct page *page;
 
-	/*
-	 * The page lifetime is protected by the fact that we have opened the
-	 * device node -- brd pages will never be deleted under us, so we
-	 * don't need any further locking or refcounting.
-	 *
-	 * This is strictly true for the radix-tree nodes as well (ie. we
-	 * don't actually need the rcu_read_lock()), however that is not a
-	 * documented feature of the radix-tree API so it is better to be
-	 * safe here (we don't have total exclusion from radix tree updates
-	 * here, only deletes).
-	 */
-	rcu_read_lock();
 	idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */
 	page = radix_tree_lookup(&brd->brd_pages, idx);
-	rcu_read_unlock();
-
-	BUG_ON(page && page->index != idx);
 
 	return page;
 }
@@ -88,7 +74,9 @@ static bool brd_insert_page(struct brd_d
 	struct page *page;
 	gfp_t gfp_flags;
 
+	rcu_read_lock();
 	page = brd_lookup_page(brd, sector);
+	rcu_read_unlock();
 	if (page)
 		return true;
 
@@ -198,23 +186,29 @@ static void copy_to_brd(struct brd_devic
 	size_t copy;
 
 	copy = min_t(size_t, n, PAGE_SIZE - offset);
-	page = brd_lookup_page(brd, sector);
-	BUG_ON(!page);
 
-	dst = kmap_atomic(page);
-	memcpy(dst + offset, src, copy);
-	kunmap_atomic(dst);
+	rcu_read_lock();
+	page = brd_lookup_page(brd, sector);
+	if (page) {
+		dst = kmap_atomic(page);
+		memcpy(dst + offset, src, copy);
+		kunmap_atomic(dst);
+	}
+	rcu_read_unlock();
 
 	if (copy < n) {
 		src += copy;
 		sector += copy >> SECTOR_SHIFT;
 		copy = n - copy;
-		page = brd_lookup_page(brd, sector);
-		BUG_ON(!page);
 
-		dst = kmap_atomic(page);
-		memcpy(dst, src, copy);
-		kunmap_atomic(dst);
+		rcu_read_lock();
+		page = brd_lookup_page(brd, sector);
+		if (page) {
+			dst = kmap_atomic(page);
+			memcpy(dst, src, copy);
+			kunmap_atomic(dst);
+		}
+		rcu_read_unlock();
 	}
 }
 
@@ -230,6 +224,8 @@ static void copy_from_brd(void *dst, str
 	size_t copy;
 
 	copy = min_t(size_t, n, PAGE_SIZE - offset);
+
+	rcu_read_lock();
 	page = brd_lookup_page(brd, sector);
 	if (page) {
 		src = kmap_atomic(page);
@@ -237,11 +233,14 @@ static void copy_from_brd(void *dst, str
 		kunmap_atomic(src);
 	} else
 		memset(dst, 0, copy);
+	rcu_read_unlock();
 
 	if (copy < n) {
 		dst += copy;
 		sector += copy >> SECTOR_SHIFT;
 		copy = n - copy;
+
+		rcu_read_lock();
 		page = brd_lookup_page(brd, sector);
 		if (page) {
 			src = kmap_atomic(page);
@@ -249,6 +248,7 @@ static void copy_from_brd(void *dst, str
 			kunmap_atomic(src);
 		} else
 			memset(dst, 0, copy);
+		rcu_read_unlock();
 	}
 }
 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dm-devel] [PATCH 3/4] brd: enable discard
  2022-09-16  8:58 ` [dm-devel] " Mikulas Patocka
@ 2022-09-16  9:00   ` Mikulas Patocka
  -1 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  9:00 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

This patch implements discard in the brd driver. We use RCU to free the
page, so that if there are any concurrent readers or writes, they won't
touch the page after it is freed.

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

---
 drivers/block/brd.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c
+++ linux-2.6/drivers/block/brd.c
@@ -112,6 +112,25 @@ static bool brd_insert_page(struct brd_d
 	return true;
 }
 
+static void brd_free_page_rcu(struct rcu_head *head)
+{
+	struct page *page = container_of(head, struct page, rcu_head);
+	__free_page(page);
+}
+
+static void brd_free_page(struct brd_device *brd, sector_t sector)
+{
+	struct page *page;
+	pgoff_t idx;
+
+	spin_lock(&brd->brd_lock);
+	idx = sector >> PAGE_SECTORS_SHIFT;
+	page = radix_tree_delete(&brd->brd_pages, idx);
+	spin_unlock(&brd->brd_lock);
+	if (page)
+		call_rcu(&page->rcu_head, brd_free_page_rcu);
+}
+
 /*
  * Free all backing store pages and radix tree. This must only be called when
  * there are no other users of the device.
@@ -289,6 +308,23 @@ static void brd_submit_bio(struct bio *b
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 
+	if (bio_op(bio) == REQ_OP_DISCARD) {
+		sector_t len = bio_sectors(bio);
+		sector_t front_pad = -sector & (PAGE_SECTORS - 1);
+		sector += front_pad;
+		if (unlikely(len <= front_pad))
+			goto endio;
+		len -= front_pad;
+		len = round_down(len, PAGE_SECTORS);
+		while (len) {
+			brd_free_page(brd, sector);
+			sector += PAGE_SECTORS;
+			len -= PAGE_SECTORS;
+			cond_resched();
+		}
+		goto endio;
+	}
+
 	bio_for_each_segment(bvec, bio, iter) {
 		unsigned int len = bvec.bv_len;
 		int err;
@@ -306,6 +342,7 @@ static void brd_submit_bio(struct bio *b
 		sector += len >> SECTOR_SHIFT;
 	}
 
+endio:
 	bio_endio(bio);
 }
 
@@ -409,6 +446,9 @@ static int brd_alloc(int i)
 	 */
 	blk_queue_physical_block_size(disk->queue, PAGE_SIZE);
 
+	disk->queue->limits.discard_granularity = PAGE_SIZE;
+	blk_queue_max_discard_sectors(disk->queue, UINT_MAX);
+
 	/* Tell the block layer that this is not a rotational device */
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
 	blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, disk->queue);

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/4] brd: enable discard
@ 2022-09-16  9:00   ` Mikulas Patocka
  0 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  9:00 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

This patch implements discard in the brd driver. We use RCU to free the
page, so that if there are any concurrent readers or writes, they won't
touch the page after it is freed.

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

---
 drivers/block/brd.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c
+++ linux-2.6/drivers/block/brd.c
@@ -112,6 +112,25 @@ static bool brd_insert_page(struct brd_d
 	return true;
 }
 
+static void brd_free_page_rcu(struct rcu_head *head)
+{
+	struct page *page = container_of(head, struct page, rcu_head);
+	__free_page(page);
+}
+
+static void brd_free_page(struct brd_device *brd, sector_t sector)
+{
+	struct page *page;
+	pgoff_t idx;
+
+	spin_lock(&brd->brd_lock);
+	idx = sector >> PAGE_SECTORS_SHIFT;
+	page = radix_tree_delete(&brd->brd_pages, idx);
+	spin_unlock(&brd->brd_lock);
+	if (page)
+		call_rcu(&page->rcu_head, brd_free_page_rcu);
+}
+
 /*
  * Free all backing store pages and radix tree. This must only be called when
  * there are no other users of the device.
@@ -289,6 +308,23 @@ static void brd_submit_bio(struct bio *b
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 
+	if (bio_op(bio) == REQ_OP_DISCARD) {
+		sector_t len = bio_sectors(bio);
+		sector_t front_pad = -sector & (PAGE_SECTORS - 1);
+		sector += front_pad;
+		if (unlikely(len <= front_pad))
+			goto endio;
+		len -= front_pad;
+		len = round_down(len, PAGE_SECTORS);
+		while (len) {
+			brd_free_page(brd, sector);
+			sector += PAGE_SECTORS;
+			len -= PAGE_SECTORS;
+			cond_resched();
+		}
+		goto endio;
+	}
+
 	bio_for_each_segment(bvec, bio, iter) {
 		unsigned int len = bvec.bv_len;
 		int err;
@@ -306,6 +342,7 @@ static void brd_submit_bio(struct bio *b
 		sector += len >> SECTOR_SHIFT;
 	}
 
+endio:
 	bio_endio(bio);
 }
 
@@ -409,6 +446,9 @@ static int brd_alloc(int i)
 	 */
 	blk_queue_physical_block_size(disk->queue, PAGE_SIZE);
 
+	disk->queue->limits.discard_granularity = PAGE_SIZE;
+	blk_queue_max_discard_sectors(disk->queue, UINT_MAX);
+
 	/* Tell the block layer that this is not a rotational device */
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
 	blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, disk->queue);


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [dm-devel] [PATCH 4/4] brd: implement secure erase and write zeroes
  2022-09-16  8:58 ` [dm-devel] " Mikulas Patocka
@ 2022-09-16  9:00   ` Mikulas Patocka
  -1 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  9:00 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

This patch implements REQ_OP_SECURE_ERASE and REQ_OP_WRITE_ZEROES on brd.
Write zeroes will free the pages just like discard, but the difference is
that it writes zeroes to the preceding and following page if the range is
not aligned on page boundary. Secure erase is just like write zeroes,
except that it clears the page content before freeing the page.

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

---
 drivers/block/brd.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c
+++ linux-2.6/drivers/block/brd.c
@@ -118,7 +118,7 @@ static void brd_free_page_rcu(struct rcu
 	__free_page(page);
 }
 
-static void brd_free_page(struct brd_device *brd, sector_t sector)
+static void brd_free_page(struct brd_device *brd, sector_t sector, bool secure)
 {
 	struct page *page;
 	pgoff_t idx;
@@ -127,8 +127,11 @@ static void brd_free_page(struct brd_dev
 	idx = sector >> PAGE_SECTORS_SHIFT;
 	page = radix_tree_delete(&brd->brd_pages, idx);
 	spin_unlock(&brd->brd_lock);
-	if (page)
+	if (page) {
+		if (secure)
+			clear_highpage(page);
 		call_rcu(&page->rcu_head, brd_free_page_rcu);
+	}
 }
 
 /*
@@ -308,16 +311,29 @@ static void brd_submit_bio(struct bio *b
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 
-	if (bio_op(bio) == REQ_OP_DISCARD) {
+	if (bio_op(bio) == REQ_OP_DISCARD ||
+	    bio_op(bio) == REQ_OP_SECURE_ERASE ||
+	    bio_op(bio) == REQ_OP_WRITE_ZEROES) {
+		bool zero_padding = bio_op(bio) == REQ_OP_SECURE_ERASE || bio_op(bio) == REQ_OP_WRITE_ZEROES;
 		sector_t len = bio_sectors(bio);
 		sector_t front_pad = -sector & (PAGE_SECTORS - 1);
+		sector_t end_pad;
+
+		if (zero_padding && unlikely(front_pad != 0))
+			copy_to_brd(brd, page_address(ZERO_PAGE(0)), sector, min(len, front_pad) << SECTOR_SHIFT);
+
 		sector += front_pad;
 		if (unlikely(len <= front_pad))
 			goto endio;
 		len -= front_pad;
-		len = round_down(len, PAGE_SECTORS);
+
+		end_pad = len & (PAGE_SECTORS - 1);
+		if (zero_padding && unlikely(end_pad != 0))
+			copy_to_brd(brd, page_address(ZERO_PAGE(0)), sector + len - end_pad, end_pad << SECTOR_SHIFT);
+		len -= end_pad;
+
 		while (len) {
-			brd_free_page(brd, sector);
+			brd_free_page(brd, sector, bio_op(bio) == REQ_OP_SECURE_ERASE);
 			sector += PAGE_SECTORS;
 			len -= PAGE_SECTORS;
 			cond_resched();
@@ -448,6 +464,8 @@ static int brd_alloc(int i)
 
 	disk->queue->limits.discard_granularity = PAGE_SIZE;
 	blk_queue_max_discard_sectors(disk->queue, UINT_MAX);
+	blk_queue_max_write_zeroes_sectors(disk->queue, UINT_MAX);
+	blk_queue_max_secure_erase_sectors(disk->queue, UINT_MAX);
 
 	/* Tell the block layer that this is not a rotational device */
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 4/4] brd: implement secure erase and write zeroes
@ 2022-09-16  9:00   ` Mikulas Patocka
  0 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-16  9:00 UTC (permalink / raw)
  To: Jens Axboe, Zdenek Kabelac; +Cc: linux-block, dm-devel

This patch implements REQ_OP_SECURE_ERASE and REQ_OP_WRITE_ZEROES on brd.
Write zeroes will free the pages just like discard, but the difference is
that it writes zeroes to the preceding and following page if the range is
not aligned on page boundary. Secure erase is just like write zeroes,
except that it clears the page content before freeing the page.

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

---
 drivers/block/brd.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c
+++ linux-2.6/drivers/block/brd.c
@@ -118,7 +118,7 @@ static void brd_free_page_rcu(struct rcu
 	__free_page(page);
 }
 
-static void brd_free_page(struct brd_device *brd, sector_t sector)
+static void brd_free_page(struct brd_device *brd, sector_t sector, bool secure)
 {
 	struct page *page;
 	pgoff_t idx;
@@ -127,8 +127,11 @@ static void brd_free_page(struct brd_dev
 	idx = sector >> PAGE_SECTORS_SHIFT;
 	page = radix_tree_delete(&brd->brd_pages, idx);
 	spin_unlock(&brd->brd_lock);
-	if (page)
+	if (page) {
+		if (secure)
+			clear_highpage(page);
 		call_rcu(&page->rcu_head, brd_free_page_rcu);
+	}
 }
 
 /*
@@ -308,16 +311,29 @@ static void brd_submit_bio(struct bio *b
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 
-	if (bio_op(bio) == REQ_OP_DISCARD) {
+	if (bio_op(bio) == REQ_OP_DISCARD ||
+	    bio_op(bio) == REQ_OP_SECURE_ERASE ||
+	    bio_op(bio) == REQ_OP_WRITE_ZEROES) {
+		bool zero_padding = bio_op(bio) == REQ_OP_SECURE_ERASE || bio_op(bio) == REQ_OP_WRITE_ZEROES;
 		sector_t len = bio_sectors(bio);
 		sector_t front_pad = -sector & (PAGE_SECTORS - 1);
+		sector_t end_pad;
+
+		if (zero_padding && unlikely(front_pad != 0))
+			copy_to_brd(brd, page_address(ZERO_PAGE(0)), sector, min(len, front_pad) << SECTOR_SHIFT);
+
 		sector += front_pad;
 		if (unlikely(len <= front_pad))
 			goto endio;
 		len -= front_pad;
-		len = round_down(len, PAGE_SECTORS);
+
+		end_pad = len & (PAGE_SECTORS - 1);
+		if (zero_padding && unlikely(end_pad != 0))
+			copy_to_brd(brd, page_address(ZERO_PAGE(0)), sector + len - end_pad, end_pad << SECTOR_SHIFT);
+		len -= end_pad;
+
 		while (len) {
-			brd_free_page(brd, sector);
+			brd_free_page(brd, sector, bio_op(bio) == REQ_OP_SECURE_ERASE);
 			sector += PAGE_SECTORS;
 			len -= PAGE_SECTORS;
 			cond_resched();
@@ -448,6 +464,8 @@ static int brd_alloc(int i)
 
 	disk->queue->limits.discard_granularity = PAGE_SIZE;
 	blk_queue_max_discard_sectors(disk->queue, UINT_MAX);
+	blk_queue_max_write_zeroes_sectors(disk->queue, UINT_MAX);
+	blk_queue_max_secure_erase_sectors(disk->queue, UINT_MAX);
 
 	/* Tell the block layer that this is not a rotational device */
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/4] brd: make brd_insert_page return bool
  2022-09-16  8:59   ` [dm-devel] " Mikulas Patocka
@ 2022-09-20  7:28     ` Christoph Hellwig
  -1 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2022-09-20  7:28 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Jens Axboe, Zdenek Kabelac, linux-block, dm-devel

On Fri, Sep 16, 2022 at 04:59:19AM -0400, Mikulas Patocka wrote:
> brd_insert_page returns a pointer to struct page, however the pointer is
> never used (it is only compared against NULL), so to clean-up the code, we
> make it return bool.

Looks good:

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dm-devel] [PATCH 1/4] brd: make brd_insert_page return bool
@ 2022-09-20  7:28     ` Christoph Hellwig
  0 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2022-09-20  7:28 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Jens Axboe, linux-block, dm-devel, Zdenek Kabelac

On Fri, Sep 16, 2022 at 04:59:19AM -0400, Mikulas Patocka wrote:
> brd_insert_page returns a pointer to struct page, however the pointer is
> never used (it is only compared against NULL), so to clean-up the code, we
> make it return bool.

Looks good:

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/4] brd: implement secure erase and write zeroes
  2022-09-16  9:00   ` Mikulas Patocka
@ 2022-09-20  7:29     ` Christoph Hellwig
  -1 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2022-09-20  7:29 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Jens Axboe, Zdenek Kabelac, linux-block, dm-devel

On Fri, Sep 16, 2022 at 05:00:46AM -0400, Mikulas Patocka wrote:
> This patch implements REQ_OP_SECURE_ERASE and REQ_OP_WRITE_ZEROES on brd.
> Write zeroes will free the pages just like discard, but the difference is
> that it writes zeroes to the preceding and following page if the range is
> not aligned on page boundary. Secure erase is just like write zeroes,
> except that it clears the page content before freeing the page.

What is the use case of this?  And just a single overwrite is not what
storage standards would consider a secure erase, but then again we
don't really have any documentation or standards for the Linux OP,
which strongly suggests not actually implementing it for now.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dm-devel] [PATCH 4/4] brd: implement secure erase and write zeroes
@ 2022-09-20  7:29     ` Christoph Hellwig
  0 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2022-09-20  7:29 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Jens Axboe, linux-block, dm-devel, Zdenek Kabelac

On Fri, Sep 16, 2022 at 05:00:46AM -0400, Mikulas Patocka wrote:
> This patch implements REQ_OP_SECURE_ERASE and REQ_OP_WRITE_ZEROES on brd.
> Write zeroes will free the pages just like discard, but the difference is
> that it writes zeroes to the preceding and following page if the range is
> not aligned on page boundary. Secure erase is just like write zeroes,
> except that it clears the page content before freeing the page.

What is the use case of this?  And just a single overwrite is not what
storage standards would consider a secure erase, but then again we
don't really have any documentation or standards for the Linux OP,
which strongly suggests not actually implementing it for now.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/4] brd: extend the rcu regions to cover read and write
  2022-09-16  8:59   ` [dm-devel] " Mikulas Patocka
@ 2022-09-20  7:38     ` Christoph Hellwig
  -1 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2022-09-20  7:38 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Jens Axboe, Zdenek Kabelac, linux-block, dm-devel

>   * Look up and return a brd's page for a given sector.
> + * This must be called with the rcu lock held.

Please ad a rcu_read_lock_held() check then.

> -	rcu_read_lock();
>  	idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */
>  	page = radix_tree_lookup(&brd->brd_pages, idx);
> -	rcu_read_unlock();
> -
> -	BUG_ON(page && page->index != idx);
>  
>  	return page;

No need for the page variable now.  In fact there is no real need
for this helper now, as all the callers really should operate on
the sector on the index anyway.

>  }
> @@ -88,7 +74,9 @@ static bool brd_insert_page(struct brd_d
>  	struct page *page;
>  	gfp_t gfp_flags;
>  
> +	rcu_read_lock();
>  	page = brd_lookup_page(brd, sector);
> +	rcu_read_unlock();
>  	if (page)
>  		return true;

So this looks odd, as we drop the rcu lock without doing anything,
but it actually turns out to be correct as brd_do_bvec does yet
another lookup of it.  So we get an initial look, and optional
insert and then another lookup.  Not very efficient and it might be
worth to fix brd_do_bvec up to avoid these extra lookups given
that you touch it anyway (as would be an radix tree to xarray
conversion).


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dm-devel] [PATCH 2/4] brd: extend the rcu regions to cover read and write
@ 2022-09-20  7:38     ` Christoph Hellwig
  0 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2022-09-20  7:38 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Jens Axboe, linux-block, dm-devel, Zdenek Kabelac

>   * Look up and return a brd's page for a given sector.
> + * This must be called with the rcu lock held.

Please ad a rcu_read_lock_held() check then.

> -	rcu_read_lock();
>  	idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */
>  	page = radix_tree_lookup(&brd->brd_pages, idx);
> -	rcu_read_unlock();
> -
> -	BUG_ON(page && page->index != idx);
>  
>  	return page;

No need for the page variable now.  In fact there is no real need
for this helper now, as all the callers really should operate on
the sector on the index anyway.

>  }
> @@ -88,7 +74,9 @@ static bool brd_insert_page(struct brd_d
>  	struct page *page;
>  	gfp_t gfp_flags;
>  
> +	rcu_read_lock();
>  	page = brd_lookup_page(brd, sector);
> +	rcu_read_unlock();
>  	if (page)
>  		return true;

So this looks odd, as we drop the rcu lock without doing anything,
but it actually turns out to be correct as brd_do_bvec does yet
another lookup of it.  So we get an initial look, and optional
insert and then another lookup.  Not very efficient and it might be
worth to fix brd_do_bvec up to avoid these extra lookups given
that you touch it anyway (as would be an radix tree to xarray
conversion).

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dm-devel] [PATCH 3/4] brd: enable discard
  2022-09-16  9:00   ` Mikulas Patocka
@ 2022-09-20  7:39     ` Christoph Hellwig
  -1 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2022-09-20  7:39 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Jens Axboe, linux-block, dm-devel, Zdenek Kabelac

> @@ -289,6 +308,23 @@ static void brd_submit_bio(struct bio *b
>  	struct bio_vec bvec;
>  	struct bvec_iter iter;
>  
> +	if (bio_op(bio) == REQ_OP_DISCARD) {
> +		sector_t len = bio_sectors(bio);
> +		sector_t front_pad = -sector & (PAGE_SECTORS - 1);
> +		sector += front_pad;
> +		if (unlikely(len <= front_pad))
> +			goto endio;
> +		len -= front_pad;
> +		len = round_down(len, PAGE_SECTORS);
> +		while (len) {
> +			brd_free_page(brd, sector);
> +			sector += PAGE_SECTORS;
> +			len -= PAGE_SECTORS;
> +			cond_resched();
> +		}
> +		goto endio;
> +	}
> +
>  	bio_for_each_segment(bvec, bio, iter) {

Please add separate helpers to each type of IO and just make the
main submit_bio method a dispatch on the types instead of this
spaghetti code.

> +	disk->queue->limits.discard_granularity = PAGE_SIZE;
> +	blk_queue_max_discard_sectors(disk->queue, UINT_MAX);

We'll probably want an opt in for this new feature.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/4] brd: enable discard
@ 2022-09-20  7:39     ` Christoph Hellwig
  0 siblings, 0 replies; 22+ messages in thread
From: Christoph Hellwig @ 2022-09-20  7:39 UTC (permalink / raw)
  To: Mikulas Patocka; +Cc: Jens Axboe, Zdenek Kabelac, linux-block, dm-devel

> @@ -289,6 +308,23 @@ static void brd_submit_bio(struct bio *b
>  	struct bio_vec bvec;
>  	struct bvec_iter iter;
>  
> +	if (bio_op(bio) == REQ_OP_DISCARD) {
> +		sector_t len = bio_sectors(bio);
> +		sector_t front_pad = -sector & (PAGE_SECTORS - 1);
> +		sector += front_pad;
> +		if (unlikely(len <= front_pad))
> +			goto endio;
> +		len -= front_pad;
> +		len = round_down(len, PAGE_SECTORS);
> +		while (len) {
> +			brd_free_page(brd, sector);
> +			sector += PAGE_SECTORS;
> +			len -= PAGE_SECTORS;
> +			cond_resched();
> +		}
> +		goto endio;
> +	}
> +
>  	bio_for_each_segment(bvec, bio, iter) {

Please add separate helpers to each type of IO and just make the
main submit_bio method a dispatch on the types instead of this
spaghetti code.

> +	disk->queue->limits.discard_granularity = PAGE_SIZE;
> +	blk_queue_max_discard_sectors(disk->queue, UINT_MAX);

We'll probably want an opt in for this new feature.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dm-devel] [PATCH 4/4] brd: implement secure erase and write zeroes
  2022-09-20  7:29     ` [dm-devel] " Christoph Hellwig
@ 2022-09-20 17:46       ` Mikulas Patocka
  -1 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-20 17:46 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-block, dm-devel, Zdenek Kabelac



On Tue, 20 Sep 2022, Christoph Hellwig wrote:

> On Fri, Sep 16, 2022 at 05:00:46AM -0400, Mikulas Patocka wrote:
> > This patch implements REQ_OP_SECURE_ERASE and REQ_OP_WRITE_ZEROES on brd.
> > Write zeroes will free the pages just like discard, but the difference is
> > that it writes zeroes to the preceding and following page if the range is
> > not aligned on page boundary. Secure erase is just like write zeroes,
> > except that it clears the page content before freeing the page.
> 
> What is the use case of this?  And just a single overwrite is not what
> storage standards would consider a secure erase, but then again we
> don't really have any documentation or standards for the Linux OP,
> which strongly suggests not actually implementing it for now.

Without support for REQ_OP_WRITE_ZEROES, "blkdiscard -z" actually 
overwrites the ramdisk with zeroes and allocates all the blocks. 
Allocating all the blocks is pointless if we want to clear them.

I implemented REQ_OP_SECURE_ERASE just because it is similar to 
REQ_OP_WRITE_ZEROES. Unlike disks, DRAM has no memory of previous content, 
so a single overwrite should be OK. We could also flush cache in 
REQ_OP_SECURE_ERASE, but I don't know if Linux has any portable function 
that does it.

Mikulas
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dm-devel] [PATCH 4/4] brd: implement secure erase and write zeroes
@ 2022-09-20 17:46       ` Mikulas Patocka
  0 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-20 17:46 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-block, dm-devel, Zdenek Kabelac



On Tue, 20 Sep 2022, Christoph Hellwig wrote:

> On Fri, Sep 16, 2022 at 05:00:46AM -0400, Mikulas Patocka wrote:
> > This patch implements REQ_OP_SECURE_ERASE and REQ_OP_WRITE_ZEROES on brd.
> > Write zeroes will free the pages just like discard, but the difference is
> > that it writes zeroes to the preceding and following page if the range is
> > not aligned on page boundary. Secure erase is just like write zeroes,
> > except that it clears the page content before freeing the page.
> 
> What is the use case of this?  And just a single overwrite is not what
> storage standards would consider a secure erase, but then again we
> don't really have any documentation or standards for the Linux OP,
> which strongly suggests not actually implementing it for now.

Without support for REQ_OP_WRITE_ZEROES, "blkdiscard -z" actually 
overwrites the ramdisk with zeroes and allocates all the blocks. 
Allocating all the blocks is pointless if we want to clear them.

I implemented REQ_OP_SECURE_ERASE just because it is similar to 
REQ_OP_WRITE_ZEROES. Unlike disks, DRAM has no memory of previous content, 
so a single overwrite should be OK. We could also flush cache in 
REQ_OP_SECURE_ERASE, but I don't know if Linux has any portable function 
that does it.

Mikulas


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [dm-devel] [PATCH 3/4] brd: enable discard
  2022-09-20  7:39     ` Christoph Hellwig
@ 2022-09-20 17:47       ` Mikulas Patocka
  -1 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-20 17:47 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-block, dm-devel, Zdenek Kabelac



On Tue, 20 Sep 2022, Christoph Hellwig wrote:

> > @@ -289,6 +308,23 @@ static void brd_submit_bio(struct bio *b
> >  	struct bio_vec bvec;
> >  	struct bvec_iter iter;
> >  
> > +	if (bio_op(bio) == REQ_OP_DISCARD) {
> > +		sector_t len = bio_sectors(bio);
> > +		sector_t front_pad = -sector & (PAGE_SECTORS - 1);
> > +		sector += front_pad;
> > +		if (unlikely(len <= front_pad))
> > +			goto endio;
> > +		len -= front_pad;
> > +		len = round_down(len, PAGE_SECTORS);
> > +		while (len) {
> > +			brd_free_page(brd, sector);
> > +			sector += PAGE_SECTORS;
> > +			len -= PAGE_SECTORS;
> > +			cond_resched();
> > +		}
> > +		goto endio;
> > +	}
> > +
> >  	bio_for_each_segment(bvec, bio, iter) {
> 
> Please add separate helpers to each type of IO and just make the
> main submit_bio method a dispatch on the types instead of this
> spaghetti code.
> 
> > +	disk->queue->limits.discard_granularity = PAGE_SIZE;
> > +	blk_queue_max_discard_sectors(disk->queue, UINT_MAX);
> 
> We'll probably want an opt in for this new feature.

OK. I addressed these concerns and I'll send a second version of the patch 
set.

Mikulas
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/4] brd: enable discard
@ 2022-09-20 17:47       ` Mikulas Patocka
  0 siblings, 0 replies; 22+ messages in thread
From: Mikulas Patocka @ 2022-09-20 17:47 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, Zdenek Kabelac, linux-block, dm-devel



On Tue, 20 Sep 2022, Christoph Hellwig wrote:

> > @@ -289,6 +308,23 @@ static void brd_submit_bio(struct bio *b
> >  	struct bio_vec bvec;
> >  	struct bvec_iter iter;
> >  
> > +	if (bio_op(bio) == REQ_OP_DISCARD) {
> > +		sector_t len = bio_sectors(bio);
> > +		sector_t front_pad = -sector & (PAGE_SECTORS - 1);
> > +		sector += front_pad;
> > +		if (unlikely(len <= front_pad))
> > +			goto endio;
> > +		len -= front_pad;
> > +		len = round_down(len, PAGE_SECTORS);
> > +		while (len) {
> > +			brd_free_page(brd, sector);
> > +			sector += PAGE_SECTORS;
> > +			len -= PAGE_SECTORS;
> > +			cond_resched();
> > +		}
> > +		goto endio;
> > +	}
> > +
> >  	bio_for_each_segment(bvec, bio, iter) {
> 
> Please add separate helpers to each type of IO and just make the
> main submit_bio method a dispatch on the types instead of this
> spaghetti code.
> 
> > +	disk->queue->limits.discard_granularity = PAGE_SIZE;
> > +	blk_queue_max_discard_sectors(disk->queue, UINT_MAX);
> 
> We'll probably want an opt in for this new feature.

OK. I addressed these concerns and I'll send a second version of the patch 
set.

Mikulas


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-09-20 17:47 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-16  8:58 [PATCH 0/4] brd: implement discard Mikulas Patocka
2022-09-16  8:58 ` [dm-devel] " Mikulas Patocka
2022-09-16  8:59 ` [PATCH 1/4] brd: make brd_insert_page return bool Mikulas Patocka
2022-09-16  8:59   ` [dm-devel] " Mikulas Patocka
2022-09-20  7:28   ` Christoph Hellwig
2022-09-20  7:28     ` [dm-devel] " Christoph Hellwig
2022-09-16  8:59 ` [PATCH 2/4] brd: extend the rcu regions to cover read and write Mikulas Patocka
2022-09-16  8:59   ` [dm-devel] " Mikulas Patocka
2022-09-20  7:38   ` Christoph Hellwig
2022-09-20  7:38     ` [dm-devel] " Christoph Hellwig
2022-09-16  9:00 ` [dm-devel] [PATCH 3/4] brd: enable discard Mikulas Patocka
2022-09-16  9:00   ` Mikulas Patocka
2022-09-20  7:39   ` [dm-devel] " Christoph Hellwig
2022-09-20  7:39     ` Christoph Hellwig
2022-09-20 17:47     ` [dm-devel] " Mikulas Patocka
2022-09-20 17:47       ` Mikulas Patocka
2022-09-16  9:00 ` [dm-devel] [PATCH 4/4] brd: implement secure erase and write zeroes Mikulas Patocka
2022-09-16  9:00   ` Mikulas Patocka
2022-09-20  7:29   ` Christoph Hellwig
2022-09-20  7:29     ` [dm-devel] " Christoph Hellwig
2022-09-20 17:46     ` Mikulas Patocka
2022-09-20 17:46       ` Mikulas Patocka

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.