On Mon, Oct 28, 2019 at 2:09 PM Chengguang Xu <cgxu519@xxxxxxxxxxxx> wrote:
>
> ---- 在 星期日, 2019-10-27 21:59:36 Amir Goldstein <amir73il@xxxxxxxxx> 撰写 ----
> > On Fri, Oct 25, 2019 at 4:19 AM Chengguang Xu <cgxu519@xxxxxxxxxxxx> wrote:
> > >
> > > ---- 在 星期五, 2019-10-25 05:02:07 Amir Goldstein <amir73il@xxxxxxxxx> 撰写 ----
> > > > On Thu, Oct 24, 2019 at 3:29 PM Chengguang Xu <cgxu519@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > This is intensive copy-up test for sparse files,
> > > > > these cases will be mainly used for regression test
> > > > > of copy-up improvement for sparse files.
> > > > >
> > > > > Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > > > Signed-off-by: Chengguang Xu <cgxu519@xxxxxxxxxxxx>
> > > > >
> > > > > ---
> > > > > v1->v2:
> > > > > - Call _get_block_size to get fs block size.
> > > > > - Add comment for test space requirement.
> > > > > - Print meaningful error message when copy-up fail.
> > > > > - Adjust random hole range to 1M~5M.
> > > > > - Fix typo.
> > > > >
> > > > > v2->v3:
> > > > > - Fix space requiremnt for test.
> > > > > - Add more descriptions for test files and hole patterns.
> > > > > - Define well named variables to replace unexplained numbers.
> > > > > - Fix random hole algorithm to what Amir suggested.
> > > > > - Adjust iosize to start from 1K.
> >
> > Chengguang,
> >
> > Sorry, I did't notice that you did that. Why?
> > As you can see below, this change has a very bad impact on test run time.
> > Any reason not to use _get_block_size?
>
> Use _get_block_size cannot mitigate the effect perfectly,
> in the worst case that we formatted fs with blocksize=1K,
> the test will take long time and also test time is not fixed.
>
> >
> >
> > > > > - Remove from quick test group.
> > > >
> > > > Why? you said it takes 7s without the kernel patch.
> > > > The test overlay/001 is in quick group and it copies up 2*4GB
> > > > sparse files.
> > >
> > > I noticed that after changed to start from 1K iosize the test took about 23s.
> > > I'm afraid maybe it will take more time on low performance VM env.
> > >
> > > The test overlay/001 took 8s/1s with/without kernel patch, so mainly test time
> > > wasted on creating test files on test overlay/066.
> >
> > You are correct about the time spent on creating the files, but...
> >
> > On my low perf VM, the test runs 95s with overlay over xfs+reflink
> >
> > But if I set start iosize=4 (which what my fs block size is) the test
> > runs only 30s.
> >
> > IOW, most of the test time is spent on creating the files with small iosize
> > below fs block size, which doesn't test copy up of holes at all.
> >
> > If I further change file size to be a multiply of iosize (x10),
> > test run time drops to 6s!
> > I don't think we loose too much test coverage if we do that?
> > If anything we gain testing different file sizes.
>
> hmm, for small iosize the file size is even smaller than
> copy-up CHUNK SIZE(1M), so that all contents(data+hole)
> will be passed at once, I'm not very sure is it helpful for
> hole copy-up logic in kernel patch. What do you think?
>
Not sure. I don't think we should target the test by what we know your
patch does, but by maximizing test coverage in a cost effective way.
Creating a 10M file with so many small holes doesn't add much to test
coverage IMO. If you feel those are needed, you should use a C helper
to create those files more efficiently.
BTW I think what is missing from test coverage is small holes
that are not aligned to 1M boundary.
>
> >
> > The disk space requirement formula for ${iosize}K_holefiles becomes:
> > 10*(2^0 + 2^11)K*12/2 =~ 10 * 1024 * 12
>
> That's the mean of 12/2 ?
it's the formula to the sum of the series:
2^0+2^2+...2^11 = (2^0 + 2^11)*12/2
>
> > same as before, just needs explaining.
> > (the formula assumes the worst case of min_iosize=1)
> >
> > -------------
> > #
> > # |-- hole --|-- data --| ... |-- data --|-- hole --|
> >
> > -iosize=1
> > +min_iosize=$(($(_get_block_size "${lowerdir}") / 1024 ))
> > +iosize=$min_iosize
> > max_iosize=2048
> > -file_size=10240
> > -max_pos=`expr $file_size - $max_iosize`
> >
> > while [ $iosize -le $max_iosize ]; do
> > + file_size=$((10*$iosize))
> > + max_pos=`expr $file_size - $iosize`
> > + date >>$seqres.full
>
> That's the purpose for putting data info here?
leftover from my debugging patch to figure out what takes so much time
You don't need it
Thanks,
Amir.
