Re: [PATCH 0/6 RFC] utilize bio_clone_fast to clean up

[Liu Bo <bo.li.liu@oracle.com>]

On Tue, May 09, 2017 at 03:49:13PM -0700, Liu Bo wrote:
> On Fri, May 05, 2017 at 04:24:47PM +0200, David Sterba wrote:
> > On Mon, Apr 17, 2017 at 06:16:21PM -0700, Liu Bo wrote:
> > > This attempts to use bio_clone_fast() in the places where we clone bio,
> > > such as when bio got cloned for multiple disks and when bio got split
> > > during dio submit.
> > > 
> > > One benefit is to simplify dio submit to avoid calling bio_add_page one by
> > > one.
> > > 
> > > Another benefit is that comparing to bio_clone_bioset, bio_clone_fast is
> > > faster because of copying the vector pointer directly, and bio_clone_fast
> > > doesn't modify bi_vcnt, so the extra work is to fix up bi_vcnt usage we
> > > currently have to use bi_iter to iterate bvec.
> > > 
> > > Liu Bo (6):
> > >   Btrfs: use bio_clone_fast to clone our bio
> > 
> > Please extend the changelog of this patch, use the text in the cover
> > letter.
> >
> 
> OK.
> 
> > >   Btrfs: use bio_clone_bioset_partial to simplify DIO submit
> > 
> > This patch is too big, can you split it to smaller chunks? I was not
> > able to review it, it seems to touch several things at once, it's hard
> > to keep the context.
> >
> 
> Oh I see, the diff does look scary but the changes are in fact not
> intrusive, I'll try to do something.
> 
> > >   Btrfs: change how we iterate bios in endio
> > >   Btrfs: record error if one block has failed to retry
> > >   Btrfs: change check-integrity to use bvec_iter
> > >   Btrfs: unify naming of btrfs_io_bio
> > 
> > The rest looks ok.
> > 
> > Have you done perofrmance tests? Not that it's necessary, but would be
> > interesting to see the effects. The effects of simplified code are
> > likely unmeasurable, but the _fast version skips some mempool exercises
> > so this could lead to improvements under memory pressure. And these is
> > hardly deterministic conditions, could be hard. I'me expecting some
> > latency improvemtnest.
> 
> I haven't done the perf. test since it is a RFC that I basically hope
> to check whether the idea makes sense.
> 
> And yes, using bio_clone_fas could save us some memory which is
> allocated for bio->bi_io_vec if (nr_iovecs > inline_vecs).

A quick test[1] showed a slightly better result, I ran the test 5 times each
round and took the average value,

vanilla 4.11-rc5: 15.39s
patched 4.11-rc5: 14.12s

[1]
M=/mnt/btrfs

D1=/dev/pmem0p1
D2=/dev/pmem0p2

umount $M
mkfs.btrfs -f $D1 $D2 >/dev/null || exit

mount $D1 $M -onodatasum || exit

xfs_io -f -c "falloc 0 2G" $M/foo

time xfs_io -d -c "pwrite -b 128K 0 2G" $M/foo


Thanks,

-liubo