On Thu 03-10-19 21:34:00, Matthew Bobrowski wrote:
> This patch introduces a new direct I/O read path that makes use of the
> iomap infrastructure.
>
> The new function ext4_dio_read_iter() is responsible for calling into
> the iomap infrastructure via iomap_dio_rw(). If the read operation
> being performed on the inode does not pass the preliminary checks
> performed within ext4_dio_supported(), then we simply fallback to
> buffered I/O in order to fulfil the request.
>
> Existing direct I/O read buffer_head code has been removed as it's now
> redundant.
>
> Signed-off-by: Matthew Bobrowski <mbobrowski@xxxxxxxxxxxxxx>
The patch looks good to me. Just one small nit below. With that fixed, you
can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
> + /*
> + * Get exclusion from truncate and other inode operations.
> + */
> + if (!inode_trylock_shared(inode)) {
> + if (iocb->ki_flags & IOCB_NOWAIT)
> + return -EAGAIN;
> + inode_lock_shared(inode);
> + }
I've noticed here you actually introduce new trylock pattern - previously
we had unconditional inode_lock_shared() in ext4_direct_IO_read(). So the
cleanest would be to just use unconditional inode_lock_shared() here and
then fixup IOCB_NOWAIT handling (I agree that was missing in the original
code) in a separate patch. And the pattern should rather look like:
if (iocb->ki_flags & IOCB_NOWAIT) {
if (!inode_trylock_shared(inode))
return -EAGAIN;
} else {
inode_lock_shared(inode);
}
to avoid two atomical operations instead of one in the fast path. No need
to repeat old mistakes when we know better :).
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
