[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [4 Patches] New blktap implementation, 2nd try
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |