* [PATCH v3 0/3] Fix DM DAX handling
@ 2018-06-26 17:59 Ross Zwisler
2018-06-26 17:59 ` [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode Ross Zwisler
` (3 more replies)
0 siblings, 4 replies; 27+ messages in thread
From: Ross Zwisler @ 2018-06-26 17:59 UTC (permalink / raw)
To: Toshi Kani, Mike Snitzer, dm-devel
Cc: Ross Zwisler, linux-fsdevel, linux-kernel, linux-nvdimm, linux-xfs
This series fixes a few issues that I found with DM's handling of DAX
devices. Here are some of the issues I found:
* We can create a dm-stripe or dm-linear device which is made up of an
fsdax PMEM namespace and a raw PMEM namespace but which can hold a
filesystem mounted with the -o dax mount option. DAX operations to
the raw PMEM namespace part lack struct page and can fail in
interesting/unexpected ways when doing things like fork(), examining
memory with gdb, etc.
* We can create a dm-stripe or dm-linear device which is made up of an
fsdax PMEM namespace and a BRD ramdisk which can hold a filesystem
mounted with the -o dax mount option. All I/O to this filesystem
will fail.
---
Changes since v2:
* Only set QUEUE_FLAG_DAX for fsdax mode PMEM namespaces. (Mike)
* Check for QUEUE_FLAG_DAX in __bdev_dax_supported(). (Mike)
* Get rid of DM_TYPE_DAX_BIO_BASED reworks. (Mike)
* Dropped the first 2 prep patches of v2 since they were merged for
v4.18-rc1. (Thanks, Darrick!)
---
Mike, can you take this series through your tree?
Personally I think this should be treated as a bug fix and merged in the
v4.18-rc* series.
Ross Zwisler (3):
pmem: only set QUEUE_FLAG_DAX for fsdax mode
dax: bdev_dax_supported() check for QUEUE_FLAG_DAX
dm: prevent DAX mounts if not supported
drivers/dax/super.c | 8 ++++++++
drivers/md/dm-ioctl.c | 5 +++++
drivers/md/dm-table.c | 7 +++----
drivers/md/dm.c | 3 +--
drivers/nvdimm/pmem.c | 3 ++-
include/linux/device-mapper.h | 5 +++++
6 files changed, 24 insertions(+), 7 deletions(-)
--
2.14.4
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 17:59 [PATCH v3 0/3] Fix DM DAX handling Ross Zwisler @ 2018-06-26 17:59 ` Ross Zwisler 2018-06-26 18:52 ` Dan Williams 2018-06-26 17:59 ` [PATCH v3 2/3] dax: bdev_dax_supported() check for QUEUE_FLAG_DAX Ross Zwisler ` (2 subsequent siblings) 3 siblings, 1 reply; 27+ messages in thread From: Ross Zwisler @ 2018-06-26 17:59 UTC (permalink / raw) To: Toshi Kani, Mike Snitzer, dm-devel Cc: Ross Zwisler, linux-fsdevel, linux-kernel, linux-nvdimm, linux-xfs, stable QUEUE_FLAG_DAX is an indication that a given block device supports filesystem DAX and should not be set for PMEM namespaces which are in "raw" or "sector" modes. These namespaces lack struct page and are prevented from participating in filesystem DAX. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> Suggested-by: Mike Snitzer <snitzer@redhat.com> Cc: stable@vger.kernel.org --- drivers/nvdimm/pmem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index 68940356cad3..8b1fd7f1a224 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -414,7 +414,8 @@ static int pmem_attach_disk(struct device *dev, blk_queue_logical_block_size(q, pmem_sector_size(ndns)); blk_queue_max_hw_sectors(q, UINT_MAX); blk_queue_flag_set(QUEUE_FLAG_NONROT, q); - blk_queue_flag_set(QUEUE_FLAG_DAX, q); + if (pmem->pfn_flags & PFN_MAP) + blk_queue_flag_set(QUEUE_FLAG_DAX, q); q->queuedata = pmem; disk = alloc_disk_node(0, nid); -- 2.14.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 17:59 ` [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode Ross Zwisler @ 2018-06-26 18:52 ` Dan Williams 2018-06-26 18:58 ` Mike Snitzer 0 siblings, 1 reply; 27+ messages in thread From: Dan Williams @ 2018-06-26 18:52 UTC (permalink / raw) To: Ross Zwisler Cc: Toshi Kani, Mike Snitzer, device-mapper development, linux-nvdimm, Linux Kernel Mailing List, stable, linux-xfs, linux-fsdevel On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > QUEUE_FLAG_DAX is an indication that a given block device supports > filesystem DAX and should not be set for PMEM namespaces which are in "raw" > or "sector" modes. These namespaces lack struct page and are prevented > from participating in filesystem DAX. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > Suggested-by: Mike Snitzer <snitzer@redhat.com> > Cc: stable@vger.kernel.org Why is this cc: stable? What is the user visible impact of this change especially given the requirement to validate QUEUE_FLAG_DAX with bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup afaics. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 18:52 ` Dan Williams @ 2018-06-26 18:58 ` Mike Snitzer 2018-06-26 19:07 ` Dan Williams 2018-06-26 19:11 ` Ross Zwisler 0 siblings, 2 replies; 27+ messages in thread From: Mike Snitzer @ 2018-06-26 18:58 UTC (permalink / raw) To: Dan Williams Cc: Ross Zwisler, Toshi Kani, device-mapper development, linux-nvdimm, Linux Kernel Mailing List, stable, linux-xfs, linux-fsdevel On Tue, Jun 26 2018 at 2:52pm -0400, Dan Williams <dan.j.williams@intel.com> wrote: > On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler > <ross.zwisler@linux.intel.com> wrote: > > QUEUE_FLAG_DAX is an indication that a given block device supports > > filesystem DAX and should not be set for PMEM namespaces which are in "raw" > > or "sector" modes. These namespaces lack struct page and are prevented > > from participating in filesystem DAX. > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > Suggested-by: Mike Snitzer <snitzer@redhat.com> > > Cc: stable@vger.kernel.org > > Why is this cc: stable? What is the user visible impact of this change > especially given the requirement to validate QUEUE_FLAG_DAX with > bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup > afaics. This isn't cosmetic when you consider that stacking up a DM device is looking at this flag to determine whether a table does or does _not_ support DAX. So this patch, in conjunction with the other changes in the series, is certainly something I'd consider appropriate for stable. Mike ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 18:58 ` Mike Snitzer @ 2018-06-26 19:07 ` Dan Williams 2018-06-26 19:12 ` Ross Zwisler 2018-06-26 19:13 ` Mike Snitzer 2018-06-26 19:11 ` Ross Zwisler 1 sibling, 2 replies; 27+ messages in thread From: Dan Williams @ 2018-06-26 19:07 UTC (permalink / raw) To: Mike Snitzer Cc: Ross Zwisler, Toshi Kani, device-mapper development, linux-nvdimm, Linux Kernel Mailing List, stable, linux-xfs, linux-fsdevel On Tue, Jun 26, 2018 at 11:58 AM, Mike Snitzer <snitzer@redhat.com> wrote: > On Tue, Jun 26 2018 at 2:52pm -0400, > Dan Williams <dan.j.williams@intel.com> wrote: > >> On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler >> <ross.zwisler@linux.intel.com> wrote: >> > QUEUE_FLAG_DAX is an indication that a given block device supports >> > filesystem DAX and should not be set for PMEM namespaces which are in "raw" >> > or "sector" modes. These namespaces lack struct page and are prevented >> > from participating in filesystem DAX. >> > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> > Suggested-by: Mike Snitzer <snitzer@redhat.com> >> > Cc: stable@vger.kernel.org >> >> Why is this cc: stable? What is the user visible impact of this change >> especially given the requirement to validate QUEUE_FLAG_DAX with >> bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup >> afaics. > > This isn't cosmetic when you consider that stacking up a DM device is > looking at this flag to determine whether a table does or does _not_ > support DAX. > > So this patch, in conjunction with the other changes in the series, is > certainly something I'd consider appropriate for stable. I think this classifies as something that never worked correctly and is not a regression. It does not identify which commit it is repairing or the user visible failure mode. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 19:07 ` Dan Williams @ 2018-06-26 19:12 ` Ross Zwisler 2018-06-26 19:13 ` Mike Snitzer 1 sibling, 0 replies; 27+ messages in thread From: Ross Zwisler @ 2018-06-26 19:12 UTC (permalink / raw) To: Dan Williams Cc: Mike Snitzer, Ross Zwisler, Toshi Kani, device-mapper development, linux-nvdimm, Linux Kernel Mailing List, stable, linux-xfs, linux-fsdevel On Tue, Jun 26, 2018 at 12:07:40PM -0700, Dan Williams wrote: > On Tue, Jun 26, 2018 at 11:58 AM, Mike Snitzer <snitzer@redhat.com> wrote: > > On Tue, Jun 26 2018 at 2:52pm -0400, > > Dan Williams <dan.j.williams@intel.com> wrote: > > > >> On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler > >> <ross.zwisler@linux.intel.com> wrote: > >> > QUEUE_FLAG_DAX is an indication that a given block device supports > >> > filesystem DAX and should not be set for PMEM namespaces which are in "raw" > >> > or "sector" modes. These namespaces lack struct page and are prevented > >> > from participating in filesystem DAX. > >> > > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > >> > Suggested-by: Mike Snitzer <snitzer@redhat.com> > >> > Cc: stable@vger.kernel.org > >> > >> Why is this cc: stable? What is the user visible impact of this change > >> especially given the requirement to validate QUEUE_FLAG_DAX with > >> bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup > >> afaics. > > > > This isn't cosmetic when you consider that stacking up a DM device is > > looking at this flag to determine whether a table does or does _not_ > > support DAX. > > > > So this patch, in conjunction with the other changes in the series, is > > certainly something I'd consider appropriate for stable. > > I think this classifies as something that never worked correctly and > is not a regression. It does not identify which commit it is repairing > or the user visible failure mode. Ah, do I need a Fixes: tag for patch 2, then? That one *does* need to go to stable, I think. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 19:07 ` Dan Williams 2018-06-26 19:12 ` Ross Zwisler @ 2018-06-26 19:13 ` Mike Snitzer 2018-06-26 19:19 ` Dan Williams 2018-06-26 20:54 ` Kani, Toshi 1 sibling, 2 replies; 27+ messages in thread From: Mike Snitzer @ 2018-06-26 19:13 UTC (permalink / raw) To: Dan Williams Cc: Ross Zwisler, Toshi Kani, device-mapper development, linux-nvdimm, Linux Kernel Mailing List, stable, linux-xfs, linux-fsdevel On Tue, Jun 26 2018 at 3:07pm -0400, Dan Williams <dan.j.williams@intel.com> wrote: > On Tue, Jun 26, 2018 at 11:58 AM, Mike Snitzer <snitzer@redhat.com> wrote: > > On Tue, Jun 26 2018 at 2:52pm -0400, > > Dan Williams <dan.j.williams@intel.com> wrote: > > > >> On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler > >> <ross.zwisler@linux.intel.com> wrote: > >> > QUEUE_FLAG_DAX is an indication that a given block device supports > >> > filesystem DAX and should not be set for PMEM namespaces which are in "raw" > >> > or "sector" modes. These namespaces lack struct page and are prevented > >> > from participating in filesystem DAX. > >> > > >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > >> > Suggested-by: Mike Snitzer <snitzer@redhat.com> > >> > Cc: stable@vger.kernel.org > >> > >> Why is this cc: stable? What is the user visible impact of this change > >> especially given the requirement to validate QUEUE_FLAG_DAX with > >> bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup > >> afaics. > > > > This isn't cosmetic when you consider that stacking up a DM device is > > looking at this flag to determine whether a table does or does _not_ > > support DAX. > > > > So this patch, in conjunction with the other changes in the series, is > > certainly something I'd consider appropriate for stable. > > I think this classifies as something that never worked correctly and > is not a regression. It does not identify which commit it is repairing > or the user visible failure mode. So you're taking issue with making stacked dax configs work in older kernels? That's fine. We can drop the stable cc if you like. But I mean we intended for this to work.. so the Fixes commit references can easily be added, e.g.: 545ed20e6df68a4d2584a29a2a28ee8b2f7e9547 ("dm: add infrastructure for DAX support") ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 19:13 ` Mike Snitzer @ 2018-06-26 19:19 ` Dan Williams 2018-06-26 20:54 ` Kani, Toshi 1 sibling, 0 replies; 27+ messages in thread From: Dan Williams @ 2018-06-26 19:19 UTC (permalink / raw) To: Mike Snitzer Cc: Ross Zwisler, Toshi Kani, device-mapper development, linux-nvdimm, Linux Kernel Mailing List, stable, linux-xfs, linux-fsdevel On Tue, Jun 26, 2018 at 12:13 PM, Mike Snitzer <snitzer@redhat.com> wrote: > On Tue, Jun 26 2018 at 3:07pm -0400, > Dan Williams <dan.j.williams@intel.com> wrote: > >> On Tue, Jun 26, 2018 at 11:58 AM, Mike Snitzer <snitzer@redhat.com> wrote: >> > On Tue, Jun 26 2018 at 2:52pm -0400, >> > Dan Williams <dan.j.williams@intel.com> wrote: >> > >> >> On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler >> >> <ross.zwisler@linux.intel.com> wrote: >> >> > QUEUE_FLAG_DAX is an indication that a given block device supports >> >> > filesystem DAX and should not be set for PMEM namespaces which are in "raw" >> >> > or "sector" modes. These namespaces lack struct page and are prevented >> >> > from participating in filesystem DAX. >> >> > >> >> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> >> > Suggested-by: Mike Snitzer <snitzer@redhat.com> >> >> > Cc: stable@vger.kernel.org >> >> >> >> Why is this cc: stable? What is the user visible impact of this change >> >> especially given the requirement to validate QUEUE_FLAG_DAX with >> >> bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup >> >> afaics. >> > >> > This isn't cosmetic when you consider that stacking up a DM device is >> > looking at this flag to determine whether a table does or does _not_ >> > support DAX. >> > >> > So this patch, in conjunction with the other changes in the series, is >> > certainly something I'd consider appropriate for stable. >> >> I think this classifies as something that never worked correctly and >> is not a regression. It does not identify which commit it is repairing >> or the user visible failure mode. > > So you're taking issue with making stacked dax configs work in older > kernels? That's fine. We can drop the stable cc if you like. > > But I mean we intended for this to work.. so the Fixes commit references > can easily be added, e.g.: 545ed20e6df68a4d2584a29a2a28ee8b2f7e9547 > ("dm: add infrastructure for DAX support") ...but to be clear stacked DAX configs are not broken, right? They just happen not to work in this corner case of pmem devices without pages. Given we dropped DAX support for page less devices what configurations are repaired by this change? Am I missing something? Is it a data corruption scenario if this DM gets the wrong idea about DAX support. I could be convinced to recommend this for -stable, but the changelog as written does not make a strong case. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 19:13 ` Mike Snitzer 2018-06-26 19:19 ` Dan Williams @ 2018-06-26 20:54 ` Kani, Toshi 2018-06-26 21:02 ` Dan Williams 1 sibling, 1 reply; 27+ messages in thread From: Kani, Toshi @ 2018-06-26 20:54 UTC (permalink / raw) To: dan.j.williams, snitzer Cc: dm-devel, ross.zwisler, linux-nvdimm, linux-kernel, stable, linux-fsdevel, linux-xfs On Tue, 2018-06-26 at 15:13 -0400, Mike Snitzer wrote: > On Tue, Jun 26 2018 at 3:07pm -0400, > Dan Williams <dan.j.williams@intel.com> wrote: > > > On Tue, Jun 26, 2018 at 11:58 AM, Mike Snitzer <snitzer@redhat.com> wrote: > > > On Tue, Jun 26 2018 at 2:52pm -0400, > > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > > On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler > > > > <ross.zwisler@linux.intel.com> wrote: > > > > > QUEUE_FLAG_DAX is an indication that a given block device supports > > > > > filesystem DAX and should not be set for PMEM namespaces which are in "raw" > > > > > or "sector" modes. These namespaces lack struct page and are prevented > > > > > from participating in filesystem DAX. > > > > > > > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > > > Suggested-by: Mike Snitzer <snitzer@redhat.com> > > > > > Cc: stable@vger.kernel.org > > > > > > > > Why is this cc: stable? What is the user visible impact of this change > > > > especially given the requirement to validate QUEUE_FLAG_DAX with > > > > bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup > > > > afaics. > > > > > > This isn't cosmetic when you consider that stacking up a DM device is > > > looking at this flag to determine whether a table does or does _not_ > > > support DAX. > > > > > > So this patch, in conjunction with the other changes in the series, is > > > certainly something I'd consider appropriate for stable. > > > > I think this classifies as something that never worked correctly and > > is not a regression. It does not identify which commit it is repairing > > or the user visible failure mode. > > So you're taking issue with making stacked dax configs work in older > kernels? That's fine. We can drop the stable cc if you like. > > But I mean we intended for this to work.. so the Fixes commit references > can easily be added, e.g.: 545ed20e6df68a4d2584a29a2a28ee8b2f7e9547 > ("dm: add infrastructure for DAX support") When this dm change was made, the pmem driver supported DAX for both raw and memory modes (note: sector mode does not use the pmem driver). I think the issue was introduced when we dropped DAX support from raw mode. Thanks, -Toshi ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 20:54 ` Kani, Toshi @ 2018-06-26 21:02 ` Dan Williams 2018-06-26 21:23 ` Kani, Toshi 0 siblings, 1 reply; 27+ messages in thread From: Dan Williams @ 2018-06-26 21:02 UTC (permalink / raw) To: Kani, Toshi Cc: snitzer, dm-devel, ross.zwisler, linux-nvdimm, linux-kernel, stable, linux-fsdevel, linux-xfs On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > On Tue, 2018-06-26 at 15:13 -0400, Mike Snitzer wrote: >> On Tue, Jun 26 2018 at 3:07pm -0400, >> Dan Williams <dan.j.williams@intel.com> wrote: >> >> > On Tue, Jun 26, 2018 at 11:58 AM, Mike Snitzer <snitzer@redhat.com> wrote: >> > > On Tue, Jun 26 2018 at 2:52pm -0400, >> > > Dan Williams <dan.j.williams@intel.com> wrote: >> > > >> > > > On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler >> > > > <ross.zwisler@linux.intel.com> wrote: >> > > > > QUEUE_FLAG_DAX is an indication that a given block device supports >> > > > > filesystem DAX and should not be set for PMEM namespaces which are in "raw" >> > > > > or "sector" modes. These namespaces lack struct page and are prevented >> > > > > from participating in filesystem DAX. >> > > > > >> > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> > > > > Suggested-by: Mike Snitzer <snitzer@redhat.com> >> > > > > Cc: stable@vger.kernel.org >> > > > >> > > > Why is this cc: stable? What is the user visible impact of this change >> > > > especially given the requirement to validate QUEUE_FLAG_DAX with >> > > > bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup >> > > > afaics. >> > > >> > > This isn't cosmetic when you consider that stacking up a DM device is >> > > looking at this flag to determine whether a table does or does _not_ >> > > support DAX. >> > > >> > > So this patch, in conjunction with the other changes in the series, is >> > > certainly something I'd consider appropriate for stable. >> > >> > I think this classifies as something that never worked correctly and >> > is not a regression. It does not identify which commit it is repairing >> > or the user visible failure mode. >> >> So you're taking issue with making stacked dax configs work in older >> kernels? That's fine. We can drop the stable cc if you like. >> >> But I mean we intended for this to work.. so the Fixes commit references >> can easily be added, e.g.: 545ed20e6df68a4d2584a29a2a28ee8b2f7e9547 >> ("dm: add infrastructure for DAX support") > > When this dm change was made, the pmem driver supported DAX for both raw > and memory modes (note: sector mode does not use the pmem driver). I > think the issue was introduced when we dropped DAX support from raw > mode. Still DAX with raw mode never really worked any way. It was also something that was broken from day one. So what happens to someone who happened to avoid all the problems with page-less DAX and enabled device-mapper on top? That failure mode detail needs to be added to this changelog if we want to propose this for -stable. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 21:02 ` Dan Williams @ 2018-06-26 21:23 ` Kani, Toshi 2018-06-26 21:28 ` Dan Williams 0 siblings, 1 reply; 27+ messages in thread From: Kani, Toshi @ 2018-06-26 21:23 UTC (permalink / raw) To: dan.j.williams Cc: linux-kernel, linux-xfs, snitzer, stable, linux-nvdimm, linux-fsdevel, ross.zwisler, dm-devel On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: > On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > On Tue, 2018-06-26 at 15:13 -0400, Mike Snitzer wrote: > > > On Tue, Jun 26 2018 at 3:07pm -0400, > > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > > On Tue, Jun 26, 2018 at 11:58 AM, Mike Snitzer <snitzer@redhat.com> wrote: > > > > > On Tue, Jun 26 2018 at 2:52pm -0400, > > > > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > > > > > > On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler > > > > > > <ross.zwisler@linux.intel.com> wrote: > > > > > > > QUEUE_FLAG_DAX is an indication that a given block device supports > > > > > > > filesystem DAX and should not be set for PMEM namespaces which are in "raw" > > > > > > > or "sector" modes. These namespaces lack struct page and are prevented > > > > > > > from participating in filesystem DAX. > > > > > > > > > > > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > > > > > Suggested-by: Mike Snitzer <snitzer@redhat.com> > > > > > > > Cc: stable@vger.kernel.org > > > > > > > > > > > > Why is this cc: stable? What is the user visible impact of this change > > > > > > especially given the requirement to validate QUEUE_FLAG_DAX with > > > > > > bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup > > > > > > afaics. > > > > > > > > > > This isn't cosmetic when you consider that stacking up a DM device is > > > > > looking at this flag to determine whether a table does or does _not_ > > > > > support DAX. > > > > > > > > > > So this patch, in conjunction with the other changes in the series, is > > > > > certainly something I'd consider appropriate for stable. > > > > > > > > I think this classifies as something that never worked correctly and > > > > is not a regression. It does not identify which commit it is repairing > > > > or the user visible failure mode. > > > > > > So you're taking issue with making stacked dax configs work in older > > > kernels? That's fine. We can drop the stable cc if you like. > > > > > > But I mean we intended for this to work.. so the Fixes commit references > > > can easily be added, e.g.: 545ed20e6df68a4d2584a29a2a28ee8b2f7e9547 > > > ("dm: add infrastructure for DAX support") > > > > When this dm change was made, the pmem driver supported DAX for both raw > > and memory modes (note: sector mode does not use the pmem driver). I > > think the issue was introduced when we dropped DAX support from raw > > mode. > > Still DAX with raw mode never really worked any way. It was also > something that was broken from day one. So what happens to someone who > happened to avoid all the problems with page-less DAX and enabled > device-mapper on top? That failure mode detail needs to be added to > this changelog if we want to propose this for -stable. My point is that the behavior should be consistent between pmem and device-mapper. When -o dax succeeds on a pmem, then it should succeed on a device-mapper on top of that pmem. Has the drop of dax support from raw mode made to -stable back to the baseline accepted 545ed20e6df6? It will introduce inconsistency, otherwise. Thanks, -Toshi ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 21:23 ` Kani, Toshi @ 2018-06-26 21:28 ` Dan Williams 2018-06-26 21:31 ` Kani, Toshi 0 siblings, 1 reply; 27+ messages in thread From: Dan Williams @ 2018-06-26 21:28 UTC (permalink / raw) To: Kani, Toshi Cc: linux-kernel, linux-xfs, snitzer, stable, linux-nvdimm, linux-fsdevel, ross.zwisler, dm-devel On Tue, Jun 26, 2018 at 2:23 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: >> On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: [..] >> > When this dm change was made, the pmem driver supported DAX for both raw >> > and memory modes (note: sector mode does not use the pmem driver). I >> > think the issue was introduced when we dropped DAX support from raw >> > mode. >> >> Still DAX with raw mode never really worked any way. It was also >> something that was broken from day one. So what happens to someone who >> happened to avoid all the problems with page-less DAX and enabled >> device-mapper on top? That failure mode detail needs to be added to >> this changelog if we want to propose this for -stable. > > My point is that the behavior should be consistent between pmem and > device-mapper. When -o dax succeeds on a pmem, then it should succeed > on a device-mapper on top of that pmem. > > Has the drop of dax support from raw mode made to -stable back to the > baseline accepted 545ed20e6df6? It will introduce inconsistency, > otherwise. That commit, 569d0365f571 "dax: require 'struct page' by default for filesystem dax", has not been tagged for -stable. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 21:28 ` Dan Williams @ 2018-06-26 21:31 ` Kani, Toshi 2018-06-26 21:51 ` Dan Williams 0 siblings, 1 reply; 27+ messages in thread From: Kani, Toshi @ 2018-06-26 21:31 UTC (permalink / raw) To: dan.j.williams Cc: linux-kernel, linux-xfs, snitzer, stable, linux-nvdimm, linux-fsdevel, ross.zwisler, dm-devel On Tue, 2018-06-26 at 14:28 -0700, Dan Williams wrote: > On Tue, Jun 26, 2018 at 2:23 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: > > > On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > [..] > > > > When this dm change was made, the pmem driver supported DAX for both raw > > > > and memory modes (note: sector mode does not use the pmem driver). I > > > > think the issue was introduced when we dropped DAX support from raw > > > > mode. > > > > > > Still DAX with raw mode never really worked any way. It was also > > > something that was broken from day one. So what happens to someone who > > > happened to avoid all the problems with page-less DAX and enabled > > > device-mapper on top? That failure mode detail needs to be added to > > > this changelog if we want to propose this for -stable. > > > > My point is that the behavior should be consistent between pmem and > > device-mapper. When -o dax succeeds on a pmem, then it should succeed > > on a device-mapper on top of that pmem. > > > > Has the drop of dax support from raw mode made to -stable back to the > > baseline accepted 545ed20e6df6? It will introduce inconsistency, > > otherwise. > > That commit, 569d0365f571 "dax: require 'struct page' by default for > filesystem dax", has not been tagged for -stable. Then, Fixes tag should be set to 569d0365f571 to keep the behavior consistent. -Toshi ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 21:31 ` Kani, Toshi @ 2018-06-26 21:51 ` Dan Williams 2018-06-26 22:04 ` Ross Zwisler 0 siblings, 1 reply; 27+ messages in thread From: Dan Williams @ 2018-06-26 21:51 UTC (permalink / raw) To: Kani, Toshi Cc: linux-kernel, linux-xfs, snitzer, stable, linux-nvdimm, linux-fsdevel, ross.zwisler, dm-devel On Tue, Jun 26, 2018 at 2:31 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > On Tue, 2018-06-26 at 14:28 -0700, Dan Williams wrote: >> On Tue, Jun 26, 2018 at 2:23 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: >> > On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: >> > > On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: >> >> [..] >> > > > When this dm change was made, the pmem driver supported DAX for both raw >> > > > and memory modes (note: sector mode does not use the pmem driver). I >> > > > think the issue was introduced when we dropped DAX support from raw >> > > > mode. >> > > >> > > Still DAX with raw mode never really worked any way. It was also >> > > something that was broken from day one. So what happens to someone who >> > > happened to avoid all the problems with page-less DAX and enabled >> > > device-mapper on top? That failure mode detail needs to be added to >> > > this changelog if we want to propose this for -stable. >> > >> > My point is that the behavior should be consistent between pmem and >> > device-mapper. When -o dax succeeds on a pmem, then it should succeed >> > on a device-mapper on top of that pmem. >> > >> > Has the drop of dax support from raw mode made to -stable back to the >> > baseline accepted 545ed20e6df6? It will introduce inconsistency, >> > otherwise. >> >> That commit, 569d0365f571 "dax: require 'struct page' by default for >> filesystem dax", has not been tagged for -stable. > > Then, Fixes tag should be set to 569d0365f571 to keep the behavior > consistent. Sure, and the failure mode is...? I'm thinking the commit log should say: "Starting with commit 569d0365f571 "dax: require 'struct page' by default for filesystem dax", dax is no longer supported for page-less configurations. However, device-mapper sees the QUEUE_FLAG_DAX still being set and falsely assumes that DAX is enabled, this leads to <insert user visible failure mode details here>" ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 21:51 ` Dan Williams @ 2018-06-26 22:04 ` Ross Zwisler 2018-06-28 17:42 ` Kani, Toshi 0 siblings, 1 reply; 27+ messages in thread From: Ross Zwisler @ 2018-06-26 22:04 UTC (permalink / raw) To: Dan Williams Cc: Kani, Toshi, linux-kernel, linux-xfs, snitzer, stable, linux-nvdimm, linux-fsdevel, ross.zwisler, dm-devel On Tue, Jun 26, 2018 at 02:51:52PM -0700, Dan Williams wrote: > On Tue, Jun 26, 2018 at 2:31 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > On Tue, 2018-06-26 at 14:28 -0700, Dan Williams wrote: > >> On Tue, Jun 26, 2018 at 2:23 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > >> > On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: > >> > > On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > >> > >> [..] > >> > > > When this dm change was made, the pmem driver supported DAX for both raw > >> > > > and memory modes (note: sector mode does not use the pmem driver). I > >> > > > think the issue was introduced when we dropped DAX support from raw > >> > > > mode. > >> > > > >> > > Still DAX with raw mode never really worked any way. It was also > >> > > something that was broken from day one. So what happens to someone who > >> > > happened to avoid all the problems with page-less DAX and enabled > >> > > device-mapper on top? That failure mode detail needs to be added to > >> > > this changelog if we want to propose this for -stable. > >> > > >> > My point is that the behavior should be consistent between pmem and > >> > device-mapper. When -o dax succeeds on a pmem, then it should succeed > >> > on a device-mapper on top of that pmem. > >> > > >> > Has the drop of dax support from raw mode made to -stable back to the > >> > baseline accepted 545ed20e6df6? It will introduce inconsistency, > >> > otherwise. > >> > >> That commit, 569d0365f571 "dax: require 'struct page' by default for > >> filesystem dax", has not been tagged for -stable. > > > > Then, Fixes tag should be set to 569d0365f571 to keep the behavior > > consistent. > > Sure, and the failure mode is...? I'm thinking the commit log should say: > > "Starting with commit 569d0365f571 "dax: require 'struct page' by > default for filesystem dax", dax is no longer supported for page-less > configurations. However, device-mapper sees the QUEUE_FLAG_DAX still > being set and falsely assumes that DAX is enabled, this leads to > <insert user visible failure mode details here>" Dan is correct that there is no user visible change for this. It is the right thing to do for consistency and sanity, but it doesn't actually have user visible behavior that needs to be backported to stable. Toshi is correct that this change is only for raw mode namespaces, not btt namespaces. I'll adjust the changelog and remove the stable flag for v5, and I'll add a Fixes: tag for patch 2. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 22:04 ` Ross Zwisler @ 2018-06-28 17:42 ` Kani, Toshi 2018-06-28 17:48 ` Mike Snitzer 2018-06-28 19:04 ` Ross Zwisler 0 siblings, 2 replies; 27+ messages in thread From: Kani, Toshi @ 2018-06-28 17:42 UTC (permalink / raw) To: dan.j.williams, ross.zwisler Cc: dm-devel, linux-kernel, linux-xfs, linux-nvdimm, stable, snitzer, linux-fsdevel On Tue, 2018-06-26 at 16:04 -0600, Ross Zwisler wrote: > On Tue, Jun 26, 2018 at 02:51:52PM -0700, Dan Williams wrote: > > On Tue, Jun 26, 2018 at 2:31 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > On Tue, 2018-06-26 at 14:28 -0700, Dan Williams wrote: > > > > On Tue, Jun 26, 2018 at 2:23 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > > On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: > > > > > > On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > > > > > [..] > > > > > > > When this dm change was made, the pmem driver supported DAX for both raw > > > > > > > and memory modes (note: sector mode does not use the pmem driver). I > > > > > > > think the issue was introduced when we dropped DAX support from raw > > > > > > > mode. > > > > > > > > > > > > Still DAX with raw mode never really worked any way. It was also > > > > > > something that was broken from day one. So what happens to someone who > > > > > > happened to avoid all the problems with page-less DAX and enabled > > > > > > device-mapper on top? That failure mode detail needs to be added to > > > > > > this changelog if we want to propose this for -stable. > > > > > > > > > > My point is that the behavior should be consistent between pmem and > > > > > device-mapper. When -o dax succeeds on a pmem, then it should succeed > > > > > on a device-mapper on top of that pmem. > > > > > > > > > > Has the drop of dax support from raw mode made to -stable back to the > > > > > baseline accepted 545ed20e6df6? It will introduce inconsistency, > > > > > otherwise. > > > > > > > > That commit, 569d0365f571 "dax: require 'struct page' by default for > > > > filesystem dax", has not been tagged for -stable. > > > > > > Then, Fixes tag should be set to 569d0365f571 to keep the behavior > > > consistent. > > > > Sure, and the failure mode is...? I'm thinking the commit log should say: > > > > "Starting with commit 569d0365f571 "dax: require 'struct page' by > > default for filesystem dax", dax is no longer supported for page-less > > configurations. However, device-mapper sees the QUEUE_FLAG_DAX still > > being set and falsely assumes that DAX is enabled, this leads to > > <insert user visible failure mode details here>" > > Dan is correct that there is no user visible change for this. It is the right > thing to do for consistency and sanity, but it doesn't actually have user > visible behavior that needs to be backported to stable. > > Toshi is correct that this change is only for raw mode namespaces, not btt > namespaces. > > I'll adjust the changelog and remove the stable flag for v5, and I'll add a > Fixes: tag for patch 2. Hi Ross, Your patches look good. But I am still not clear about the Fixes & stable handling. Talking about user visible behavior, I do not think we had any issue until dax support was dropped from raw mode. Until then, the pmem driver supported dax for all modes, and the check for direct_access worked. Thanks, -Toshi ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-28 17:42 ` Kani, Toshi @ 2018-06-28 17:48 ` Mike Snitzer 2018-06-28 17:59 ` Dan Williams 2018-06-28 18:01 ` Kani, Toshi 2018-06-28 19:04 ` Ross Zwisler 1 sibling, 2 replies; 27+ messages in thread From: Mike Snitzer @ 2018-06-28 17:48 UTC (permalink / raw) To: Kani, Toshi Cc: dan.j.williams, ross.zwisler, dm-devel, linux-kernel, linux-xfs, linux-nvdimm, stable, linux-fsdevel On Thu, Jun 28 2018 at 1:42pm -0400, Kani, Toshi <toshi.kani@hpe.com> wrote: > On Tue, 2018-06-26 at 16:04 -0600, Ross Zwisler wrote: > > On Tue, Jun 26, 2018 at 02:51:52PM -0700, Dan Williams wrote: > > > On Tue, Jun 26, 2018 at 2:31 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > On Tue, 2018-06-26 at 14:28 -0700, Dan Williams wrote: > > > > > On Tue, Jun 26, 2018 at 2:23 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > > > On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: > > > > > > > On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > > > > > > > [..] > > > > > > > > When this dm change was made, the pmem driver supported DAX for both raw > > > > > > > > and memory modes (note: sector mode does not use the pmem driver). I > > > > > > > > think the issue was introduced when we dropped DAX support from raw > > > > > > > > mode. > > > > > > > > > > > > > > Still DAX with raw mode never really worked any way. It was also > > > > > > > something that was broken from day one. So what happens to someone who > > > > > > > happened to avoid all the problems with page-less DAX and enabled > > > > > > > device-mapper on top? That failure mode detail needs to be added to > > > > > > > this changelog if we want to propose this for -stable. > > > > > > > > > > > > My point is that the behavior should be consistent between pmem and > > > > > > device-mapper. When -o dax succeeds on a pmem, then it should succeed > > > > > > on a device-mapper on top of that pmem. > > > > > > > > > > > > Has the drop of dax support from raw mode made to -stable back to the > > > > > > baseline accepted 545ed20e6df6? It will introduce inconsistency, > > > > > > otherwise. > > > > > > > > > > That commit, 569d0365f571 "dax: require 'struct page' by default for > > > > > filesystem dax", has not been tagged for -stable. > > > > > > > > Then, Fixes tag should be set to 569d0365f571 to keep the behavior > > > > consistent. > > > > > > Sure, and the failure mode is...? I'm thinking the commit log should say: > > > > > > "Starting with commit 569d0365f571 "dax: require 'struct page' by > > > default for filesystem dax", dax is no longer supported for page-less > > > configurations. However, device-mapper sees the QUEUE_FLAG_DAX still > > > being set and falsely assumes that DAX is enabled, this leads to > > > <insert user visible failure mode details here>" > > > > Dan is correct that there is no user visible change for this. It is the right > > thing to do for consistency and sanity, but it doesn't actually have user > > visible behavior that needs to be backported to stable. > > > > Toshi is correct that this change is only for raw mode namespaces, not btt > > namespaces. > > > > I'll adjust the changelog and remove the stable flag for v5, and I'll add a > > Fixes: tag for patch 2. > > Hi Ross, > > Your patches look good. But I am still not clear about the Fixes & > stable handling. Talking about user visible behavior, I do not think we > had any issue until dax support was dropped from raw mode. Until then, > the pmem driver supported dax for all modes, and the check for > direct_access worked. I've staged the changes to send to Linus shortly. The first patch has: Fixes: 569d0365f571 ("dax: require 'struct page' by default for filesystem dax") Cc: stable@vger.kernel.org As that is the right thing to do given the other 2 patches are marked for stable. We don't want to have a stable kernel with the last 2 patches but not the first. Mike ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-28 17:48 ` Mike Snitzer @ 2018-06-28 17:59 ` Dan Williams 2018-06-28 18:01 ` Kani, Toshi 1 sibling, 0 replies; 27+ messages in thread From: Dan Williams @ 2018-06-28 17:59 UTC (permalink / raw) To: Mike Snitzer Cc: Kani, Toshi, ross.zwisler, dm-devel, linux-kernel, linux-xfs, linux-nvdimm, stable, linux-fsdevel On Thu, Jun 28, 2018 at 10:48 AM, Mike Snitzer <snitzer@redhat.com> wrote: > On Thu, Jun 28 2018 at 1:42pm -0400, > Kani, Toshi <toshi.kani@hpe.com> wrote: > >> On Tue, 2018-06-26 at 16:04 -0600, Ross Zwisler wrote: >> > On Tue, Jun 26, 2018 at 02:51:52PM -0700, Dan Williams wrote: >> > > On Tue, Jun 26, 2018 at 2:31 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: >> > > > On Tue, 2018-06-26 at 14:28 -0700, Dan Williams wrote: >> > > > > On Tue, Jun 26, 2018 at 2:23 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: >> > > > > > On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: >> > > > > > > On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: >> > > > > >> > > > > [..] >> > > > > > > > When this dm change was made, the pmem driver supported DAX for both raw >> > > > > > > > and memory modes (note: sector mode does not use the pmem driver). I >> > > > > > > > think the issue was introduced when we dropped DAX support from raw >> > > > > > > > mode. >> > > > > > > >> > > > > > > Still DAX with raw mode never really worked any way. It was also >> > > > > > > something that was broken from day one. So what happens to someone who >> > > > > > > happened to avoid all the problems with page-less DAX and enabled >> > > > > > > device-mapper on top? That failure mode detail needs to be added to >> > > > > > > this changelog if we want to propose this for -stable. >> > > > > > >> > > > > > My point is that the behavior should be consistent between pmem and >> > > > > > device-mapper. When -o dax succeeds on a pmem, then it should succeed >> > > > > > on a device-mapper on top of that pmem. >> > > > > > >> > > > > > Has the drop of dax support from raw mode made to -stable back to the >> > > > > > baseline accepted 545ed20e6df6? It will introduce inconsistency, >> > > > > > otherwise. >> > > > > >> > > > > That commit, 569d0365f571 "dax: require 'struct page' by default for >> > > > > filesystem dax", has not been tagged for -stable. >> > > > >> > > > Then, Fixes tag should be set to 569d0365f571 to keep the behavior >> > > > consistent. >> > > >> > > Sure, and the failure mode is...? I'm thinking the commit log should say: >> > > >> > > "Starting with commit 569d0365f571 "dax: require 'struct page' by >> > > default for filesystem dax", dax is no longer supported for page-less >> > > configurations. However, device-mapper sees the QUEUE_FLAG_DAX still >> > > being set and falsely assumes that DAX is enabled, this leads to >> > > <insert user visible failure mode details here>" >> > >> > Dan is correct that there is no user visible change for this. It is the right >> > thing to do for consistency and sanity, but it doesn't actually have user >> > visible behavior that needs to be backported to stable. >> > >> > Toshi is correct that this change is only for raw mode namespaces, not btt >> > namespaces. >> > >> > I'll adjust the changelog and remove the stable flag for v5, and I'll add a >> > Fixes: tag for patch 2. >> >> Hi Ross, >> >> Your patches look good. But I am still not clear about the Fixes & >> stable handling. Talking about user visible behavior, I do not think we >> had any issue until dax support was dropped from raw mode. Until then, >> the pmem driver supported dax for all modes, and the check for >> direct_access worked. > > I've staged the changes to send to Linus shortly. > > The first patch has: > > Fixes: 569d0365f571 ("dax: require 'struct page' by default for filesystem dax") > Cc: stable@vger.kernel.org > > As that is the right thing to do given the other 2 patches are marked > for stable. We don't want to have a stable kernel with the last 2 > patches but not the first. Ok, I'm still grumbling about the changelog being more clear about what the problem was, but let's just go with what you got. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-28 17:48 ` Mike Snitzer 2018-06-28 17:59 ` Dan Williams @ 2018-06-28 18:01 ` Kani, Toshi 1 sibling, 0 replies; 27+ messages in thread From: Kani, Toshi @ 2018-06-28 18:01 UTC (permalink / raw) To: snitzer Cc: linux-kernel, linux-xfs, stable, dan.j.williams, linux-nvdimm, linux-fsdevel, ross.zwisler, dm-devel On Thu, 2018-06-28 at 13:48 -0400, Mike Snitzer wrote: > On Thu, Jun 28 2018 at 1:42pm -0400, > Kani, Toshi <toshi.kani@hpe.com> wrote: > > > On Tue, 2018-06-26 at 16:04 -0600, Ross Zwisler wrote: > > > On Tue, Jun 26, 2018 at 02:51:52PM -0700, Dan Williams wrote: > > > > On Tue, Jun 26, 2018 at 2:31 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > > On Tue, 2018-06-26 at 14:28 -0700, Dan Williams wrote: > > > > > > On Tue, Jun 26, 2018 at 2:23 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > > > > On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: > > > > > > > > On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > > > > > > > > > [..] > > > > > > > > > When this dm change was made, the pmem driver supported DAX for both raw > > > > > > > > > and memory modes (note: sector mode does not use the pmem driver). I > > > > > > > > > think the issue was introduced when we dropped DAX support from raw > > > > > > > > > mode. > > > > > > > > > > > > > > > > Still DAX with raw mode never really worked any way. It was also > > > > > > > > something that was broken from day one. So what happens to someone who > > > > > > > > happened to avoid all the problems with page-less DAX and enabled > > > > > > > > device-mapper on top? That failure mode detail needs to be added to > > > > > > > > this changelog if we want to propose this for -stable. > > > > > > > > > > > > > > My point is that the behavior should be consistent between pmem and > > > > > > > device-mapper. When -o dax succeeds on a pmem, then it should succeed > > > > > > > on a device-mapper on top of that pmem. > > > > > > > > > > > > > > Has the drop of dax support from raw mode made to -stable back to the > > > > > > > baseline accepted 545ed20e6df6? It will introduce inconsistency, > > > > > > > otherwise. > > > > > > > > > > > > That commit, 569d0365f571 "dax: require 'struct page' by default for > > > > > > filesystem dax", has not been tagged for -stable. > > > > > > > > > > Then, Fixes tag should be set to 569d0365f571 to keep the behavior > > > > > consistent. > > > > > > > > Sure, and the failure mode is...? I'm thinking the commit log should say: > > > > > > > > "Starting with commit 569d0365f571 "dax: require 'struct page' by > > > > default for filesystem dax", dax is no longer supported for page-less > > > > configurations. However, device-mapper sees the QUEUE_FLAG_DAX still > > > > being set and falsely assumes that DAX is enabled, this leads to > > > > <insert user visible failure mode details here>" > > > > > > Dan is correct that there is no user visible change for this. It is the right > > > thing to do for consistency and sanity, but it doesn't actually have user > > > visible behavior that needs to be backported to stable. > > > > > > Toshi is correct that this change is only for raw mode namespaces, not btt > > > namespaces. > > > > > > I'll adjust the changelog and remove the stable flag for v5, and I'll add a > > > Fixes: tag for patch 2. > > > > Hi Ross, > > > > Your patches look good. But I am still not clear about the Fixes & > > stable handling. Talking about user visible behavior, I do not think we > > had any issue until dax support was dropped from raw mode. Until then, > > the pmem driver supported dax for all modes, and the check for > > direct_access worked. > > I've staged the changes to send to Linus shortly. > > The first patch has: > > Fixes: 569d0365f571 ("dax: require 'struct page' by default for filesystem dax") > Cc: stable@vger.kernel.org > > As that is the right thing to do given the other 2 patches are marked > for stable. We don't want to have a stable kernel with the last 2 > patches but not the first. Agreed. Technically, all 3 patches may have "Fixes: 569d0365f571 dax..", but I think having "Fixes 545ed20e6df6 dm.." for patch 2 & 3 provide a protection in case 569d0365f571 gets backported in future. For the series: Reviewed-by: Toshi Kani <toshi.kani@hpe.com> Thanks, -Toshi ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-28 17:42 ` Kani, Toshi 2018-06-28 17:48 ` Mike Snitzer @ 2018-06-28 19:04 ` Ross Zwisler 2018-06-28 19:40 ` Kani, Toshi 1 sibling, 1 reply; 27+ messages in thread From: Ross Zwisler @ 2018-06-28 19:04 UTC (permalink / raw) To: Kani, Toshi Cc: dan.j.williams, ross.zwisler, dm-devel, linux-kernel, linux-xfs, linux-nvdimm, stable, snitzer, linux-fsdevel On Thu, Jun 28, 2018 at 05:42:34PM +0000, Kani, Toshi wrote: > On Tue, 2018-06-26 at 16:04 -0600, Ross Zwisler wrote: > > On Tue, Jun 26, 2018 at 02:51:52PM -0700, Dan Williams wrote: > > > On Tue, Jun 26, 2018 at 2:31 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > On Tue, 2018-06-26 at 14:28 -0700, Dan Williams wrote: > > > > > On Tue, Jun 26, 2018 at 2:23 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > > > On Tue, 2018-06-26 at 14:02 -0700, Dan Williams wrote: > > > > > > > On Tue, Jun 26, 2018 at 1:54 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: > > > > > > > > > > [..] > > > > > > > > When this dm change was made, the pmem driver supported DAX for both raw > > > > > > > > and memory modes (note: sector mode does not use the pmem driver). I > > > > > > > > think the issue was introduced when we dropped DAX support from raw > > > > > > > > mode. > > > > > > > > > > > > > > Still DAX with raw mode never really worked any way. It was also > > > > > > > something that was broken from day one. So what happens to someone who > > > > > > > happened to avoid all the problems with page-less DAX and enabled > > > > > > > device-mapper on top? That failure mode detail needs to be added to > > > > > > > this changelog if we want to propose this for -stable. > > > > > > > > > > > > My point is that the behavior should be consistent between pmem and > > > > > > device-mapper. When -o dax succeeds on a pmem, then it should succeed > > > > > > on a device-mapper on top of that pmem. > > > > > > > > > > > > Has the drop of dax support from raw mode made to -stable back to the > > > > > > baseline accepted 545ed20e6df6? It will introduce inconsistency, > > > > > > otherwise. > > > > > > > > > > That commit, 569d0365f571 "dax: require 'struct page' by default for > > > > > filesystem dax", has not been tagged for -stable. > > > > > > > > Then, Fixes tag should be set to 569d0365f571 to keep the behavior > > > > consistent. > > > > > > Sure, and the failure mode is...? I'm thinking the commit log should say: > > > > > > "Starting with commit 569d0365f571 "dax: require 'struct page' by > > > default for filesystem dax", dax is no longer supported for page-less > > > configurations. However, device-mapper sees the QUEUE_FLAG_DAX still > > > being set and falsely assumes that DAX is enabled, this leads to > > > <insert user visible failure mode details here>" > > > > Dan is correct that there is no user visible change for this. It is the right > > thing to do for consistency and sanity, but it doesn't actually have user > > visible behavior that needs to be backported to stable. > > > > Toshi is correct that this change is only for raw mode namespaces, not btt > > namespaces. > > > > I'll adjust the changelog and remove the stable flag for v5, and I'll add a > > Fixes: tag for patch 2. > > Hi Ross, > > Your patches look good. But I am still not clear about the Fixes & > stable handling. Talking about user visible behavior, I do not think we > had any issue until dax support was dropped from raw mode. Until then, > the pmem driver supported dax for all modes, and the check for > direct_access worked. I agree that the fsdax + raw mode failure mode I mentioned in my cover letter only started when we restricted filesystem DAX to having struct page, but I think that the other failure mode, fsdax + some random block driver (I used brd) was present in DM from the beginning. In any case, I think both are fixed with the patches, and I think it's fine that all 3 get thrown at stable. Thanks, Mike, for the help. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-28 19:04 ` Ross Zwisler @ 2018-06-28 19:40 ` Kani, Toshi 0 siblings, 0 replies; 27+ messages in thread From: Kani, Toshi @ 2018-06-28 19:40 UTC (permalink / raw) To: ross.zwisler Cc: linux-kernel, linux-xfs, snitzer, stable, dan.j.williams, linux-nvdimm, linux-fsdevel, dm-devel On Thu, 2018-06-28 at 13:04 -0600, Ross Zwisler wrote: > On Thu, Jun 28, 2018 at 05:42:34PM +0000, Kani, Toshi wrote: > > On Tue, 2018-06-26 at 16:04 -0600, Ross Zwisler wrote: > > > On Tue, Jun 26, 2018 at 02:51:52PM -0700, Dan Williams wrote: > > > > On Tue, Jun 26, 2018 at 2:31 PM, Kani, Toshi <toshi.kani@hpe.com> wrote: : > > Your patches look good. But I am still not clear about the Fixes & > > stable handling. Talking about user visible behavior, I do not think we > > had any issue until dax support was dropped from raw mode. Until then, > > the pmem driver supported dax for all modes, and the check for > > direct_access worked. > > I agree that the fsdax + raw mode failure mode I mentioned in my cover letter > only started when we restricted filesystem DAX to having struct page, but I > think that the other failure mode, fsdax + some random block driver (I used > brd) was present in DM from the beginning. Ah, I see. > In any case, I think both are fixed with the patches, and I think it's fine > that all 3 get thrown at stable. Thanks, Mike, for the help. Yup, agreed. Thanks! -Toshi ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode 2018-06-26 18:58 ` Mike Snitzer 2018-06-26 19:07 ` Dan Williams @ 2018-06-26 19:11 ` Ross Zwisler 1 sibling, 0 replies; 27+ messages in thread From: Ross Zwisler @ 2018-06-26 19:11 UTC (permalink / raw) To: Mike Snitzer Cc: Dan Williams, Ross Zwisler, Toshi Kani, device-mapper development, linux-nvdimm, Linux Kernel Mailing List, stable, linux-xfs, linux-fsdevel On Tue, Jun 26, 2018 at 02:58:30PM -0400, Mike Snitzer wrote: > On Tue, Jun 26 2018 at 2:52pm -0400, > Dan Williams <dan.j.williams@intel.com> wrote: > > > On Tue, Jun 26, 2018 at 10:59 AM, Ross Zwisler > > <ross.zwisler@linux.intel.com> wrote: > > > QUEUE_FLAG_DAX is an indication that a given block device supports > > > filesystem DAX and should not be set for PMEM namespaces which are in "raw" > > > or "sector" modes. These namespaces lack struct page and are prevented > > > from participating in filesystem DAX. > > > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > Suggested-by: Mike Snitzer <snitzer@redhat.com> > > > Cc: stable@vger.kernel.org > > > > Why is this cc: stable? What is the user visible impact of this change > > especially given the requirement to validate QUEUE_FLAG_DAX with > > bdev_dax_supported()? Patch looks good, but it's just a cosmetic fixup > > afaics. > > This isn't cosmetic when you consider that stacking up a DM device is > looking at this flag to determine whether a table does or does _not_ > support DAX. > > So this patch, in conjunction with the other changes in the series, is > certainly something I'd consider appropriate for stable. > > Mike Because in patch 3 of this series we now use the full bdev_dax_supported() instead of just checking the queue flag in device_supports_dax(), I agree that this isn't strictly necessary for stable. device_supports_dax() will still notice that the raw/sector namespaces don't support DAX because bdev_dax_supported() will fail, and we'll end up doing the right thing and not setting QUEUE_FLAG_DAX on the DM device. I think maybe it's good to have in stable for completeness (and it's a very small change), but if we drop it from stable the code will still do the right thing AFAICT. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 2/3] dax: bdev_dax_supported() check for QUEUE_FLAG_DAX 2018-06-26 17:59 [PATCH v3 0/3] Fix DM DAX handling Ross Zwisler 2018-06-26 17:59 ` [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode Ross Zwisler @ 2018-06-26 17:59 ` Ross Zwisler 2018-06-26 17:59 ` [PATCH v3 3/3] dm: prevent DAX mounts if not supported Ross Zwisler 2018-06-26 18:48 ` [PATCH v3 0/3] Fix DM DAX handling Mike Snitzer 3 siblings, 0 replies; 27+ messages in thread From: Ross Zwisler @ 2018-06-26 17:59 UTC (permalink / raw) To: Toshi Kani, Mike Snitzer, dm-devel Cc: Ross Zwisler, linux-fsdevel, linux-kernel, linux-nvdimm, linux-xfs, stable Add an explicit check for QUEUE_FLAG_DAX to __bdev_dax_supported(). This is needed for DM configurations where the first element in the dm-linear or dm-stripe target supports DAX, but other elements do not. Without this check __bdev_dax_supported() will pass for such devices, letting a filesystem on that device mount with the DAX option. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> Suggested-by: Mike Snitzer <snitzer@redhat.com> Cc: stable@vger.kernel.org --- drivers/dax/super.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/dax/super.c b/drivers/dax/super.c index 903d9c473749..45276abf03aa 100644 --- a/drivers/dax/super.c +++ b/drivers/dax/super.c @@ -86,6 +86,7 @@ bool __bdev_dax_supported(struct block_device *bdev, int blocksize) { struct dax_device *dax_dev; bool dax_enabled = false; + struct request_queue *q; pgoff_t pgoff; int err, id; void *kaddr; @@ -99,6 +100,13 @@ bool __bdev_dax_supported(struct block_device *bdev, int blocksize) return false; } + q = bdev_get_queue(bdev); + if (!q || !blk_queue_dax(q)) { + pr_debug("%s: error: request queue doesn't support dax\n", + bdevname(bdev, buf)); + return false; + } + err = bdev_dax_pgoff(bdev, 0, PAGE_SIZE, &pgoff); if (err) { pr_debug("%s: error: unaligned partition for dax\n", -- 2.14.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 3/3] dm: prevent DAX mounts if not supported 2018-06-26 17:59 [PATCH v3 0/3] Fix DM DAX handling Ross Zwisler 2018-06-26 17:59 ` [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode Ross Zwisler 2018-06-26 17:59 ` [PATCH v3 2/3] dax: bdev_dax_supported() check for QUEUE_FLAG_DAX Ross Zwisler @ 2018-06-26 17:59 ` Ross Zwisler 2018-06-26 18:17 ` Mike Snitzer 2018-06-26 19:01 ` [PATCH v4 " Ross Zwisler 2018-06-26 18:48 ` [PATCH v3 0/3] Fix DM DAX handling Mike Snitzer 3 siblings, 2 replies; 27+ messages in thread From: Ross Zwisler @ 2018-06-26 17:59 UTC (permalink / raw) To: Toshi Kani, Mike Snitzer, dm-devel Cc: Ross Zwisler, linux-fsdevel, linux-kernel, linux-nvdimm, linux-xfs, stable Currently device_supports_dax() just checks to see if the QUEUE_FLAG_DAX flag is set on the device's request queue to decide whether or not the device supports filesystem DAX. Really we should be using bdev_dax_supported() like filesystems do at mount time. This performs other tests like checking to make sure the dax_direct_access() path works. Conditionally set QUEUE_FLAG_DAX on the DM device's request queue based on whether all underlying devices support DAX. Now that bdev_dax_supported() explicitly checks for this flag, this will ensure that filesystems built upon DM devices will only be able to mount with DAX if all underlying devices also support DAX. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> Fixes: commit 545ed20e6df6 ("dm: add infrastructure for DAX support") Cc: stable@vger.kernel.org --- drivers/md/dm-ioctl.c | 5 +++++ drivers/md/dm-table.c | 7 +++---- drivers/md/dm.c | 3 +-- include/linux/device-mapper.h | 5 +++++ 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index b810ea77e6b1..0055bdbee5b1 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1350,6 +1350,11 @@ static int table_load(struct file *filp, struct dm_ioctl *param, size_t param_si goto err_unlock_md_type; } + if (dm_table_supports_dax(t)) + blk_queue_flag_set(QUEUE_FLAG_DAX, md->queue); + else + blk_queue_flag_clear(QUEUE_FLAG_DAX, md->queue); + dm_unlock_md_type(md); /* stage inactive table */ diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 938766794c2e..c673b4a51fb2 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -885,12 +885,10 @@ EXPORT_SYMBOL_GPL(dm_table_set_type); static int device_supports_dax(struct dm_target *ti, struct dm_dev *dev, sector_t start, sector_t len, void *data) { - struct request_queue *q = bdev_get_queue(dev->bdev); - - return q && blk_queue_dax(q); + return bdev_dax_supported(dev->bdev, PAGE_SIZE); } -static bool dm_table_supports_dax(struct dm_table *t) +bool dm_table_supports_dax(struct dm_table *t) { struct dm_target *ti; unsigned i; @@ -909,6 +907,7 @@ static bool dm_table_supports_dax(struct dm_table *t) return true; } +EXPORT_SYMBOL_GPL(dm_table_supports_dax); static bool dm_table_does_not_support_partial_completion(struct dm_table *t); diff --git a/drivers/md/dm.c b/drivers/md/dm.c index e65429a29c06..bef5a3f243ed 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1056,8 +1056,7 @@ static long dm_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff, if (len < 1) goto out; nr_pages = min(len, nr_pages); - if (ti->type->direct_access) - ret = ti->type->direct_access(ti, pgoff, nr_pages, kaddr, pfn); + ret = ti->type->direct_access(ti, pgoff, nr_pages, kaddr, pfn); out: dm_put_live_table(md, srcu_idx); diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h index 6fb0808e87c8..45ea9e1f9af9 100644 --- a/include/linux/device-mapper.h +++ b/include/linux/device-mapper.h @@ -461,6 +461,11 @@ void dm_table_add_target_callbacks(struct dm_table *t, struct dm_target_callback */ void dm_table_set_type(struct dm_table *t, enum dm_queue_mode type); +/* + * Check to see if this target type and all table devices support DAX. + */ +bool dm_table_supports_dax(struct dm_table *t); + /* * Finally call this to make the table ready for use. */ -- 2.14.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3 3/3] dm: prevent DAX mounts if not supported 2018-06-26 17:59 ` [PATCH v3 3/3] dm: prevent DAX mounts if not supported Ross Zwisler @ 2018-06-26 18:17 ` Mike Snitzer 2018-06-26 19:01 ` [PATCH v4 " Ross Zwisler 1 sibling, 0 replies; 27+ messages in thread From: Mike Snitzer @ 2018-06-26 18:17 UTC (permalink / raw) To: Ross Zwisler Cc: Toshi Kani, dm-devel, linux-fsdevel, linux-kernel, linux-nvdimm, linux-xfs, stable On Tue, Jun 26 2018 at 1:59pm -0400, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > Currently device_supports_dax() just checks to see if the QUEUE_FLAG_DAX > flag is set on the device's request queue to decide whether or not the > device supports filesystem DAX. Really we should be using > bdev_dax_supported() like filesystems do at mount time. This performs > other tests like checking to make sure the dax_direct_access() path works. > > Conditionally set QUEUE_FLAG_DAX on the DM device's request queue based on > whether all underlying devices support DAX. Now that bdev_dax_supported() > explicitly checks for this flag, this will ensure that filesystems built > upon DM devices will only be able to mount with DAX if all underlying > devices also support DAX. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > Fixes: commit 545ed20e6df6 ("dm: add infrastructure for DAX support") > Cc: stable@vger.kernel.org > --- > drivers/md/dm-ioctl.c | 5 +++++ > drivers/md/dm-table.c | 7 +++---- > drivers/md/dm.c | 3 +-- > include/linux/device-mapper.h | 5 +++++ > 4 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > index b810ea77e6b1..0055bdbee5b1 100644 > --- a/drivers/md/dm-ioctl.c > +++ b/drivers/md/dm-ioctl.c > @@ -1350,6 +1350,11 @@ static int table_load(struct file *filp, struct dm_ioctl *param, size_t param_si > goto err_unlock_md_type; > } > > + if (dm_table_supports_dax(t)) > + blk_queue_flag_set(QUEUE_FLAG_DAX, md->queue); > + else > + blk_queue_flag_clear(QUEUE_FLAG_DAX, md->queue); > + > dm_unlock_md_type(md); > > /* stage inactive table */ This should occur in drivers/md/dm-table.c:dm_table_set_restrictions(). Which already has: if (dm_table_supports_dax(t)) blk_queue_flag_set(QUEUE_FLAG_DAX, q); So it just needs tweaking. > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c > index 938766794c2e..c673b4a51fb2 100644 > --- a/drivers/md/dm-table.c > +++ b/drivers/md/dm-table.c > @@ -885,12 +885,10 @@ EXPORT_SYMBOL_GPL(dm_table_set_type); > static int device_supports_dax(struct dm_target *ti, struct dm_dev *dev, > sector_t start, sector_t len, void *data) > { > - struct request_queue *q = bdev_get_queue(dev->bdev); > - > - return q && blk_queue_dax(q); > + return bdev_dax_supported(dev->bdev, PAGE_SIZE); > } > > -static bool dm_table_supports_dax(struct dm_table *t) > +bool dm_table_supports_dax(struct dm_table *t) > { > struct dm_target *ti; > unsigned i; > @@ -909,6 +907,7 @@ static bool dm_table_supports_dax(struct dm_table *t) > > return true; > } > +EXPORT_SYMBOL_GPL(dm_table_supports_dax); > > static bool dm_table_does_not_support_partial_completion(struct dm_table *t); > Which would mean you don't need to export dm_table_supports_dax(). > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index e65429a29c06..bef5a3f243ed 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -1056,8 +1056,7 @@ static long dm_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff, > if (len < 1) > goto out; > nr_pages = min(len, nr_pages); > - if (ti->type->direct_access) > - ret = ti->type->direct_access(ti, pgoff, nr_pages, kaddr, pfn); > + ret = ti->type->direct_access(ti, pgoff, nr_pages, kaddr, pfn); > > out: > dm_put_live_table(md, srcu_idx); > diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h > index 6fb0808e87c8..45ea9e1f9af9 100644 > --- a/include/linux/device-mapper.h > +++ b/include/linux/device-mapper.h > @@ -461,6 +461,11 @@ void dm_table_add_target_callbacks(struct dm_table *t, struct dm_target_callback > */ > void dm_table_set_type(struct dm_table *t, enum dm_queue_mode type); > > +/* > + * Check to see if this target type and all table devices support DAX. > + */ > +bool dm_table_supports_dax(struct dm_table *t); > + > /* > * Finally call this to make the table ready for use. > */ Nor do you need to publish the dm_table_supports_dax() interface via include/linux/device-mapper.h ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4 3/3] dm: prevent DAX mounts if not supported 2018-06-26 17:59 ` [PATCH v3 3/3] dm: prevent DAX mounts if not supported Ross Zwisler 2018-06-26 18:17 ` Mike Snitzer @ 2018-06-26 19:01 ` Ross Zwisler 1 sibling, 0 replies; 27+ messages in thread From: Ross Zwisler @ 2018-06-26 19:01 UTC (permalink / raw) To: Toshi Kani, Mike Snitzer, dm-devel Cc: Ross Zwisler, linux-fsdevel, linux-kernel, linux-nvdimm, linux-xfs, stable Currently device_supports_dax() just checks to see if the QUEUE_FLAG_DAX flag is set on the device's request queue to decide whether or not the device supports filesystem DAX. Really we should be using bdev_dax_supported() like filesystems do at mount time. This performs other tests like checking to make sure the dax_direct_access() path works. Conditionally set QUEUE_FLAG_DAX on the DM device's request queue based on whether all underlying devices support DAX. Now that bdev_dax_supported() explicitly checks for this flag, this will ensure that filesystems built upon DM devices will only be able to mount with DAX if all underlying devices also support DAX. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> Fixes: commit 545ed20e6df6 ("dm: add infrastructure for DAX support") Cc: stable@vger.kernel.org --- Changes in v4: * Set/clear QUEUE_FLAG_DAX in dm_table_set_restrictions(). (Mike) --- drivers/md/dm-table.c | 7 ++++--- drivers/md/dm.c | 3 +-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 938766794c2e..3d0e2c198f06 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -885,9 +885,7 @@ EXPORT_SYMBOL_GPL(dm_table_set_type); static int device_supports_dax(struct dm_target *ti, struct dm_dev *dev, sector_t start, sector_t len, void *data) { - struct request_queue *q = bdev_get_queue(dev->bdev); - - return q && blk_queue_dax(q); + return bdev_dax_supported(dev->bdev, PAGE_SIZE); } static bool dm_table_supports_dax(struct dm_table *t) @@ -1907,6 +1905,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q, if (dm_table_supports_dax(t)) blk_queue_flag_set(QUEUE_FLAG_DAX, q); + else + blk_queue_flag_clear(QUEUE_FLAG_DAX, q); + if (dm_table_supports_dax_write_cache(t)) dax_write_cache(t->md->dax_dev, true); diff --git a/drivers/md/dm.c b/drivers/md/dm.c index e65429a29c06..bef5a3f243ed 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1056,8 +1056,7 @@ static long dm_dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff, if (len < 1) goto out; nr_pages = min(len, nr_pages); - if (ti->type->direct_access) - ret = ti->type->direct_access(ti, pgoff, nr_pages, kaddr, pfn); + ret = ti->type->direct_access(ti, pgoff, nr_pages, kaddr, pfn); out: dm_put_live_table(md, srcu_idx); -- 2.14.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3 0/3] Fix DM DAX handling 2018-06-26 17:59 [PATCH v3 0/3] Fix DM DAX handling Ross Zwisler ` (2 preceding siblings ...) 2018-06-26 17:59 ` [PATCH v3 3/3] dm: prevent DAX mounts if not supported Ross Zwisler @ 2018-06-26 18:48 ` Mike Snitzer 3 siblings, 0 replies; 27+ messages in thread From: Mike Snitzer @ 2018-06-26 18:48 UTC (permalink / raw) To: Ross Zwisler Cc: Toshi Kani, dm-devel, linux-fsdevel, linux-kernel, linux-nvdimm, linux-xfs On Tue, Jun 26 2018 at 1:59pm -0400, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > This series fixes a few issues that I found with DM's handling of DAX > devices. Here are some of the issues I found: > > * We can create a dm-stripe or dm-linear device which is made up of an > fsdax PMEM namespace and a raw PMEM namespace but which can hold a > filesystem mounted with the -o dax mount option. DAX operations to > the raw PMEM namespace part lack struct page and can fail in > interesting/unexpected ways when doing things like fork(), examining > memory with gdb, etc. > > * We can create a dm-stripe or dm-linear device which is made up of an > fsdax PMEM namespace and a BRD ramdisk which can hold a filesystem > mounted with the -o dax mount option. All I/O to this filesystem > will fail. > > --- > > Changes since v2: > * Only set QUEUE_FLAG_DAX for fsdax mode PMEM namespaces. (Mike) > * Check for QUEUE_FLAG_DAX in __bdev_dax_supported(). (Mike) > * Get rid of DM_TYPE_DAX_BIO_BASED reworks. (Mike) > * Dropped the first 2 prep patches of v2 since they were merged for > v4.18-rc1. (Thanks, Darrick!) > > --- > > Mike, can you take this series through your tree? > > Personally I think this should be treated as a bug fix and merged in the > v4.18-rc* series. I'd be fine with staging it for 4.18. Only question is whether others are fine with the dax patch (and me being the one to get it to Linus)? I already replied to the 3rd patch with some feedback for v4 (but I can also take care of those changes if I'm the one to stage these changes). Maybe if Dan and/or others could provide their review for both the dax and pmem patches? If I can get review on those I'll get the series staged for Linus to pull this week. Thanks, Mike ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-06-28 19:40 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-26 17:59 [PATCH v3 0/3] Fix DM DAX handling Ross Zwisler 2018-06-26 17:59 ` [PATCH v3 1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode Ross Zwisler 2018-06-26 18:52 ` Dan Williams 2018-06-26 18:58 ` Mike Snitzer 2018-06-26 19:07 ` Dan Williams 2018-06-26 19:12 ` Ross Zwisler 2018-06-26 19:13 ` Mike Snitzer 2018-06-26 19:19 ` Dan Williams 2018-06-26 20:54 ` Kani, Toshi 2018-06-26 21:02 ` Dan Williams 2018-06-26 21:23 ` Kani, Toshi 2018-06-26 21:28 ` Dan Williams 2018-06-26 21:31 ` Kani, Toshi 2018-06-26 21:51 ` Dan Williams 2018-06-26 22:04 ` Ross Zwisler 2018-06-28 17:42 ` Kani, Toshi 2018-06-28 17:48 ` Mike Snitzer 2018-06-28 17:59 ` Dan Williams 2018-06-28 18:01 ` Kani, Toshi 2018-06-28 19:04 ` Ross Zwisler 2018-06-28 19:40 ` Kani, Toshi 2018-06-26 19:11 ` Ross Zwisler 2018-06-26 17:59 ` [PATCH v3 2/3] dax: bdev_dax_supported() check for QUEUE_FLAG_DAX Ross Zwisler 2018-06-26 17:59 ` [PATCH v3 3/3] dm: prevent DAX mounts if not supported Ross Zwisler 2018-06-26 18:17 ` Mike Snitzer 2018-06-26 19:01 ` [PATCH v4 " Ross Zwisler 2018-06-26 18:48 ` [PATCH v3 0/3] Fix DM DAX handling Mike Snitzer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).