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

[Andreas Gruenbacher <agruenba@redhat.com>]

On 19 June 2018 at 08:42, Christoph Hellwig <hch@lst.de> 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