Re: [PATCH 2/2] xfs: use block layer helper for rw

[Dave Chinner <david@fromorbit.com>]

On Tue, Apr 07, 2020 at 08:06:35PM +0000, Chaitanya Kulkarni wrote:
> On 04/06/2020 05:32 PM, Damien Le Moal wrote:
> >> -
> >> >-	do {
> >> >-		struct page	*page = kmem_to_page(data);
> >> >-		unsigned int	off = offset_in_page(data);
> >> >-		unsigned int	len = min_t(unsigned, left, PAGE_SIZE - off);
> >> >-
> >> >-		while (bio_add_page(bio, page, len, off) != len) {
> >> >-			struct bio	*prev = bio;
> >> >-
> >> >-			bio = bio_alloc(GFP_KERNEL, bio_max_vecs(left));
> >> >-			bio_copy_dev(bio, prev);
> >> >-			bio->bi_iter.bi_sector = bio_end_sector(prev);
> >> >-			bio->bi_opf = prev->bi_opf;
> >> >-			bio_chain(prev, bio);
> >> >-
> >> >-			submit_bio(prev);
> >> >-		}
> >> >-
> >> >-		data += len;
> >> >-		left -= len;
> >> >-	} while (left > 0);
> > Your helper could use a similar loop structure. This is very easy to read.
> >
> If I understand correctly this pattern is used since it is not a part of 
> block layer.

It's because it was simple and easy to understandi, not because of
the fact it is outside the core block layer.

Us XFS folks are simple people who like simple things that are easy to
understand because there is so much of XFS that is so horrifically
complex that we want to implement simple stuff once and just not
have to care about it again.

> The helpers in blk-lib.c are not accessible so this :-

So export the helpers?

> All above breaks the existing pattern and code reuse in blk-lib.c, since 
> blk-lib.c:-
> 1. Already provides blk_next_bio() why repeat the bio allocation
>     and bio chaining code ?

So export the helper?

> 2. Instead of adding a new helper bio_max_vecs() why not use existing
>      __blkdev_sectors_to_bio_pages() ?

That's not an improvement. The XFS code is _much_ easier to read
and understand.

> 3. Why use two bios when it can be done with one bio with the helpers
>     in blk-lib.c ?

That helper is blk_next_bio(), which hides the second bio inside it.
So you aren't actually getting rid of the need for two bio pointers.

> 4. Allows async version.

Which is not used by XFS, so just adds complexity to this XFS path
for no good reason.

Seriously, this 20 lines of code in XFS turns into 50-60 lines
of code in the block layer to do the same thing. How is that an
improvement?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com