Re: [PATCH] brd: implement discard support

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

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