Re: [PATCH v9 4/5] gfs2: iomap direct I/O support

[Christoph Hellwig <hch@xxxxxx>]

>  	} else if (flags & IOMAP_WRITE) {
>  		u64 size;
> +
> +		if (flags & IOMAP_DIRECT)
> +			goto out;
> +

Maybe add a comment here on why you don't allow block allocations for
direct I/O.

> +		if (flags & IOMAP_DIRECT) {
> +			ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp);
> +			release_metapath(&mp);
> +			if (iomap->type != IOMAP_MAPPED)
> +				ret = -ENOTBLK;
> +		} else {
> +			ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap);
> +		}

A couple too long lines.

>  	} else {
>  		ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp);
>  		release_metapath(&mp);

But shouldn't the direct I/O code try to reuse this part anyway?

E.g. something like:

	if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) == IOMAP_WRITE)) {
		ret = gfs2_iomap_begin_write(inode, pos, length, flags, iomap);
	} else {
 		ret = gfs2_iomap_get(inode, pos, length, flags, iomap, &mp);
  		release_metapath(&mp);
		if ((flags & IOMAP_WRITE) && iomap->type != IOMAP_MAPPED)
			ret = -ENOTBLK;

> +	/* fall back to buffered I/O for stuffed files */
> +	ret = -ENOTBLK;
> +	if (gfs2_is_stuffed(ip))
> +		goto out;

I think we can handle stuffed files in the direct I/O code trivially
by copying out the inline data in the iomap.  It would be great to
just handle this instead of adding fallbacks.

> +	/* Silently fall back to buffered I/O for stuffed files */
> +	if (gfs2_is_stuffed(ip))
> +		goto out;

Same here.

> +static ssize_t gfs2_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> +{
> +	ssize_t ret;
> +
> +	if (iocb->ki_flags & IOCB_DIRECT) {
> +		ret = gfs2_file_direct_read(iocb, to);
> +		if (likely(ret != -ENOTBLK))
> +			goto out;

			return ret;

> +		iocb->ki_flags &= ~IOCB_DIRECT;
> +	}
> +	ret = generic_file_read_iter(iocb, to);
> +out:
> +	return ret;

	return generic_file_read_iter(iocb, to);