From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:38343 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751473AbeFDMCL (ORCPT ); Mon, 4 Jun 2018 08:02:11 -0400 Received: by mail-wm0-f66.google.com with SMTP id m129-v6so14323786wmb.3 for ; Mon, 04 Jun 2018 05:02:11 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20180602170429.GC15847@lst.de> References: <20180602095717.31641-1-agruenba@redhat.com> <20180602095717.31641-5-agruenba@redhat.com> <20180602170429.GC15847@lst.de> From: =?UTF-8?Q?Andreas_Gr=C3=BCnbacher?= Date: Mon, 4 Jun 2018 14:02:10 +0200 Message-ID: Subject: Re: [PATCH v6 4/9] iomap: Generic inline data handling To: Christoph Hellwig Cc: Andreas Gruenbacher , cluster-devel , Linux FS-devel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 2018-06-02 19:04 GMT+02:00 Christoph Hellwig : > 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