From: Shiyang Ruan <ruansy.fnst@cn.fujitsu.com> To: Dave Chinner <david@fromorbit.com> Cc: kilobyte@angband.pl, Jan Kara <jack@suse.cz>, "Darrick J. Wong <darrick.wong@oracle.com>, nborisov@suse.com, Goldwyn Rodrigues" <rgoldwyn@suse.de>, linux-nvdimm@lists.01.org, dsterba@suse.cz, willy@infradead.org, linux-fsdevel@vger.kernel.org, hch@lst.de, linux-btrfs@vger.kernel.org Subject: Re: [PATCH 04/18] dax: Introduce IOMAP_DAX_COW to CoW edges during writes Date: Wed, 29 May 2019 12:02:40 +0800 [thread overview] Message-ID: <376256fd-dee4-5561-eb4e-546e227303cd@cn.fujitsu.com> (raw) In-Reply-To: <20190529024749.GC16786@dread.disaster.area> On 5/29/19 10:47 AM, Dave Chinner wrote: > On Wed, May 29, 2019 at 10:01:58AM +0800, Shiyang Ruan wrote: >> >> On 5/28/19 5:17 PM, Jan Kara wrote: >>> On Mon 27-05-19 16:25:41, Shiyang Ruan wrote: >>>> On 5/23/19 7:51 PM, Goldwyn Rodrigues wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> I'm working on reflink & dax in XFS, here are some thoughts on this: >>>>>> >>>>>> As mentioned above: the second iomap's offset and length must match the >>>>>> first. I thought so at the beginning, but later found that the only >>>>>> difference between these two iomaps is @addr. So, what about adding a >>>>>> @saddr, which means the source address of COW extent, into the struct iomap. >>>>>> The ->iomap_begin() fills @saddr if the extent is COW, and 0 if not. Then >>>>>> handle this @saddr in each ->actor(). No more modifications in other >>>>>> functions. >>>>> >>>>> Yes, I started of with the exact idea before being recommended this by Dave. >>>>> I used two fields instead of one namely cow_pos and cow_addr which defined >>>>> the source details. I had put it as a iomap flag as opposed to a type >>>>> which of course did not appeal well. >>>>> >>>>> We may want to use iomaps for cases where two inodes are involved. >>>>> An example of the other scenario where offset may be different is file >>>>> comparison for dedup: vfs_dedup_file_range_compare(). However, it would >>>>> need two inodes in iomap as well. >>>>> >>>> Yes, it is reasonable. Thanks for your explanation. >>>> >>>> One more thing RFC: >>>> I'd like to add an end-io callback argument in ->dax_iomap_actor() to update >>>> the metadata after one whole COW operation is completed. The end-io can >>>> also be called in ->iomap_end(). But one COW operation may call >>>> ->iomap_apply() many times, and so does the end-io. Thus, I think it would >>>> be nice to move it to the bottom of ->dax_iomap_actor(), called just once in >>>> each COW operation. >>> >>> I'm sorry but I don't follow what you suggest. One COW operation is a call >>> to dax_iomap_rw(), isn't it? That may call iomap_apply() several times, >>> each invocation calls ->iomap_begin(), ->actor() (dax_iomap_actor()), >>> ->iomap_end() once. So I don't see a difference between doing something in >>> ->actor() and ->iomap_end() (besides the passed arguments but that does not >>> seem to be your concern). So what do you exactly want to do? >> >> Hi Jan, >> >> Thanks for pointing out, and I'm sorry for my mistake. It's >> ->dax_iomap_rw(), not ->dax_iomap_actor(). >> >> I want to call the callback function at the end of ->dax_iomap_rw(). >> >> Like this: >> dax_iomap_rw(..., callback) { >> >> ... >> while (...) { >> iomap_apply(...); >> } >> >> if (callback != null) { >> callback(); >> } >> return ...; >> } > > Why does this need to be in dax_iomap_rw()? > > We already do post-dax_iomap_rw() "io-end callbacks" directly in > xfs_file_dax_write() to update the file size.... Yes, but we also need to call ->xfs_reflink_end_cow() after a COW operation. And an is-cow flag(from iomap) is also needed to determine if we call it. I think it would be better to put this into ->dax_iomap_rw() as a callback function. So sorry for my poor expression. > > Cheers, > > Dave. > -- Thanks, Shiyang Ruan. _______________________________________________ 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: Shiyang Ruan <ruansy.fnst@cn.fujitsu.com> To: Dave Chinner <david@fromorbit.com> Cc: Jan Kara <jack@suse.cz>, Goldwyn Rodrigues <rgoldwyn@suse.de>, "Darrick J. Wong" <darrick.wong@oracle.com>, <linux-btrfs@vger.kernel.org>, <kilobyte@angband.pl>, <linux-fsdevel@vger.kernel.org>, <willy@infradead.org>, <hch@lst.de>, <dsterba@suse.cz>, <nborisov@suse.com>, <linux-nvdimm@lists.01.org> Subject: Re: [PATCH 04/18] dax: Introduce IOMAP_DAX_COW to CoW edges during writes Date: Wed, 29 May 2019 12:02:40 +0800 [thread overview] Message-ID: <376256fd-dee4-5561-eb4e-546e227303cd@cn.fujitsu.com> (raw) In-Reply-To: <20190529024749.GC16786@dread.disaster.area> On 5/29/19 10:47 AM, Dave Chinner wrote: > On Wed, May 29, 2019 at 10:01:58AM +0800, Shiyang Ruan wrote: >> >> On 5/28/19 5:17 PM, Jan Kara wrote: >>> On Mon 27-05-19 16:25:41, Shiyang Ruan wrote: >>>> On 5/23/19 7:51 PM, Goldwyn Rodrigues wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> I'm working on reflink & dax in XFS, here are some thoughts on this: >>>>>> >>>>>> As mentioned above: the second iomap's offset and length must match the >>>>>> first. I thought so at the beginning, but later found that the only >>>>>> difference between these two iomaps is @addr. So, what about adding a >>>>>> @saddr, which means the source address of COW extent, into the struct iomap. >>>>>> The ->iomap_begin() fills @saddr if the extent is COW, and 0 if not. Then >>>>>> handle this @saddr in each ->actor(). No more modifications in other >>>>>> functions. >>>>> >>>>> Yes, I started of with the exact idea before being recommended this by Dave. >>>>> I used two fields instead of one namely cow_pos and cow_addr which defined >>>>> the source details. I had put it as a iomap flag as opposed to a type >>>>> which of course did not appeal well. >>>>> >>>>> We may want to use iomaps for cases where two inodes are involved. >>>>> An example of the other scenario where offset may be different is file >>>>> comparison for dedup: vfs_dedup_file_range_compare(). However, it would >>>>> need two inodes in iomap as well. >>>>> >>>> Yes, it is reasonable. Thanks for your explanation. >>>> >>>> One more thing RFC: >>>> I'd like to add an end-io callback argument in ->dax_iomap_actor() to update >>>> the metadata after one whole COW operation is completed. The end-io can >>>> also be called in ->iomap_end(). But one COW operation may call >>>> ->iomap_apply() many times, and so does the end-io. Thus, I think it would >>>> be nice to move it to the bottom of ->dax_iomap_actor(), called just once in >>>> each COW operation. >>> >>> I'm sorry but I don't follow what you suggest. One COW operation is a call >>> to dax_iomap_rw(), isn't it? That may call iomap_apply() several times, >>> each invocation calls ->iomap_begin(), ->actor() (dax_iomap_actor()), >>> ->iomap_end() once. So I don't see a difference between doing something in >>> ->actor() and ->iomap_end() (besides the passed arguments but that does not >>> seem to be your concern). So what do you exactly want to do? >> >> Hi Jan, >> >> Thanks for pointing out, and I'm sorry for my mistake. It's >> ->dax_iomap_rw(), not ->dax_iomap_actor(). >> >> I want to call the callback function at the end of ->dax_iomap_rw(). >> >> Like this: >> dax_iomap_rw(..., callback) { >> >> ... >> while (...) { >> iomap_apply(...); >> } >> >> if (callback != null) { >> callback(); >> } >> return ...; >> } > > Why does this need to be in dax_iomap_rw()? > > We already do post-dax_iomap_rw() "io-end callbacks" directly in > xfs_file_dax_write() to update the file size.... Yes, but we also need to call ->xfs_reflink_end_cow() after a COW operation. And an is-cow flag(from iomap) is also needed to determine if we call it. I think it would be better to put this into ->dax_iomap_rw() as a callback function. So sorry for my poor expression. > > Cheers, > > Dave. > -- Thanks, Shiyang Ruan.
next prev parent reply other threads:[~2019-05-29 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 [this message] 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 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 04/18] dax: Introduce IOMAP_DAX_COW to CoW edges during writes Goldwyn Rodrigues 2019-04-16 16:41 ` Goldwyn Rodrigues [not found] ` <20190416164154.30390-5-rgoldwyn-l3A5Bk7waGM@public.gmane.org> 2019-04-17 16:46 ` Darrick J. Wong 2019-04-17 16:46 ` 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=376256fd-dee4-5561-eb4e-546e227303cd@cn.fujitsu.com \ --to=ruansy.fnst@cn.fujitsu.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=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.