Dutch Meyer schrieb:
>> Why would you want to keep the old version around if the new one is so
>> much better?
>
> We're very open to removing the old version, but there is a backwards
> compatability concern. See below.
Right, I was afraid this would be your answer. I'm certainly not opposed
to improve blktap by a rewrite, but I do have concerns that you're not
going the right way with this series - not implementation-wise, but from
a design perspective.
You're saying that removing tap:ioemu is a compatibility issue. That's
not quite the point. The user won't lose functionality (in the current
upstream implementation of tap:ioemu), he could use tap:aio or tap:qcow2
or whatever instead.
The big evil about what we currently have is code duplication. For each
image format we support we have two implementations: One in ioemu and
one in tapdisk. This is why it was agreed that we want to get rid of the
tapdisk implementations and move everything into ioemu (the other way
round wouldn't work because of HVM).
Now instead of doing the final step and making tap:ioemu the default so
that only one implementation is left, you're trying to add even the
third implementation. This is clearly not what you want from a design
and maintainability perspective.
So what I'm saying is that while I'm not opposed to a rewrite in
principle, the rewrite needs to be a complete drop-in replacement to
avoid this third copy of the code. Ideally the rewrite would be
completely integrated into qemu, but at least not having a third copy
and making things even worse is a must, IMHO.
One more thought, even if it gets lengthy now... If you really want to
stay with the two implementations, what about at least changing the
driver's interface to be more similar to qemu's? This certainly would
help with merging upstream changes. Ideally you would take qemu's code,
throw it into the blktap directory and you're done.
>> Now this is a huge patch, nearly impossible to review. At a first glance
>> I noticed that it shares large parts with the old implementation. So
>> while I believe you that the first patch is only moving things around, I
>> really can't tell what this patch is actually doing.
>
> I think relative to the changes, there isn't that much that is shared
> with the old implementation. Taking a diff against the two directories
> will show that the core of blktap has been redone, much of it from
> scratch. That's why we did it this way.
>
> You can diff versus the two blktap directories to see this. If you
> still think this would be very helpful, I can provide this diff. The
> drivers are, at their core, unchanged so perhaps separate patches would
> make sense in that case.
Separating the drivers out could help a lot, I think. Would be great if
you could do this.
>> I didn't really look into the patch but merely scrolled through it and
>> looked at the diffstat, so maybe I'm wrong, but it seems to lack
>> implementations for tap:qcow2, tap:vmdk and tap:ioemu. Why that?
>
> You're right about that - we decided to post this without qcow2, but can
> port that one as well. VMDK is slow, it's written to be purely
> synchronous, and we don't think anyone actually uses it.
>
> ioemu is a different matter - This is a very difficult piece of code to
> work with, because large portions of blktap have been replicated to make
> it work. On top of that, the wire protocol changed to support this.
> Porting this driver would be quite difficult. This ioemu code is the
> primary reason we provided backwards compatability with what is now
> "blktap_old".
I've already explained above why from a design perspective I don't think
ioemu is a compatibility thing, but actually the right next step.
For the implementation, I don't think tap:ioemu is a particularly
difficult piece of code. In ioemu, it shares a few lines with tapdisk.c
which is bad, but tapdisk was meant to disappear anyway when tap:ioemu
is completed. I guess we can easily replace them with your new code,
that should be no problem. (Haven't looked yet at the new code, though)
The other thing that was changed is blktapctrl which simply opens a
different pipe when it uses ioemu instead of tapdisk. The wire protocol
has not been changed to achieve this (why do you think so?). This
shouldn't be too hard to implement either.
> I really appreciate the conversation here, and look forward to keeping
> it going, but i may be away from email for the weekend, so please excuse
> any slowness in my reply.
I was having a weekend myself, so that's no problem. ;-)
If you agree that the points I mentioned are issues and they need to be
fixed, I'm willing to help you with it as time permits. I know that my
suggestions actually mean quite a bit of reworking your patches, but I
think it may be worth it.
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home