qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 00/16] QEMU vhost-scsi support


From: Zhi Yong Wu
Subject: Re: [Qemu-devel] [PATCH 00/16] QEMU vhost-scsi support
Date: Thu, 19 Apr 2012 16:19:19 +0800

On Thu, Apr 19, 2012 at 3:16 PM, Paolo Bonzini <address@hidden> wrote:
> Il 19/04/2012 04:38, address@hidden ha scritto:
>> From: Zhi Yong Wu <address@hidden>
>>
>> The patchset was developed originally by Stefan about one year ago. I
>> now rebase it to latest qemu.git/master and fixed some issues to make
>> it work against tcm_vhost and virtio_scsi driver. But there are still
>> some issues to fix. Let us make more effort later.
>
> Zhi Yong, I'll warn you in advance that this message is going to be a
> bit harsh.
thanks for you reminder.
>
> You need to understand that once people have patches that work, that can
> be only a minor part of the work for getting them ready for submission.
>  This is especially true of experienced people like Stefan.  It is quite
> common to work briefly on something to build a proof of concept, and
> then pick up the work again months later.
>
> Taking patches from other people and completing them can be much harder
> than working on new code, because you need to get up to speed with
> incomplete code and that is difficult.  You need to understand where the
> work was left and what comes next.  You also need to ask the right
> person to the guy who did the work, Stefan in this case.  Anything else
> will lead to frustration for you and everyone involved, and delay the
> project indefinitely.
>
> To put it very frankly I don't know where to start listing mistakes in
> this series.  For example:
>
> 1) The series is missing any information on how you configure and use
> vhost-scsi.  Or on how you build it for that matter.
OK, i will send out how to build & configure in next version if
everything is good enough.
>
> 2) The series uses vhost-scsi unconditionally, as far as I can tell.  So
Perhaps there're some cleanup work to do, but it use vhost-scsi
*conditionally*. In virtio_scsi_init(), you can see the code as below:
    if (s->vhost_scsi) {
        s->vdev.set_status = virtio_scsi_set_status;
    }
As you have seen, virtio_scsi_set_status will be used to trigger
vhost_scsi function.
> it breaks command-lines that build scsi-{disk,block} devices for use
> with a virtio-scsi-pci device.
>
> Even if it actually worked, you need to mention what happens if you
> configure SCSI devices on the same virtio-scsi driver that uses vhost.
OK, i will do this in next version.

>
> 3) The cover letter is missing any performance numbers or examples of
> why vhost-scsi is preferable to scsi-{disk,block}.  I know some of them,
> but it needs to be documented elsewhere.
Sorry, i will do more effort about this later.
>
> Documentation is also needed on how/whether migration works with
> vhost-scsi, and between the QEMU and kernel targets (I know the answer
> to the latter is "it doesn't"; I don't know about the former.
ditto
>
> 4) The series is not properly split, for example a patch like 4/16 needs
> to be squashed above.
ah, i thought that i can carefully split them only before this
patchset are basically accepted.
>
> 5) The series is not up-to-date, for example CONFIG_VIRTIO_SCSI does not
> exist anymore.
Let me check this.
>
> 6) You sent this series without even an RFC tag in the subject.
OK, will add it in next version.
>
> 7) You sent this series, but you didn't even reply on target-devel to my
> review of the kernel-side changes.
Sorry, recently i indeed am busy. i will reply it soon.
>
> 8) Last but not least, I count two pending patch series from you (NetDev
> QOM conversion and network hubs) that you dropped on the list without
Observe so carefully.:), Yeah, but i have dropped them, and Stefan
usually pushed me do them and i am working on them.:)
In fact, the NetDev QOM depends on some of your "push, push" patchset,
i hoped to work on it after your patchset is merged into UPSTREAM. But
everything is changing, so i have to work on it now.
> hardly following up to comments that were made.  So this gives me very
> little incentive to look at your series.
sorry, please don't discourage, these tasks have to be done by me this
year. Very thinks for your comments, and they let me get a lot of
useful skills.
>
> To put it very frankly, I seriously think that you need either more
> discipline, or very good mentoring to contribute complex patch series to
> QEMU.  The list can offer mentoring, but you must expect criticism like
> this one (believe me, this is nothing compared to what you get on the
> linux kernel mailing list).
:), welcome.
>
> I strongly suggest that you work on one series at a time, for example
> the network hub conversion seems to be the most promising.
OK, thanks.
>
> Paolo



-- 
Regards,

Zhi Yong Wu



reply via email to

[Prev in Thread] Current Thread [Next in Thread]