Re: [PATCH 12/16] block: add single bio async direct IO helper

[Pavel Begunkov <asml.silence@gmail.com>]

On 10/20/21 07:36, Christoph Hellwig wrote:
> On Tue, Oct 19, 2021 at 10:24:21PM +0100, Pavel Begunkov wrote:
>> +	bio = bio_alloc_kiocb(iocb, nr_pages, &blkdev_dio_pool);
>> +	dio = container_of(bio, struct blkdev_dio, bio);
>> +	__bio_set_dev(bio, bdev);
>> +	bio->bi_iter.bi_sector = pos >> 9;
> 
> SECTOR_SHIFT.
> 
>> +	bio->bi_write_hint = iocb->ki_hint;
>> +	bio->bi_end_io = blkdev_bio_end_io_async;
>> +	bio->bi_ioprio = iocb->ki_ioprio;
>> +	dio->flags = 0;
>> +	dio->iocb = iocb;
>> +
>> +	ret = bio_iov_iter_get_pages(bio, iter);
>> +	if (unlikely(ret)) {
>> +		bio->bi_status = BLK_STS_IOERR;
>> +		bio_endio(bio);
>> +		return BLK_STS_IOERR;
> 
> This function does not return a blk_status_t, so this is wrong (and
> sparse should have complained).  I also don't think the error path
> here should go hrough the bio for error handling but just do a put and
> return the error.

My bad, following __blkdev_direct_IO() it was intended to be
blk_status_to_errno(BLK_STS_IOERR), but just return is much
better.

> 
>> +	if (iov_iter_rw(iter) == READ) {
>> +		bio->bi_opf = REQ_OP_READ;
>> +		if (iter_is_iovec(iter)) {
>> +			dio->flags |= DIO_SHOULD_DIRTY;
>> +			bio_set_pages_dirty(bio);
>> +		}
>> +	} else {
>> +		bio->bi_opf = dio_bio_write_op(iocb);
>> +		task_io_account_write(bio->bi_iter.bi_size);
>> +	}
>> +
>> +	if (iocb->ki_flags & IOCB_NOWAIT)
>> +		bio->bi_opf |= REQ_NOWAIT;
> 
> This code is entirely duplicated, pleae move it into an (inline) helper.

I'll try it out, thanks

>> +	/*
>> +	 * Don't plug for HIPRI/polled IO, as those should go straight
>> +	 * to issue
>> +	 */
> 
> This comment seems misplaced as the function does not use plugging at
> all.

will kill it


-- 
Pavel Begunkov