| Message ID | 20181113115947.19290-3-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] um: Switch to block-mq constants in the UML UBD driver | expand |
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.
On Tue, Nov 13, 2018 at 06:34:31AM -0700, Jens Axboe wrote: > > + /* 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: And for extra point just split the code into a separate helper, avoiding the whole switch scoping issue entirely.
On 14/11/2018 15:28, Christoph Hellwig wrote: > On Tue, Nov 13, 2018 at 06:34:31AM -0700, Jens Axboe wrote: >>> + /* 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: > And for extra point just split the code into a separate helper, > avoiding the whole switch scoping issue entirely. > I thought about it :) And forgot as I was fighting with the origin of the crashes at the time. Do you want me to reissue the series or I can do an incremental?
On Wed, Nov 14, 2018 at 03:31:01PM +0000, Anton Ivanov wrote: > I thought about it :) And forgot as I was fighting with the origin of the > crashes at the time. > > Do you want me to reissue the series or I can do an incremental? Based on the other comments I had a resend might be worthwhile.
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; + default: + WARN_ON_ONCE(1); + spin_unlock_irq(&ubd_dev->lock); + return BLK_STS_NOTSUPP; } out: spin_unlock_irq(&ubd_dev->lock);