Re: [PATCH for-next v8 3/5] nvme: refactor nvme_alloc_user_request

[Christoph Hellwig <hch@lst.de>]

> -static void *nvme_add_user_metadata(struct bio *bio, void __user *ubuf,
> -		unsigned len, u32 seed, bool write)
> +static void *nvme_add_user_metadata(struct request *req, void __user *ubuf,
> +		unsigned len, u32 seed)

Please split out a separate patch just for the changes to
nvme_add_user_metadata.

> -	if (ret == len)
> +	if (ret == len) {
> +		req->cmd_flags |= REQ_INTEGRITY;
>  		return buf;
> +	}
>  	ret = -ENOMEM;

Nit: Please keep the successful path in line and branch out for the
errors, i.e.


	if (ret != len) {
	 	ret = -ENOMEM;
		goto out_free_meta;
	}

	req->cmd_flags |= REQ_INTEGRITY;
	return buf;

We should have done this already with the old code, but now that more
is added to the success path it becomes even more important:.

> +	else {
> +		struct iovec fast_iov[UIO_FASTIOV];
> +		struct iovec *iov = fast_iov;
> +		struct iov_iter iter;
> +
> +		ret = import_iovec(rq_data_dir(req), ubuffer, bufflen,
> +				UIO_FASTIOV, &iov, &iter);
> +		if (ret < 0)
>  			goto out;
> +		ret = blk_rq_map_user_iov(q, req, NULL, &iter, GFP_KERNEL);
> +		kfree(iov);
> +	}

While you touch this:  I think thi block of code would also be a good
separate helper.  Maybe even in the block layer given the the scsi
ioctl code and sg duplicate it, and already missed the fast_iov
treatment due to the duplication.  Having this in a separate function
is also nice to keep the fast_iov stack footprint isolated.

> +	if (ret)
> +		goto out;
> +	bio = req->bio;

I think we can also do away with this bio local variable now.

> +	if (bdev)
> +		bio_set_dev(bio, bdev);

We don't need the bio_set_dev here as mentioned last time, so I think
we should remove it in a prep patch.

> +		ret = nvme_map_user_request(req, ubuffer, bufflen, meta_buffer,
> +			meta_len, meta_seed, &meta, vec);

Nit: Add an extra tab for indentation here please.

>  	if (IS_ERR(req))
>  		return PTR_ERR(req);
> +	req->timeout = d.timeout_ms ? msecs_to_jiffies(d.timeout_ms) : 0;

	if (d.timeout_ms)
		req->timeout = msecs_to_jiffies(d.timeout_ms);