[PATCH 0/4] brd: implement discard

[PATCH 0/4] brd: implement discard

[Mikulas Patocka]

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] [PATCH 0/4] brd: implement discard

[Mikulas Patocka]

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

[PATCH 1/4] brd: make brd_insert_page return bool

[Mikulas Patocka]

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] [PATCH 1/4] brd: make brd_insert_page return bool

[Mikulas Patocka]

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

Re: [PATCH 1/4] brd: make brd_insert_page return bool

[Christoph Hellwig]

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>

Re: [dm-devel] [PATCH 1/4] brd: make brd_insert_page return bool

[Christoph Hellwig]

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

[PATCH 2/4] brd: extend the rcu regions to cover read and write

[Mikulas Patocka]

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] [PATCH 2/4] brd: extend the rcu regions to cover read and write

[Mikulas Patocka]

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

Re: [PATCH 2/4] brd: extend the rcu regions to cover read and write

[Christoph Hellwig]

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

Re: [dm-devel] [PATCH 2/4] brd: extend the rcu regions to cover read and write

[Christoph Hellwig]

>   * 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

[dm-devel] [PATCH 3/4] brd: enable discard

[Mikulas Patocka]

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

[PATCH 3/4] brd: enable discard

[Mikulas Patocka]

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

Re: [dm-devel] [PATCH 3/4] brd: enable discard

[Christoph Hellwig]

> @@ -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

Re: [PATCH 3/4] brd: enable discard

[Christoph Hellwig]

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

Re: [dm-devel] [PATCH 3/4] brd: enable discard

[Mikulas Patocka]

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

Re: [PATCH 3/4] brd: enable discard

[Mikulas Patocka]

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] [PATCH 4/4] brd: implement secure erase and write zeroes

[Mikulas Patocka]

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

[PATCH 4/4] brd: implement secure erase and write zeroes

[Mikulas Patocka]

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

Re: [PATCH 4/4] brd: implement secure erase and write zeroes

[Christoph Hellwig]

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.

Re: [dm-devel] [PATCH 4/4] brd: implement secure erase and write zeroes

[Christoph Hellwig]

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

Re: [dm-devel] [PATCH 4/4] brd: implement secure erase and write zeroes

[Mikulas Patocka]

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

Re: [dm-devel] [PATCH 4/4] brd: implement secure erase and write zeroes

[Mikulas Patocka]

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