Re: [PATCH 2/4] um: Clean-up command processing in UML UBD driver

[Jens Axboe <axboe@kernel.dk>]

On 11/13/18 4:59 AM, anton.ivanov@cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> 
> Clean-up command processing and return BLK_STS_NOTSUP for
> uknown commands.
> 
> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> ---
>  arch/um/drivers/ubd_kern.c | 64 ++++++++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index 331837f1f632..0f02373ef632 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -1307,21 +1307,25 @@ static int ubd_queue_one_vec(struct blk_mq_hw_ctx *hctx, struct request *req,
>  		io_req->fds[0] = dev->fd;
>  	io_req->error = 0;
>  
> -	if (req_op(req) != REQ_OP_FLUSH) {
> -		io_req->fds[1] = dev->fd;
> -		io_req->cow_offset = -1;
> -		io_req->offset = off;
> +	if (bvec != NULL) {
>  		io_req->length = bvec->bv_len;
> -		io_req->sector_mask = 0;
> -		io_req->offsets[0] = 0;
> -		io_req->offsets[1] = dev->cow.data_offset;
>  		io_req->buffer = page_address(bvec->bv_page) + bvec->bv_offset;
> -		io_req->sectorsize = 1 << 9;
> +	} else {
> +		io_req->buffer = NULL;
> +		io_req->length = blk_rq_bytes(req);
> +	}
>  
> -		if (dev->cow.file) {
> -			cowify_req(io_req, dev->cow.bitmap,
> -				   dev->cow.bitmap_offset, dev->cow.bitmap_len);
> -		}
> +	io_req->sectorsize = UBD_SECTOR_SIZE;
> +	io_req->fds[1] = dev->fd;
> +	io_req->cow_offset = -1;
> +	io_req->offset = off;
> +	io_req->sector_mask = 0;
> +	io_req->offsets[0] = 0;
> +	io_req->offsets[1] = dev->cow.data_offset;
> +
> +	if (dev->cow.file) {
> +		cowify_req(io_req, dev->cow.bitmap,
> +			   dev->cow.bitmap_offset, dev->cow.bitmap_len);
>  	}
>  
>  	ret = os_write_file(thread_fd, &io_req, sizeof(io_req));
> @@ -1330,7 +1334,6 @@ static int ubd_queue_one_vec(struct blk_mq_hw_ctx *hctx, struct request *req,
>  			pr_err("write to io thread failed: %d\n", -ret);
>  		kfree(io_req);
>  	}
> -
>  	return ret;
>  }
>  
> @@ -1344,20 +1347,31 @@ static blk_status_t ubd_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	blk_mq_start_request(req);
>  
>  	spin_lock_irq(&ubd_dev->lock);
> -
> -	if (req_op(req) == REQ_OP_FLUSH) {
> +	switch (req_op(req)) {
> +	/* operations with no lentgth/offset arguments */
> +	case REQ_OP_FLUSH:
>  		ret = ubd_queue_one_vec(hctx, req, 0, NULL);
> -	} else {
> -		struct req_iterator iter;
> -		struct bio_vec bvec;
> -		u64 off = (u64)blk_rq_pos(req) << 9;
> -
> -		rq_for_each_segment(bvec, req, iter) {
> -			ret = ubd_queue_one_vec(hctx, req, off, &bvec);
> -			if (ret < 0)
> -				goto out;
> -			off += bvec.bv_len;
> +		break;
> +	/* operations with bio_vec arguments */
> +	case REQ_OP_READ:
> +	case REQ_OP_WRITE:
> +		{
> +			struct req_iterator iter;
> +			struct bio_vec bvec;
> +			u64 off = (u64)blk_rq_pos(req) << 9;
> +
> +			rq_for_each_segment(bvec, req, iter) {
> +				ret = ubd_queue_one_vec(hctx, req, off, &bvec);
> +				if (ret < 0)
> +					goto out;
> +				off += bvec.bv_len;
> +			}
>  		}
> +		break;

This indentation is wrong/awkward. The usual style is:

	case REQ_OP_READ:
	case REQ_OP_WRITE: {
		struct req_iterator iter;
		struct bio_vec bvec;
		u64 off = (u64)blk_rq_pos(req) << 9;

		rq_for_each_segment(bvec, req, iter) {
			ret = ubd_queue_one_vec(hctx, req, off, &bvec);
			if (ret < 0)
				goto out;
			off += bvec.bv_len;
		}
		break;
		}

Apart from that, looks good to me.

-- 
Jens Axboe


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um