diff mbox series

[3/3] brd: implement write zeroes

Message ID 73c46137-c06e-348f-3d37-8c305ad4c4f3@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Mike Snitzer
Headers show
Series brd discard patches | expand

Commit Message

Mikulas Patocka July 19, 2023, 8:21 p.m. UTC 
This patch implements 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.

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

---
 drivers/block/brd.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

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

Comments

Chaitanya Kulkarni July 20, 2023, 5:30 a.m. UTC | #1 
On 7/19/2023 1:21 PM, Mikulas Patocka wrote:
> This patch implements 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.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>   drivers/block/brd.c |   22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
> 
> Index: linux-2.6/drivers/block/brd.c
> ===================================================================
> --- linux-2.6.orig/drivers/block/brd.c
> +++ linux-2.6/drivers/block/brd.c
> @@ -272,7 +272,8 @@ out:
>   
>   void brd_do_discard(struct brd_device *brd, struct bio *bio)
>   {
> -	sector_t sector, len, front_pad;
> +	bool zero_padding = bio_op(bio) == REQ_OP_WRITE_ZEROES;
> +	sector_t sector, len, front_pad, end_pad;
>   
>   	if (unlikely(!discard)) {
>   		bio->bi_status = BLK_STS_NOTSUPP;
> @@ -282,11 +283,22 @@ void brd_do_discard(struct brd_device *b
>   	sector = bio->bi_iter.bi_sector;
>   	len = bio_sectors(bio);
>   	front_pad = -sector & (PAGE_SECTORS - 1);
> +
> +	if (zero_padding && unlikely(front_pad != 0))
> +		copy_to_brd(brd, page_address(ZERO_PAGE(0)),
> +			    sector, min(len, front_pad) << SECTOR_SHIFT);
> +

Is it possible to create different interface for discard and write
zeroes ? I think it is a lot of logic adding on one function if everyone
else is okay please discard my comment ..

>   	sector += front_pad;
>   	if (unlikely(len <= front_pad))
>   		return;
>   	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);
>   		sector += PAGE_SECTORS;
> @@ -302,7 +314,8 @@ 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_WRITE_ZEROES) {
>   		brd_do_discard(brd, bio);
>   		goto endio;
>   	}

can we please use switch ? unless there is a strong need for if
which I failed to understand ...

> @@ -355,7 +368,7 @@ MODULE_PARM_DESC(max_part, "Num Minors t
>   
>   static bool discard = false;
>   module_param(discard, bool, 0444);
> -MODULE_PARM_DESC(discard, "Support discard");
> +MODULE_PARM_DESC(discard, "Support discard and write zeroes");
>  

write-zeroes and discards are both different req_op and they should have
a separate module parameter ...


>   MODULE_LICENSE("GPL");
>   MODULE_ALIAS_BLOCKDEV_MAJOR(RAMDISK_MAJOR);
> @@ -425,6 +438,7 @@ static int brd_alloc(int i)
>   	if (discard) {
>   		disk->queue->limits.discard_granularity = PAGE_SIZE;

>   		blk_queue_max_discard_sectors(disk->queue, round_down(UINT_MAX, PAGE_SECTORS));
> +		blk_queue_max_write_zeroes_sectors(disk->queue, round_down(UINT_MAX, PAGE_SECTORS));

above should be moved to new module parameter write_zeroes, unless there
is a reason to use one module parameter for both req_op...

>   	}
>   
>   	/* Tell the block layer that this is not a rotational device */
> 

-ck


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Mikulas Patocka July 21, 2023, 1:46 p.m. UTC | #2 
On Thu, 20 Jul 2023, Chaitanya Kulkarni wrote:

> > Index: linux-2.6/drivers/block/brd.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/block/brd.c
> > +++ linux-2.6/drivers/block/brd.c
> > @@ -272,7 +272,8 @@ out:
> >   
> >   void brd_do_discard(struct brd_device *brd, struct bio *bio)
> >   {
> > -	sector_t sector, len, front_pad;
> > +	bool zero_padding = bio_op(bio) == REQ_OP_WRITE_ZEROES;
> > +	sector_t sector, len, front_pad, end_pad;
> >   
> >   	if (unlikely(!discard)) {
> >   		bio->bi_status = BLK_STS_NOTSUPP;
> > @@ -282,11 +283,22 @@ void brd_do_discard(struct brd_device *b
> >   	sector = bio->bi_iter.bi_sector;
> >   	len = bio_sectors(bio);
> >   	front_pad = -sector & (PAGE_SECTORS - 1);
> > +
> > +	if (zero_padding && unlikely(front_pad != 0))
> > +		copy_to_brd(brd, page_address(ZERO_PAGE(0)),
> > +			    sector, min(len, front_pad) << SECTOR_SHIFT);
> > +
> 
> Is it possible to create different interface for discard and write
> zeroes ? I think it is a lot of logic adding on one function if everyone
> else is okay please discard my comment ..

Copying code is anti-pattern - it increases code size and it makes it 
harder to modify code in the future.

Discard and write-zeroes perform almost the same operation, the only 
difference is that write-zeroes needs to zero trailing and leading parts 
of boundary pages.

Therefore I think that making one function that performs both discard and 
write zeroes is ok.

> > -	if (bio_op(bio) == REQ_OP_DISCARD) {
> > +	if (bio_op(bio) == REQ_OP_DISCARD ||
> > +	    bio_op(bio) == REQ_OP_WRITE_ZEROES) {
> >   		brd_do_discard(brd, bio);
> >   		goto endio;
> >   	}
> 
> can we please use switch ? unless there is a strong need for if
> which I failed to understand ...

Yes, I can do this change.

> > @@ -355,7 +368,7 @@ MODULE_PARM_DESC(max_part, "Num Minors t
> >   
> >   static bool discard = false;
> >   module_param(discard, bool, 0444);
> > -MODULE_PARM_DESC(discard, "Support discard");
> > +MODULE_PARM_DESC(discard, "Support discard and write zeroes");
> >  
> 
> write-zeroes and discards are both different req_op and they should have
> a separate module parameter ...
> 
> above should be moved to new module parameter write_zeroes, unless there
> is a reason to use one module parameter for both req_op...

Is there some reason why the user might want discard and not want 
write-zeroes or vice versa?

What do Christoph and Jens think? Do you think that there should be two 
separate parameters too?

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

Patch

Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c
+++ linux-2.6/drivers/block/brd.c
@@ -272,7 +272,8 @@  out:
 
 void brd_do_discard(struct brd_device *brd, struct bio *bio)
 {
-	sector_t sector, len, front_pad;
+	bool zero_padding = bio_op(bio) == REQ_OP_WRITE_ZEROES;
+	sector_t sector, len, front_pad, end_pad;
 
 	if (unlikely(!discard)) {
 		bio->bi_status = BLK_STS_NOTSUPP;
@@ -282,11 +283,22 @@  void brd_do_discard(struct brd_device *b
 	sector = bio->bi_iter.bi_sector;
 	len = bio_sectors(bio);
 	front_pad = -sector & (PAGE_SECTORS - 1);
+
+	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))
 		return;
 	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);
 		sector += PAGE_SECTORS;
@@ -302,7 +314,8 @@  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_WRITE_ZEROES) {
 		brd_do_discard(brd, bio);
 		goto endio;
 	}
@@ -355,7 +368,7 @@  MODULE_PARM_DESC(max_part, "Num Minors t
 
 static bool discard = false;
 module_param(discard, bool, 0444);
-MODULE_PARM_DESC(discard, "Support discard");
+MODULE_PARM_DESC(discard, "Support discard and write zeroes");
 
 MODULE_LICENSE("GPL");
 MODULE_ALIAS_BLOCKDEV_MAJOR(RAMDISK_MAJOR);
@@ -425,6 +438,7 @@  static int brd_alloc(int i)
 	if (discard) {
 		disk->queue->limits.discard_granularity = PAGE_SIZE;
 		blk_queue_max_discard_sectors(disk->queue, round_down(UINT_MAX, PAGE_SECTORS));
+		blk_queue_max_write_zeroes_sectors(disk->queue, round_down(UINT_MAX, PAGE_SECTORS));
 	}
 
 	/* Tell the block layer that this is not a rotational device */