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

[Asias He <asias@redhat.com>]

Hello Christoph!

On 10/10/2012 01:39 AM, Christoph Hellwig wrote:
>> +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.

vhost_blk_req_submit() can be used to handle file based image later.
Using the file interface will work for both cases.

I do need a check in vhost_blk_set_backend() to tell if the user passed
file is a raw device file for now.

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

No, vhost_blk_req_submit() is used for both read and write ops. It makes
no sense to write the code twice. Plus, this function might be complexer
when the file based image support is added.

>> +	set_fs(USER_DS);
> 
> What do we actually need the set_fs for here?

See this commit: d7ffde35e31a81100d2d0d2c4013cbf527bb32ea

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

It is used in both vhost_blk_reset_owner() and vhost_blk_release(). The
returned struct file is used for fput(). We have similar helper in
vhost_net: vhost_net_stop().


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

Will fix this.

Thanks for the review!

-- 
Asias