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