On 19 June 2018 at 08:42, Christoph Hellwig <hch@xxxxxx> wrote:
>> } 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.
On gfs2, direct I/O uses a "deferred lock", which is a special kind of
shared lock. We can probably do inline reads under it, but not inline
writes, so falling back to buffered I/O seems inevitable in that case
at least.
>> + /* 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);
Andreas
