Re: [PATCH v6 4/9] iomap: Generic inline data handling

["Andreas Grünbacher" <andreas.gruenbacher@gmail.com>]

2018-06-02 19:04 GMT+02:00 Christoph Hellwig <hch@lst.de>:
> On Sat, Jun 02, 2018 at 11:57:12AM +0200, Andreas Gruenbacher wrote:
>> Add generic inline data handling by adding a pointer to the inline data
>> region to struct iomap.  When handling a buffered IOMAP_INLINE write,
>> iomap_write_begin will copy the current inline data from the inline data
>> region into the page cache, and iomap_write_end will copy the changes in
>> the page cache back to the inline data region.
>
> This approach looks good.  A few comments below:
>
>>
>> This doesn't cover inline data reads and direct I/O yet because so far,
>> we have no users.
>
> I'm fine with that as a first step, but gfs2 should be able to do
> proper direct I/O to inline data and use by new iomap_readpage(s)
> easily at least for blocksize == PAGE_SIZE, so this should be added
> soon.

Yes, there are a few open ends in gfs's iomap support, but what we
have so far is already quite useful.

>> -int generic_write_end(struct file *file, struct address_space *mapping,
>> +void __generic_write_end(struct file *file, struct address_space *mapping,
>>                       loff_t pos, unsigned len, unsigned copied,
>> -                     struct page *page, void *fsdata)
>> +                     struct page *page, void *fsdata, bool dirty_inode)
>
> This is going to clash with
>
> http://git.infradead.org/users/hch/xfs.git/commitdiff/2733909d6b40046ce9c7302c2e742c5e993a0108
>
> It should also be a separate prep patch with a good explanation

I'll cherry-pick that commit for now.

>>  iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
>> -             unsigned copied, struct page *page)
>> +             unsigned copied, struct page *page, struct iomap *iomap)
>
> Note that I have another patch adding this parameter.  I think we'll need
> a common iomap base tree for the gfs2 and xfs changes for the next
> merge window.  I'd be happy to one one up.

No objections there, I don't think we'll end up with major conflicts though.

>> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
>> index 918f14075702..c61113c71a60 100644
>> --- a/include/linux/iomap.h
>> +++ b/include/linux/iomap.h
>> @@ -47,6 +47,7 @@ struct iomap {
>>       u64                     length; /* length of mapping, bytes */
>>       u16                     type;   /* type of mapping */
>>       u16                     flags;  /* flags for mapping */
>> +     void                    *inline_data;  /* inline data buffer */
>>       struct block_device     *bdev;  /* block device for I/O */
>>       struct dax_device       *dax_dev; /* dax_dev for dax operations */
>>  };
>
> Eventually we need to find a way to union the inline_data, bdev and
> dax_dev fields, but that can be left to later.

For gfs2, passing a private pointer from iomap_begin to iomap_end (for
the buffer head holding the inode) would help as well, but it's not
critical.

Thanks,
Andreas