Re: [PATCH] vhost-blk: Add vhost-blk support v2

[Christoph Hellwig <hch@infradead.org>]

> +static int vhost_blk_req_submit(struct vhost_blk_req *req, struct file *file)
> +{
> +
> +	struct inode *inode = file->f_mapping->host;
> +	struct block_device *bdev = inode->i_bdev;
> +	int ret;

Please just pass the block_device directly instead of a file struct.

> +
> +	ret = vhost_blk_bio_make(req, bdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	vhost_blk_bio_send(req);
> +
> +	return ret;
> +}

Then again how simple the this function is it probably should just go
away entirely.

> +	set_fs(USER_DS);

What do we actually need the set_fs for here?

> +
> +static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
> +{
> +
> +	*file = vhost_blk_stop_vq(blk, &blk->vq);
> +}

What is the point of this helper?  Also I can't see anyone actually
using the returned struct file.

> +	case VIRTIO_BLK_T_FLUSH:
> +		ret = vfs_fsync(file, 1);
> +		status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> +		if (!vhost_blk_set_status(req, status))
> +			vhost_add_used_and_signal(&blk->dev, vq, head, ret);
> +		break;

Sending a fsync here is actually wrong in two different ways:

 a) it operates at the filesystem level instead of bio level
 b) it's a blocking operation

It should instead send a REQ_FLUSH bio using the same submission scheme
as the read/write requests.