On Thu, Apr 19, 2012 at 4:19 PM, Zhi Yong Wu <zwu.ker...@gmail.com> wrote:
> On Thu, Apr 19, 2012 at 3:16 PM, Paolo Bonzini <pbonz...@redhat.com> wrote:
>> Il 19/04/2012 04:38, zwu.ker...@gmail.com ha scritto:
>>> From: Zhi Yong Wu <wu...@linux.vnet.ibm.com>
>>>
>>> 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
s/have/haven't/g
> 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
--
Regards,
Zhi Yong Wu
