From: Dan Williams <dan.j.williams@intel.com> To: Dave Chinner <david@fromorbit.com> Cc: Jens Axboe <axboe@fb.com>, Jan Kara <jack@suse.cz>, "linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Ross Zwisler <ross.zwisler@linux.intel.com>, Christoph Hellwig <hch@lst.de> Subject: Re: [PATCH v3 13/15] block, dax: make dax mappings opt-in by default Date: Tue, 3 Nov 2015 15:04:53 -0800 [thread overview] Message-ID: <CAPcyv4jpNtML7NuizvWUVW3Jxma4-D4+c59yJw_PZ140KoVqsw@mail.gmail.com> (raw) In-Reply-To: <20151103202041.GG19199@dastard> On Tue, Nov 3, 2015 at 12:20 PM, Dave Chinner <david@fromorbit.com> wrote: > On Mon, Nov 02, 2015 at 11:35:04PM -0800, Dan Williams wrote: >> On Mon, Nov 2, 2015 at 4:32 PM, Dave Chinner <david@fromorbit.com> wrote: [..] >> Only in the mmap path: > > which means blkdev_direct_IO() is now always going to go down the > dax_do_io() path for any driver with a ->direct_access method rather > than the direct IO path, regardless of whether DAX is enabled on the > device or not. > > That really seems wrong to me - you've replace explicit "is DAX > enabled" checks with "is DAX possible" checks, and so DAX paths are > used regardless of whether DAX is enabled or not. And it's not > obvious why this is done, nor is it now obvious how DAX interacts > with the block device. > > This really seems like a step backwards to me. I think the reason it is not obvious is the original justification for the bypass as stated in commit bbab37ddc20b "block: Add support for DAX reads/writes to block devices" was: "instead of allocating a DIO and a BIO" It turns out it's faster and as far as I can tell semantically equivalent to the __blockdev_direct_IO() path. The DAX mmap path in comparison has plenty of sharp edges and semantic differences that would be avoided by turning off DAX. I'm not opposed to also turning off dax_do_io() when S_DAX is clear, but I don't currently see the point. At the very least I need to add the above comments to the code, but do you still think opt-in DAX is a backwards step?
WARNING: multiple messages have this Message-ID (diff)
From: Dan Williams <dan.j.williams@intel.com> To: Dave Chinner <david@fromorbit.com> Cc: Jens Axboe <axboe@fb.com>, Jan Kara <jack@suse.cz>, "linux-nvdimm@lists.01.org" <linux-nvdimm@ml01.01.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Ross Zwisler <ross.zwisler@linux.intel.com>, Christoph Hellwig <hch@lst.de> Subject: Re: [PATCH v3 13/15] block, dax: make dax mappings opt-in by default Date: Tue, 3 Nov 2015 15:04:53 -0800 [thread overview] Message-ID: <CAPcyv4jpNtML7NuizvWUVW3Jxma4-D4+c59yJw_PZ140KoVqsw@mail.gmail.com> (raw) In-Reply-To: <20151103202041.GG19199@dastard> On Tue, Nov 3, 2015 at 12:20 PM, Dave Chinner <david@fromorbit.com> wrote: > On Mon, Nov 02, 2015 at 11:35:04PM -0800, Dan Williams wrote: >> On Mon, Nov 2, 2015 at 4:32 PM, Dave Chinner <david@fromorbit.com> wrote: [..] >> Only in the mmap path: > > which means blkdev_direct_IO() is now always going to go down the > dax_do_io() path for any driver with a ->direct_access method rather > than the direct IO path, regardless of whether DAX is enabled on the > device or not. > > That really seems wrong to me - you've replace explicit "is DAX > enabled" checks with "is DAX possible" checks, and so DAX paths are > used regardless of whether DAX is enabled or not. And it's not > obvious why this is done, nor is it now obvious how DAX interacts > with the block device. > > This really seems like a step backwards to me. I think the reason it is not obvious is the original justification for the bypass as stated in commit bbab37ddc20b "block: Add support for DAX reads/writes to block devices" was: "instead of allocating a DIO and a BIO" It turns out it's faster and as far as I can tell semantically equivalent to the __blockdev_direct_IO() path. The DAX mmap path in comparison has plenty of sharp edges and semantic differences that would be avoided by turning off DAX. I'm not opposed to also turning off dax_do_io() when S_DAX is clear, but I don't currently see the point. At the very least I need to add the above comments to the code, but do you still think opt-in DAX is a backwards step?
next prev parent reply other threads:[~2015-11-03 23:04 UTC|newest] Thread overview: 95+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-11-02 4:29 [PATCH v3 00/15] block, dax updates for 4.4 Dan Williams 2015-11-02 4:29 ` Dan Williams 2015-11-02 4:29 ` [PATCH v3 01/15] pmem, dax: clean up clear_pmem() Dan Williams 2015-11-02 4:29 ` Dan Williams 2015-11-02 4:29 ` [PATCH v3 02/15] dax: increase granularity of dax_clear_blocks() operations Dan Williams 2015-11-02 4:29 ` Dan Williams 2015-11-03 0:51 ` Dave Chinner 2015-11-03 0:51 ` Dave Chinner 2015-11-03 3:27 ` Dan Williams 2015-11-03 3:27 ` Dan Williams 2015-11-03 4:48 ` Dave Chinner 2015-11-03 4:48 ` Dave Chinner 2015-11-03 5:31 ` Dan Williams 2015-11-03 5:31 ` Dan Williams 2015-11-03 5:52 ` Dave Chinner 2015-11-03 5:52 ` Dave Chinner 2015-11-03 7:24 ` Dan Williams 2015-11-03 7:24 ` Dan Williams 2015-11-03 16:21 ` Jan Kara 2015-11-03 16:21 ` Jan Kara 2015-11-03 17:57 ` Ross Zwisler 2015-11-03 17:57 ` Ross Zwisler 2015-11-03 20:59 ` Dave Chinner 2015-11-03 20:59 ` Dave Chinner 2015-11-02 4:29 ` [PATCH v3 03/15] block, dax: fix lifetime of in-kernel dax mappings with dax_map_atomic() Dan Williams 2015-11-02 4:29 ` Dan Williams 2015-11-03 19:01 ` Ross Zwisler 2015-11-03 19:01 ` Ross Zwisler 2015-11-03 19:09 ` Jeff Moyer 2015-11-03 22:50 ` Dan Williams 2015-11-03 22:50 ` Dan Williams 2016-01-18 10:42 ` Geert Uytterhoeven 2016-01-18 10:42 ` Geert Uytterhoeven 2015-11-02 4:30 ` [PATCH v3 04/15] libnvdimm, pmem: move request_queue allocation earlier in probe Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-03 19:15 ` Ross Zwisler 2015-11-03 19:15 ` Ross Zwisler 2015-11-02 4:30 ` [PATCH v3 05/15] libnvdimm, pmem: fix size trim in pmem_direct_access() Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-03 19:32 ` Ross Zwisler 2015-11-03 19:32 ` Ross Zwisler 2015-11-03 21:39 ` Dan Williams 2015-11-03 21:39 ` Dan Williams 2015-11-02 4:30 ` [PATCH v3 06/15] um: kill pfn_t Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-02 4:30 ` [PATCH v3 07/15] kvm: rename pfn_t to kvm_pfn_t Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-02 4:30 ` [PATCH v3 08/15] mm, dax, pmem: introduce pfn_t Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-02 16:30 ` Joe Perches 2015-11-02 16:30 ` Joe Perches 2015-11-02 4:30 ` [PATCH v3 09/15] block: notify queue death confirmation Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-02 4:30 ` [PATCH v3 10/15] dax, pmem: introduce zone_device_revoke() and devm_memunmap_pages() Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-02 4:30 ` [PATCH v3 11/15] block: introduce bdev_file_inode() Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-02 4:30 ` [PATCH v3 12/15] block: enable dax for raw block devices Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-02 4:30 ` [PATCH v3 13/15] block, dax: make dax mappings opt-in by default Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-03 0:32 ` Dave Chinner 2015-11-03 0:32 ` Dave Chinner 2015-11-03 7:35 ` Dan Williams 2015-11-03 7:35 ` Dan Williams 2015-11-03 20:20 ` Dave Chinner 2015-11-03 20:20 ` Dave Chinner 2015-11-03 23:04 ` Dan Williams [this message] 2015-11-03 23:04 ` Dan Williams 2015-11-04 19:23 ` Dan Williams 2015-11-04 19:23 ` Dan Williams 2015-11-02 4:30 ` [PATCH v3 14/15] dax: dirty extent notification Dan Williams 2015-11-02 4:30 ` Dan Williams 2015-11-03 1:16 ` Dave Chinner 2015-11-03 1:16 ` Dave Chinner 2015-11-03 4:56 ` Dan Williams 2015-11-03 4:56 ` Dan Williams 2015-11-03 5:40 ` Dave Chinner 2015-11-03 5:40 ` Dave Chinner 2015-11-03 7:20 ` Dan Williams 2015-11-03 7:20 ` Dan Williams 2015-11-03 20:51 ` Dave Chinner 2015-11-03 20:51 ` Dave Chinner 2015-11-03 21:19 ` Dan Williams 2015-11-03 21:19 ` Dan Williams 2015-11-03 21:37 ` Ross Zwisler 2015-11-03 21:37 ` Ross Zwisler 2015-11-03 21:43 ` Dan Williams 2015-11-03 21:43 ` Dan Williams 2015-11-03 21:18 ` Ross Zwisler 2015-11-03 21:18 ` Ross Zwisler 2015-11-03 21:34 ` Dan Williams 2015-11-03 21:34 ` Dan Williams 2015-11-02 4:31 ` [PATCH v3 15/15] pmem: blkdev_issue_flush support Dan Williams 2015-11-02 4:31 ` Dan Williams
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=CAPcyv4jpNtML7NuizvWUVW3Jxma4-D4+c59yJw_PZ140KoVqsw@mail.gmail.com \ --to=dan.j.williams@intel.com \ --cc=axboe@fb.com \ --cc=david@fromorbit.com \ --cc=hch@lst.de \ --cc=jack@suse.cz \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvdimm@lists.01.org \ --cc=ross.zwisler@linux.intel.com \ /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.