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

[Pavel Tikhomirov]

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.

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.

> 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 =)

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? =)

> 
> 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.

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.