[Devel] [PATCH RH9 01/10] drivers/vhost: vhost-blk accelerator for virtio-blk guests

[Andrey Zhadchenko]

On 9/7/22 18:08, Pavel Tikhomirov wrote:
> 
> 
> On 07.09.2022 16:23, Andrey Zhadchenko wrote:
>> This will surely divert attention from several parts of the patch itself.
> 
> For someone probably it can divert an attention. But for me missing this 
> information only unnecessarily complicates the review. I prefer have all 
> the information possible in commit message.

That is why I included links to old series and copyrights, so anyone can 
trace the origin if they think it is useful for them.
As for "changes in vX", I am not sure it is useful in general in this 
case. It is expected to help people who are already somewhat familiar 
with the code, like previous reviewers. And most of the lines are rather 
statements then explanations.
Also I would say in reality the code is based on vhost/test.c with 
additions from Asias version rather than modified Asias version :)

> 
> If I were you I would also add:
> 
> Changes in v6:
> - Use inline req_page_list to reduce kmalloc
> - Switch to single thread model, thanks mst!
> - Wait until requests fired before vhost_blk_flush to be finished
> 
> Changes in v5:
> - Do not assume the buffer layout
> - Fix wakeup race
> 
> Changes in v4:
> - Mark req->status as userspace pointer
> - Use __copy_to_user() instead of copy_to_user() in vhost_blk_set_status()
> - Add if (need_resched()) schedule() in blk thread
> - Kill vhost_blk_stop_vq() and move it into vhost_blk_stop()
> - Use vq_err() instead of pr_warn()
> - Fail un Unsupported request
> - Add flush in vhost_blk_set_features()
> 
> Changes in v3:
> - Sending REQ_FLUSH bio instead of vfs_fsync, thanks Christoph!
> - Check file passed by user is a raw block device file
> 
> As this patch is a result of complex review in mainstream mailing list.

Sure it is. However things changed since. Some helpers were deleted, 
some concepts were reworked (for example bio had a page limit, which 
later was scrapped).
Although it was reviewed, some code still have flaws. For example the 
oldfile was also leaked in the previous patches. I tried to fix it but 
failed miserably :) (and with the addition of multiple vqs backend 
handling is even worse!)

> 
>> I know it is simplier to review this patch like an addition for old 
>> Asias version, but I remind that original was never merged and tested 
>> and may contain any amount of bugs and problems.
>> Please review my patch as a single entity so we may spot more problems.
> 
> You talk here like I didn't =)

I have no idea, that is my impression from several "why it is changed 
unlike in Asias version" questions. Sorry for being wrong here!

> 
> But the code is the code. And explanation what the code does is 
> separate, reviewer only can confirm that explanation correlates with the 
> code nothing more.
> 
> If there is no explanation almost any code is right, probably you 
> intentionally leak oldfile in the patch. How would I know if it is 
> intentional or not if there is no commit message explanation of what 
> code does? =)

I get your point. But it is a very hard (and holywarish) take. It is 
simple (and a must) to document 10-50-100-line change within a commit 
message, especially if it is a specific change or bugfix. But for a big 
new module? Probably not, it would take an immense amount of time and 
probably worthless. And also this code use a lot of helpers from vhost 
which should also be probably explained?
Some people may not read an explanation at all, some people may prefer 
to understand it from reading code, and for some people it may be very 
useful. And there is no guarantee that all potential description would 
be clear and concise. For a reference I personally prefer to explore the 
code rather read some explanations.
And the answer for "how would I know" is, in my opinion, a questions. I 
would be glad to answer!

> 
>>
>> On 9/5/22 18:38, Pavel Tikhomirov wrote:
>>> General comment: it would be nice to have changes relative to "Asias' 
>>> version: https://lkml.org/lkml/2012/12/1/174" listed in commit 
>>> message, so that we not only see the resulting patch, but also have 
>>> idea why it was reworked this way.
>