Messages in this thread | | | Date | Mon, 22 Jun 2015 20:09:55 +0800 | Subject | Re: [PATCH v5 5/5] block: loop: support DIO & AIO | From | Ming Lei <> |
| |
On Wed, Jun 10, 2015 at 3:46 PM, Christoph Hellwig <hch@infradead.org> wrote: >> + int ret; >> + >> + /* nomerge for loop request queue */ >> + WARN_ON(cmd->rq->bio != cmd->rq->biotail); >> + >> + bvec = __bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter); >> + iov_iter_bvec(&iter, ITER_BVEC | rw, bvec, >> + bio_segments(bio), blk_rq_bytes(cmd->rq)); >> + >> + cmd->iocb.ki_pos = pos; >> + cmd->iocb.ki_filp = file; >> + cmd->iocb.ki_complete = lo_rw_aio_complete; >> + cmd->iocb.ki_flags = IOCB_DIRECT; >> + >> + if (rw == WRITE) >> + ret = file->f_op->write_iter(&cmd->iocb, &iter); >> + else >> + ret = file->f_op->read_iter(&cmd->iocb, &iter); > > I think we really need a vfs_ wrapper here similar to what I did a while > ago, e.g. vfs_iter_read/write_async.
For the general async interface, it is a bit complicated than sync interfaces:
- iocb need to be one parameter, because it often depends on callers, such as loop can preallocate it - direct I/O need to be another parameter(in loop we can use the same helper to handle sync request) - bvec and the segment number are another two parameters - not mention the common parameters(file, offset, pos, complete...)
And this kind of interfaces appeared in V1/V2, looks AIO guys doesn't care that, then I moved the helper into loop, and it becomes quite simple now. If we convert it to vfs_iter_read/write_async(), more source code are introduced, I think.
So how about considering that if there are other uses in the future?
> >> +static inline int lo_rw_simple(struct loop_device *lo, >> + struct request *rq, loff_t pos, bool rw) >> +{ >> + struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq); >> + >> + if (cmd->use_aio) >> + return lo_rw_aio(lo, cmd, pos, rw); >> + >> + if (rw == WRITE) >> + return lo_write_simple(lo, rq, pos); >> + else >> + return lo_read_simple(lo, rq, pos); >> +} > > And the io_submit style read/write also works for buffered I/O, so no > need to keep lo_write_simple/lo_read_simple around.
That is really a good idea.
> >> @@ -1569,7 +1634,8 @@ static void loop_handle_cmd(struct loop_cmd *cmd) >> failed: >> if (ret) >> cmd->rq->errors = -EIO; >> - blk_mq_complete_request(cmd->rq); >> + if (!cmd->use_aio || ret) >> + blk_mq_complete_request(cmd->rq); > > If you don't complete the request here setting req->error doesn't > make sense. I'd suggest to move the blk_mq_complete_request for
The request with ->erros set is really completed here, and the curent rule is very simple:
- complete sync/submit failed requests in loop_handle_cmd() - complete async requests submitted successfully in its .complete
> everything but the trivial error case into the actual I/O handlers > to clean this up a bit, too.
That need to copy the code for handling error in other handlers.
Thanks, Ming -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Please read the FAQ at http://www.tux.org/lkml/
| |