Re: [Qemu-devel] [PATCH for 2.7 1/1] qcow2: improve qcow2_co_write_zeroes()

[Denis V. Lunev]

On 04/27/2016 11:12 AM, Kevin Wolf wrote:
> Am 27.04.2016 um 09:07 hat Denis V. Lunev geschrieben:
> > On 04/26/2016 01:19 PM, Kevin Wolf wrote:
> >
> > [skipped]
> > > Did you ever talk to the kernel people?
> > >
> > > We can try to make the best out of suboptimal requests in qemu, but it
> > > looks to me as if the real fix was in the kernel, and if we don't get it
> > > fixed there, we'll see more and more of this kind of problems. I think
> > > this is relevant not only for VMs, but probably on real hardware as
> > > well.
> > it is difficult and could be done partially only :(
> Was there a discussion on some public mailing list? If so, do you have a
> pointer?
we have discussed this here a year ago but I am unable to see traces in the
LKML so far. It seems that the situation was resolved and patience
was lost. Though there were some efforts...

Here is the thread with Paolo and Dmitry Monakhov,
who is working with kernel block layer in Virtuozzo.

http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg00182.html


> > > Here is the trick that I think will save us:
> > >
> > > On a misaligned call, we call bdrv_get_block_status_above() for the
> > > whole cluster that we're in. We know that it's only a single cluster
> > > because bdrv_co_do_write_zeroes() splits things this way; only aligned
> > > requests can be longer than a sector (we can even assert this).
> > >
> > > If the result is that the cluster already reads as zero, instead of
> > > doing nothing and possibly breaking backing chain manipulations, we
> > > simply extend the write zeroes operation to the whole cluster and
> > > continue as normal with an aligned request. This way we end up with a
> > > zero cluster instead of an unallocated one, and that should be safe.
> > >
> > > If the result is that the cluster isn't completed zeroed, return
> > > -ENOTSUP as you did in the snippet above.
> > >
> > > That approach should probably result in an (even) simpler patch, too.
> > let's start from the simple thing. I'll send a patch in a couple of minutes.
> > It really looks MUCH better than the original one. Thank you for this cool
> > suggestion.
> >
> > > Hm... Or actually, if we want something more complex that will help all
> > > block drivers, extending the range of the request could even be done in
> > > bdrv_co_do_write_zeroes(), I guess. I won't insist on it, though.
> > I do not think that this should be done now. This patch should go into
> > our own stable and thus it would better be as simple as possible.
> As I said, I'm okay with the simple patch for now, but I must point out
> that "your own stable" is an invalid argument here.
>
> What we merge in upstream qemu must make sense for upstream qemu, and
> for nothing else, even if it may be more inconvenient for downstreams
> and if it may mean that downstreams have to carry non-upstream patches.
> I have rejected patches upstream even though they made more sense for
> RHEL, and I will reject patches that suit other downstreams if they
> aren't doing the right thing for the upstream project.
>
> So on qemu-devel, as a rule of thumb always argue with the merits that a
> patch has for upstream, not with advantages for downstream.
ok, but I have not used this as an argument. You have said that "simple 
patch is OK",
the purpose of my clause was to note "simple patch is better for me at 
the moment".
Sorry, if this has wrong intention.

Den