[dm-devel] [PATCH 3/3] brd: implement write zeroes

[Mikulas Patocka]

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