[PATCH] brd: implement discard support

[PATCH] brd: implement discard support

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The ramdisk memory utilization can only go up when data is written to
new pages. Implement discard to provide the possibility to reduce memory
usage for pages no longer in use. Aligned discards will free the
associated pages, if any, and determinisitically return zeroed data
until written again.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/block/brd.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index e322cef6596bf..e741b0c3a4f79 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -240,6 +240,23 @@ static int brd_do_bvec(struct brd_device *brd, struct page *page,
 	return err;
 }
 
+static void brd_do_discard(struct brd_device *brd, sector_t sector, u32 size)
+{
+	sector_t aligned_sector = (sector + PAGE_SECTORS) & ~PAGE_SECTORS;
+	struct page *page;
+
+	size -= (aligned_sector - sector) * SECTOR_SIZE;
+	xa_lock(&brd->brd_pages);
+	while (size >= PAGE_SIZE && aligned_sector < rd_size * 2) {
+		page = __xa_erase(&brd->brd_pages, aligned_sector >> PAGE_SECTORS_SHIFT);
+		if (page)
+			__free_page(page);
+		aligned_sector += PAGE_SECTORS;
+		size -= PAGE_SIZE;
+	}
+	xa_unlock(&brd->brd_pages);
+}
+
 static void brd_submit_bio(struct bio *bio)
 {
 	struct brd_device *brd = bio->bi_bdev->bd_disk->private_data;
@@ -247,6 +264,12 @@ static void brd_submit_bio(struct bio *bio)
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 
+	if (unlikely(op_is_discard(bio->bi_opf))) {
+		brd_do_discard(brd, sector, bio->bi_iter.bi_size);
+		bio_endio(bio);
+		return;
+	}
+
 	bio_for_each_segment(bvec, bio, iter) {
 		unsigned int len = bvec.bv_len;
 		int err;
@@ -327,6 +350,9 @@ static int brd_alloc(int i)
 		 *  is harmless)
 		 */
 		.physical_block_size	= PAGE_SIZE,
+		.max_hw_discard_sectors	= UINT_MAX,
+		.max_discard_segments	= 1,
+		.discard_granularity	= PAGE_SIZE,
 	};
 
 	list_for_each_entry(brd, &brd_devices, brd_list)
-- 
2.43.0

Re: [PATCH] brd: implement discard support

[Chaitanya Kulkarni]

On 4/29/24 03:23, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The ramdisk memory utilization can only go up when data is written to
> new pages. Implement discard to provide the possibility to reduce memory
> usage for pages no longer in use. Aligned discards will free the
> associated pages, if any, and determinisitically return zeroed data
> until written again.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/block/brd.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>
> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> index e322cef6596bf..e741b0c3a4f79 100644
> --- a/drivers/block/brd.c
> +++ b/drivers/block/brd.c
> @@ -240,6 +240,23 @@ static int brd_do_bvec(struct brd_device *brd, struct page *page,
>   	return err;
>   }
>   
> +static void brd_do_discard(struct brd_device *brd, sector_t sector, u32 size)
> +{
> +	sector_t aligned_sector = (sector + PAGE_SECTORS) & ~PAGE_SECTORS;
> +	struct page *page;
> +
> +	size -= (aligned_sector - sector) * SECTOR_SIZE;
> +	xa_lock(&brd->brd_pages);
> +	while (size >= PAGE_SIZE && aligned_sector < rd_size * 2) {
> +		page = __xa_erase(&brd->brd_pages, aligned_sector >> PAGE_SECTORS_SHIFT);

overly long line ?

> +		if (page)
> +			__free_page(page);
> +		aligned_sector += PAGE_SECTORS;
> +		size -= PAGE_SIZE;
> +	}
> +	xa_unlock(&brd->brd_pages);
> +}
> +
>   static void brd_submit_bio(struct bio *bio)
>   {
>   	struct brd_device *brd = bio->bi_bdev->bd_disk->private_data;
> @@ -247,6 +264,12 @@ static void brd_submit_bio(struct bio *bio)
>   	struct bio_vec bvec;
>   	struct bvec_iter iter;
>   
> +	if (unlikely(op_is_discard(bio->bi_opf))) {

I've been told that unlikely should not be used with discard as it is
bad for discard workloads, if that is still true, then can you please
remove unlikely ?

Also, if you are doing this can you please also add support for
write-zeroes for the sake of completeness ? unless that support is
not desired for brd ...

-ck

Re: [PATCH] brd: implement discard support

[Keith Busch]

On Mon, Apr 29, 2024 at 07:54:05PM +0000, Chaitanya Kulkarni wrote:
> On 4/29/24 03:23, Keith Busch wrote:
> >   static void brd_submit_bio(struct bio *bio)
> >   {
> >   	struct brd_device *brd = bio->bi_bdev->bd_disk->private_data;
> > @@ -247,6 +264,12 @@ static void brd_submit_bio(struct bio *bio)
> >   	struct bio_vec bvec;
> >   	struct bvec_iter iter;
> >   
> > +	if (unlikely(op_is_discard(bio->bi_opf))) {
> 
> I've been told that unlikely should not be used with discard as it is
> bad for discard workloads, if that is still true, then can you please
> remove unlikely ?

I don't think discard workloads are likely on a ramdisk.

> Also, if you are doing this can you please also add support for
> write-zeroes for the sake of completeness ? unless that support is
> not desired for brd ...

That is orthoganal to the goals here. I just need to temporarily reclaim
memory for other purposes when the disk is temporarily not being use; I
don't care about the zero'ing out part.

I found that previous attempts at supporting discard on brd didn't make
it, supposedly because of some kind of writeback deadlock. I have no
idea what that comment way back from 2.6.35 is referring to though: we
allocate pages with NOIO or NOWAIT, so either brd can get a new page
when its needed or it fails, and either is fine, but deadlock shouldn't
happen.

Re: [PATCH] brd: implement discard support

[Jens Axboe]

On Mon, 29 Apr 2024 03:23:08 -0700, Keith Busch wrote:
> The ramdisk memory utilization can only go up when data is written to
> new pages. Implement discard to provide the possibility to reduce memory
> usage for pages no longer in use. Aligned discards will free the
> associated pages, if any, and determinisitically return zeroed data
> until written again.
> 
> 
> [...]

Applied, thanks!

[1/1] brd: implement discard support
      commit: 9ead7efc6f3f2b46c4ec68209bca4888cfbd4c19

Best regards,
-- 
Jens Axboe