From: "Darrick J. Wong" <darrick.wong@oracle.com> To: Goldwyn Rodrigues <rgoldwyn@suse.de> Cc: kilobyte@angband.pl, jack@suse.cz, linux-nvdimm@lists.01.org, nborisov@suse.com, david@fromorbit.com, dsterba@suse.cz, willy@infradead.org, linux-fsdevel@vger.kernel.org, hch@lst.de, linux-btrfs@vger.kernel.org Subject: Re: [PATCH 08/18] dax: memcpy page in case of IOMAP_DAX_COW for mmap faults Date: Wed, 22 May 2019 21:02:02 -0700 [thread overview] Message-ID: <20190523040202.GH5125@magnolia> (raw) In-Reply-To: <20190522191139.62v2rgby5ptjhzcd@fiona> On Wed, May 22, 2019 at 02:11:39PM -0500, Goldwyn Rodrigues wrote: > On 10:46 21/05, Darrick J. Wong wrote: > > On Mon, Apr 29, 2019 at 12:26:39PM -0500, Goldwyn Rodrigues wrote: > > > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > > > > Change dax_iomap_pfn to return the address as well in order to > > > use it for performing a memcpy in case the type is IOMAP_DAX_COW. > > > We don't handle PMD because btrfs does not support hugepages. > > > > > > Question: > > > The sequence of bdev_dax_pgoff() and dax_direct_access() is > > > used multiple times to calculate address and pfn's. Would it make > > > sense to call it while calculating address as well to reduce code? > > > > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > --- > > > fs/dax.c | 19 +++++++++++++++---- > > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/dax.c b/fs/dax.c > > > index 610bfa861a28..718b1632a39d 100644 > > > --- a/fs/dax.c > > > +++ b/fs/dax.c > > > @@ -984,7 +984,7 @@ static sector_t dax_iomap_sector(struct iomap *iomap, loff_t pos) > > > } > > > > > > static int dax_iomap_pfn(struct iomap *iomap, loff_t pos, size_t size, > > > - pfn_t *pfnp) > > > + pfn_t *pfnp, void **addr) > > > { > > > const sector_t sector = dax_iomap_sector(iomap, pos); > > > pgoff_t pgoff; > > > @@ -996,7 +996,7 @@ static int dax_iomap_pfn(struct iomap *iomap, loff_t pos, size_t size, > > > return rc; > > > id = dax_read_lock(); > > > length = dax_direct_access(iomap->dax_dev, pgoff, PHYS_PFN(size), > > > - NULL, pfnp); > > > + addr, pfnp); > > > if (length < 0) { > > > rc = length; > > > goto out; > > > @@ -1286,6 +1286,7 @@ static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp, > > > XA_STATE(xas, &mapping->i_pages, vmf->pgoff); > > > struct inode *inode = mapping->host; > > > unsigned long vaddr = vmf->address; > > > + void *addr; > > > loff_t pos = (loff_t)vmf->pgoff << PAGE_SHIFT; > > > struct iomap iomap = { 0 }; > > > > Ugh, I had forgotten that fs/dax.c open-codes iomap_apply, probably > > because the actor returns vm_fault_t, not bytes copied. I guess that > > makes it a tiny bit more complicated to pass in two (struct iomap *) to > > the iomap_begin function... > > I am not sure I understand this. We do not use iomap_apply() in > the fault path: dax_iomap_pte_fault(). We just use iomap_begin() > and iomap_end(). So, why can we not implement your idea of using two > iomaps? Oh, sorry, I wasn't trying to say that calling ->iomap_begin made it *impossible* to implement. I was merely complaining about the increased maintenance burden that results from open coding -- now there are three places where we have to change a struct iomap declaration, not one (iomap_apply) as I had originally thought. > What does open-coding iomap-apply mean? Any function that calls (1) ->iomap_begin; (2) performs an action on the returned iomap; and (3) then calls calling ->iomap_end. That's what iomap_apply() does. Really I'm just being maintainer-cranky. Ignore me for now. :) --D > > -- > Goldwyn _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
WARNING: multiple messages have this Message-ID (diff)
From: "Darrick J. Wong" <darrick.wong@oracle.com> To: Goldwyn Rodrigues <rgoldwyn@suse.de> Cc: linux-btrfs@vger.kernel.org, kilobyte@angband.pl, linux-fsdevel@vger.kernel.org, jack@suse.cz, david@fromorbit.com, willy@infradead.org, hch@lst.de, dsterba@suse.cz, nborisov@suse.com, linux-nvdimm@lists.01.org Subject: Re: [PATCH 08/18] dax: memcpy page in case of IOMAP_DAX_COW for mmap faults Date: Wed, 22 May 2019 21:02:02 -0700 [thread overview] Message-ID: <20190523040202.GH5125@magnolia> (raw) In-Reply-To: <20190522191139.62v2rgby5ptjhzcd@fiona> On Wed, May 22, 2019 at 02:11:39PM -0500, Goldwyn Rodrigues wrote: > On 10:46 21/05, Darrick J. Wong wrote: > > On Mon, Apr 29, 2019 at 12:26:39PM -0500, Goldwyn Rodrigues wrote: > > > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > > > > Change dax_iomap_pfn to return the address as well in order to > > > use it for performing a memcpy in case the type is IOMAP_DAX_COW. > > > We don't handle PMD because btrfs does not support hugepages. > > > > > > Question: > > > The sequence of bdev_dax_pgoff() and dax_direct_access() is > > > used multiple times to calculate address and pfn's. Would it make > > > sense to call it while calculating address as well to reduce code? > > > > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > --- > > > fs/dax.c | 19 +++++++++++++++---- > > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/dax.c b/fs/dax.c > > > index 610bfa861a28..718b1632a39d 100644 > > > --- a/fs/dax.c > > > +++ b/fs/dax.c > > > @@ -984,7 +984,7 @@ static sector_t dax_iomap_sector(struct iomap *iomap, loff_t pos) > > > } > > > > > > static int dax_iomap_pfn(struct iomap *iomap, loff_t pos, size_t size, > > > - pfn_t *pfnp) > > > + pfn_t *pfnp, void **addr) > > > { > > > const sector_t sector = dax_iomap_sector(iomap, pos); > > > pgoff_t pgoff; > > > @@ -996,7 +996,7 @@ static int dax_iomap_pfn(struct iomap *iomap, loff_t pos, size_t size, > > > return rc; > > > id = dax_read_lock(); > > > length = dax_direct_access(iomap->dax_dev, pgoff, PHYS_PFN(size), > > > - NULL, pfnp); > > > + addr, pfnp); > > > if (length < 0) { > > > rc = length; > > > goto out; > > > @@ -1286,6 +1286,7 @@ static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp, > > > XA_STATE(xas, &mapping->i_pages, vmf->pgoff); > > > struct inode *inode = mapping->host; > > > unsigned long vaddr = vmf->address; > > > + void *addr; > > > loff_t pos = (loff_t)vmf->pgoff << PAGE_SHIFT; > > > struct iomap iomap = { 0 }; > > > > Ugh, I had forgotten that fs/dax.c open-codes iomap_apply, probably > > because the actor returns vm_fault_t, not bytes copied. I guess that > > makes it a tiny bit more complicated to pass in two (struct iomap *) to > > the iomap_begin function... > > I am not sure I understand this. We do not use iomap_apply() in > the fault path: dax_iomap_pte_fault(). We just use iomap_begin() > and iomap_end(). So, why can we not implement your idea of using two > iomaps? Oh, sorry, I wasn't trying to say that calling ->iomap_begin made it *impossible* to implement. I was merely complaining about the increased maintenance burden that results from open coding -- now there are three places where we have to change a struct iomap declaration, not one (iomap_apply) as I had originally thought. > What does open-coding iomap-apply mean? Any function that calls (1) ->iomap_begin; (2) performs an action on the returned iomap; and (3) then calls calling ->iomap_end. That's what iomap_apply() does. Really I'm just being maintainer-cranky. Ignore me for now. :) --D > > -- > Goldwyn
next prev parent reply other threads:[~2019-05-23 4:02 UTC|newest] Thread overview: 113+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-29 17:26 [PATCH v4 00/18] btrfs dax support Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-04-29 17:26 ` [PATCH 01/18] btrfs: create a mount option for dax Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-05-21 18:02 ` Darrick J. Wong 2019-05-21 18:02 ` Darrick J. Wong 2019-04-29 17:26 ` [PATCH 02/18] btrfs: Carve out btrfs_get_extent_map_write() out of btrfs_get_blocks_write() Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-04-29 17:26 ` [PATCH 04/18] dax: Introduce IOMAP_DAX_COW to CoW edges during writes Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-05-21 16:51 ` Darrick J. Wong 2019-05-22 20:14 ` Goldwyn Rodrigues 2019-05-22 20:14 ` Goldwyn Rodrigues 2019-05-23 2:10 ` Dave Chinner 2019-05-23 2:10 ` Dave Chinner 2019-05-23 9:05 ` Shiyang Ruan 2019-05-23 9:05 ` Shiyang Ruan 2019-05-23 11:51 ` Goldwyn Rodrigues 2019-05-23 11:51 ` Goldwyn Rodrigues 2019-05-27 8:25 ` Shiyang Ruan 2019-05-27 8:25 ` Shiyang Ruan 2019-05-28 9:17 ` Jan Kara 2019-05-28 9:17 ` Jan Kara 2019-05-29 2:01 ` Shiyang Ruan 2019-05-29 2:01 ` Shiyang Ruan 2019-05-29 2:47 ` Dave Chinner 2019-05-29 2:47 ` Dave Chinner 2019-05-29 4:02 ` Shiyang Ruan 2019-05-29 4:02 ` Shiyang Ruan 2019-05-29 4:07 ` Darrick J. Wong 2019-05-29 4:07 ` Darrick J. Wong 2019-05-29 4:46 ` Dave Chinner 2019-05-29 4:46 ` Dave Chinner 2019-05-29 13:46 ` Jan Kara 2019-05-29 13:46 ` Jan Kara 2019-05-29 22:14 ` Dave Chinner 2019-05-29 22:14 ` Dave Chinner 2019-05-30 11:16 ` Jan Kara 2019-05-30 11:16 ` Jan Kara 2019-05-30 22:59 ` Dave Chinner 2019-05-30 22:59 ` Dave Chinner 2019-04-29 17:26 ` [PATCH 05/18] btrfs: return whether extent is nocow or not Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-04-29 17:26 ` [PATCH 06/18] btrfs: Rename __endio_write_update_ordered() to btrfs_update_ordered_extent() Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues [not found] ` <20190429172649.8288-1-rgoldwyn-l3A5Bk7waGM@public.gmane.org> 2019-04-29 17:26 ` [PATCH 03/18] btrfs: basic dax read Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues [not found] ` <20190429172649.8288-4-rgoldwyn-l3A5Bk7waGM@public.gmane.org> 2019-05-21 15:14 ` Darrick J. Wong 2019-05-21 15:14 ` Darrick J. Wong 2019-05-22 21:50 ` Goldwyn Rodrigues 2019-04-29 17:26 ` [PATCH 07/18] btrfs: add dax write support Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues [not found] ` <20190429172649.8288-8-rgoldwyn-l3A5Bk7waGM@public.gmane.org> 2019-05-21 17:08 ` Darrick J. Wong 2019-05-21 17:08 ` Darrick J. Wong 2019-04-29 17:26 ` [PATCH 13/18] fs: dedup file range to use a compare function Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues [not found] ` <20190429172649.8288-14-rgoldwyn-l3A5Bk7waGM@public.gmane.org> 2019-05-21 18:17 ` Darrick J. Wong 2019-05-21 18:17 ` Darrick J. Wong 2019-04-29 17:26 ` [PATCH 18/18] btrfs: trace functions for btrfs_iomap_begin/end Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-04-29 17:26 ` [PATCH 08/18] dax: memcpy page in case of IOMAP_DAX_COW for mmap faults Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-05-21 17:46 ` Darrick J. Wong 2019-05-21 17:46 ` Darrick J. Wong 2019-05-22 19:11 ` Goldwyn Rodrigues 2019-05-22 19:11 ` Goldwyn Rodrigues 2019-05-23 4:02 ` Darrick J. Wong [this message] 2019-05-23 4:02 ` Darrick J. Wong 2019-05-23 12:10 ` Jan Kara 2019-05-23 12:10 ` Jan Kara 2019-04-29 17:26 ` [PATCH 09/18] btrfs: Add dax specific address_space_operations Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-04-29 17:26 ` [PATCH 10/18] dax: replace mmap entry in case of CoW Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-05-21 17:35 ` Darrick J. Wong 2019-05-21 17:35 ` Darrick J. Wong 2019-05-23 13:38 ` Jan Kara 2019-05-23 13:38 ` Jan Kara 2019-04-29 17:26 ` [PATCH 11/18] btrfs: add dax mmap support Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-04-29 17:26 ` [PATCH 12/18] btrfs: allow MAP_SYNC mmap Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-05-10 15:32 ` [PATCH for-goldwyn] btrfs: disallow MAP_SYNC outside of DAX mounts Adam Borowski 2019-05-10 15:32 ` Adam Borowski 2019-05-10 15:41 ` Dan Williams 2019-05-10 15:41 ` Dan Williams 2019-05-10 15:59 ` Pankaj Gupta 2019-05-10 15:59 ` Pankaj Gupta 2019-05-23 13:44 ` [PATCH 12/18] btrfs: allow MAP_SYNC mmap Jan Kara 2019-05-23 13:44 ` Jan Kara 2019-05-23 16:19 ` Adam Borowski 2019-05-23 16:19 ` Adam Borowski 2019-04-29 17:26 ` [PATCH 14/18] dax: memcpy before zeroing range Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-05-21 17:27 ` Darrick J. Wong 2019-05-21 17:27 ` Darrick J. Wong 2019-04-29 17:26 ` [PATCH 15/18] btrfs: handle dax page zeroing Goldwyn Rodrigues 2019-04-29 17:26 ` [PATCH 16/18] btrfs: Writeprotect mmap pages on snapshot Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues 2019-05-23 14:04 ` Jan Kara 2019-05-23 14:04 ` Jan Kara 2019-05-23 15:27 ` Goldwyn Rodrigues 2019-05-23 15:27 ` Goldwyn Rodrigues 2019-05-23 19:07 ` Jan Kara 2019-05-23 19:07 ` Jan Kara 2019-05-23 21:22 ` Goldwyn Rodrigues 2019-05-23 21:22 ` Goldwyn Rodrigues 2019-04-29 17:26 ` [PATCH 17/18] btrfs: Disable dax-based defrag and send Goldwyn Rodrigues 2019-04-29 17:26 ` Goldwyn Rodrigues -- strict thread matches above, loose matches on Subject: below -- 2019-04-16 16:41 [PATCH v3 00/18] btrfs dax support Goldwyn Rodrigues 2019-04-16 16:41 ` [PATCH 08/18] dax: memcpy page in case of IOMAP_DAX_COW for mmap faults Goldwyn Rodrigues 2019-04-16 16:41 ` Goldwyn Rodrigues [not found] ` <20190416164154.30390-9-rgoldwyn-l3A5Bk7waGM@public.gmane.org> 2019-04-17 16:52 ` Darrick J. Wong 2019-04-17 16:52 ` Darrick J. Wong
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190523040202.GH5125@magnolia \ --to=darrick.wong@oracle.com \ --cc=david@fromorbit.com \ --cc=dsterba@suse.cz \ --cc=hch@lst.de \ --cc=jack@suse.cz \ --cc=kilobyte@angband.pl \ --cc=linux-btrfs@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-nvdimm@lists.01.org \ --cc=nborisov@suse.com \ --cc=rgoldwyn@suse.de \ --cc=willy@infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.