2018-06-02 19:04 GMT+02:00 Christoph Hellwig <hch@xxxxxx>: > 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
