Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1169887 > unrolled thread

Re: [PATCH v5 5/5] block: loop: support DIO & AIO

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 5/5] block: loop: support DIO & AIO Ming Lei <ming.lei@canonical.com> - 2015-06-22 14:20 +0200
    Re: [PATCH v5 5/5] block: loop: support DIO & AIO Ming Lei <ming.lei@canonical.com> - 2015-06-23 05:00 +0200

#1169887 — Re: [PATCH v5 5/5] block: loop: support DIO & AIO

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/

[toc] | [next] | [standalone]

#1170347

On Tue, Jun 23, 2015 at 12:00 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jun 22, 2015 at 08:09:55PM +0800, Ming Lei wrote:
>> 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...)
>
> We only really need iocb + iov_iter, they carry everything we need.

Then the helper becomes the fowllowing:

ssize_t vfs_iter_async_write(struct kiocb *kiocb, struct iov_iter *iter)
{
       ssize_t ret;

       iter->type |= WRITE;
       ret = file->f_op->write_iter(&kiocb, iter);

       return ret;
}

I am wondering its value and we can do that easily in call site, also it isn't
easy to name it since sometimes we may need to let the helper handle
sync requests, such as loop's case.

Thanks,
Ming
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web