diff mbox

[v3,1/3] pmem: only set QUEUE_FLAG_DAX for fsdax mode

Message ID 20180626175932.8899-2-ross.zwisler@linux.intel.com (mailing list archive)
State Deferred, archived
Headers show

Commit Message

Ross Zwisler June 26, 2018, 5:59 p.m. UTC
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(-)

Comments

Dan Williams June 26, 2018, 6:52 p.m. UTC | #1
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Snitzer June 26, 2018, 6:58 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams June 26, 2018, 7:07 p.m. UTC | #3
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ross Zwisler June 26, 2018, 7:11 p.m. UTC | #4
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ross Zwisler June 26, 2018, 7:12 p.m. UTC | #5
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Snitzer June 26, 2018, 7:13 p.m. UTC | #6
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")
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams June 26, 2018, 7:19 p.m. UTC | #7
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kani, Toshi June 26, 2018, 8:54 p.m. UTC | #8
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
Dan Williams June 26, 2018, 9:02 p.m. UTC | #9
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kani, Toshi June 26, 2018, 9:23 p.m. UTC | #10
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
Dan Williams June 26, 2018, 9:28 p.m. UTC | #11
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kani, Toshi June 26, 2018, 9:31 p.m. UTC | #12
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
Dan Williams June 26, 2018, 9:51 p.m. UTC | #13
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>"
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ross Zwisler June 26, 2018, 10:04 p.m. UTC | #14
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kani, Toshi June 28, 2018, 5:42 p.m. UTC | #15
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
Mike Snitzer June 28, 2018, 5:48 p.m. UTC | #16
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
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams June 28, 2018, 5:59 p.m. UTC | #17
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kani, Toshi June 28, 2018, 6:01 p.m. UTC | #18
T24gVGh1LCAyMDE4LTA2LTI4IGF0IDEzOjQ4IC0wNDAwLCBNaWtlIFNuaXR6ZXIgd3JvdGU6DQo+
IE9uIFRodSwgSnVuIDI4IDIwMTggYXQgIDE6NDJwbSAtMDQwMCwNCj4gS2FuaSwgVG9zaGkgPHRv
c2hpLmthbmlAaHBlLmNvbT4gd3JvdGU6DQo+IA0KPiA+IE9uIFR1ZSwgMjAxOC0wNi0yNiBhdCAx
NjowNCAtMDYwMCwgUm9zcyBad2lzbGVyIHdyb3RlOg0KPiA+ID4gT24gVHVlLCBKdW4gMjYsIDIw
MTggYXQgMDI6NTE6NTJQTSAtMDcwMCwgRGFuIFdpbGxpYW1zIHdyb3RlOg0KPiA+ID4gPiBPbiBU
dWUsIEp1biAyNiwgMjAxOCBhdCAyOjMxIFBNLCBLYW5pLCBUb3NoaSA8dG9zaGkua2FuaUBocGUu
Y29tPiB3cm90ZToNCj4gPiA+ID4gPiBPbiBUdWUsIDIwMTgtMDYtMjYgYXQgMTQ6MjggLTA3MDAs
IERhbiBXaWxsaWFtcyB3cm90ZToNCj4gPiA+ID4gPiA+IE9uIFR1ZSwgSnVuIDI2LCAyMDE4IGF0
IDI6MjMgUE0sIEthbmksIFRvc2hpIDx0b3NoaS5rYW5pQGhwZS5jb20+IHdyb3RlOg0KPiA+ID4g
PiA+ID4gPiBPbiBUdWUsIDIwMTgtMDYtMjYgYXQgMTQ6MDIgLTA3MDAsIERhbiBXaWxsaWFtcyB3
cm90ZToNCj4gPiA+ID4gPiA+ID4gPiBPbiBUdWUsIEp1biAyNiwgMjAxOCBhdCAxOjU0IFBNLCBL
YW5pLCBUb3NoaSA8dG9zaGkua2FuaUBocGUuY29tPiB3cm90ZToNCj4gPiA+ID4gPiA+IA0KPiA+
ID4gPiA+ID4gWy4uXQ0KPiA+ID4gPiA+ID4gPiA+ID4gV2hlbiB0aGlzIGRtIGNoYW5nZSB3YXMg
bWFkZSwgdGhlIHBtZW0gZHJpdmVyIHN1cHBvcnRlZCBEQVggZm9yIGJvdGggcmF3DQo+ID4gPiA+
ID4gPiA+ID4gPiBhbmQgbWVtb3J5IG1vZGVzIChub3RlOiBzZWN0b3IgbW9kZSBkb2VzIG5vdCB1
c2UgdGhlIHBtZW0gZHJpdmVyKS4gIEkNCj4gPiA+ID4gPiA+ID4gPiA+IHRoaW5rIHRoZSBpc3N1
ZSB3YXMgaW50cm9kdWNlZCB3aGVuIHdlIGRyb3BwZWQgREFYIHN1cHBvcnQgZnJvbSByYXcNCj4g
PiA+ID4gPiA+ID4gPiA+IG1vZGUuDQo+ID4gPiA+ID4gPiA+ID4gDQo+ID4gPiA+ID4gPiA+ID4g
U3RpbGwgREFYIHdpdGggcmF3IG1vZGUgbmV2ZXIgcmVhbGx5IHdvcmtlZCBhbnkgd2F5LiBJdCB3
YXMgYWxzbw0KPiA+ID4gPiA+ID4gPiA+IHNvbWV0aGluZyB0aGF0IHdhcyBicm9rZW4gZnJvbSBk
YXkgb25lLiBTbyB3aGF0IGhhcHBlbnMgdG8gc29tZW9uZSB3aG8NCj4gPiA+ID4gPiA+ID4gPiBo
YXBwZW5lZCB0byBhdm9pZCBhbGwgdGhlIHByb2JsZW1zIHdpdGggcGFnZS1sZXNzIERBWCBhbmQg
ZW5hYmxlZA0KPiA+ID4gPiA+ID4gPiA+IGRldmljZS1tYXBwZXIgb24gdG9wPyBUaGF0IGZhaWx1
cmUgbW9kZSBkZXRhaWwgbmVlZHMgdG8gYmUgYWRkZWQgdG8NCj4gPiA+ID4gPiA+ID4gPiB0aGlz
IGNoYW5nZWxvZyBpZiB3ZSB3YW50IHRvIHByb3Bvc2UgdGhpcyBmb3IgLXN0YWJsZS4NCj4gPiA+
ID4gPiA+ID4gDQo+ID4gPiA+ID4gPiA+IE15IHBvaW50IGlzIHRoYXQgdGhlIGJlaGF2aW9yIHNo
b3VsZCBiZSBjb25zaXN0ZW50IGJldHdlZW4gcG1lbSBhbmQNCj4gPiA+ID4gPiA+ID4gZGV2aWNl
LW1hcHBlci4gIFdoZW4gLW8gZGF4IHN1Y2NlZWRzIG9uIGEgcG1lbSwgdGhlbiBpdCBzaG91bGQg
c3VjY2VlZA0KPiA+ID4gPiA+ID4gPiBvbiBhIGRldmljZS1tYXBwZXIgb24gdG9wIG9mIHRoYXQg
cG1lbS4NCj4gPiA+ID4gPiA+ID4gDQo+ID4gPiA+ID4gPiA+IEhhcyB0aGUgZHJvcCBvZiBkYXgg
c3VwcG9ydCBmcm9tIHJhdyBtb2RlIG1hZGUgdG8gLXN0YWJsZSBiYWNrIHRvIHRoZQ0KPiA+ID4g
PiA+ID4gPiBiYXNlbGluZSBhY2NlcHRlZCA1NDVlZDIwZTZkZjY/ICBJdCB3aWxsIGludHJvZHVj
ZSBpbmNvbnNpc3RlbmN5LA0KPiA+ID4gPiA+ID4gPiBvdGhlcndpc2UuDQo+ID4gPiA+ID4gPiAN
Cj4gPiA+ID4gPiA+IFRoYXQgY29tbWl0LCA1NjlkMDM2NWY1NzEgImRheDogcmVxdWlyZSAnc3Ry
dWN0IHBhZ2UnIGJ5IGRlZmF1bHQgZm9yDQo+ID4gPiA+ID4gPiBmaWxlc3lzdGVtIGRheCIsIGhh
cyBub3QgYmVlbiB0YWdnZWQgZm9yIC1zdGFibGUuDQo+ID4gPiA+ID4gDQo+ID4gPiA+ID4gVGhl
biwgRml4ZXMgdGFnIHNob3VsZCBiZSBzZXQgdG8gNTY5ZDAzNjVmNTcxIHRvIGtlZXAgdGhlIGJl
aGF2aW9yDQo+ID4gPiA+ID4gY29uc2lzdGVudC4NCj4gPiA+ID4gDQo+ID4gPiA+IFN1cmUsIGFu
ZCB0aGUgZmFpbHVyZSBtb2RlIGlzLi4uPyBJJ20gdGhpbmtpbmcgdGhlIGNvbW1pdCBsb2cgc2hv
dWxkIHNheToNCj4gPiA+ID4gDQo+ID4gPiA+ICJTdGFydGluZyB3aXRoIGNvbW1pdCA1NjlkMDM2
NWY1NzEgImRheDogcmVxdWlyZSAnc3RydWN0IHBhZ2UnIGJ5DQo+ID4gPiA+IGRlZmF1bHQgZm9y
IGZpbGVzeXN0ZW0gZGF4IiwgZGF4IGlzIG5vIGxvbmdlciBzdXBwb3J0ZWQgZm9yIHBhZ2UtbGVz
cw0KPiA+ID4gPiBjb25maWd1cmF0aW9ucy4gSG93ZXZlciwgZGV2aWNlLW1hcHBlciBzZWVzIHRo
ZSBRVUVVRV9GTEFHX0RBWCBzdGlsbA0KPiA+ID4gPiBiZWluZyBzZXQgYW5kIGZhbHNlbHkgYXNz
dW1lcyB0aGF0IERBWCBpcyBlbmFibGVkLCB0aGlzIGxlYWRzIHRvDQo+ID4gPiA+IDxpbnNlcnQg
dXNlciB2aXNpYmxlIGZhaWx1cmUgbW9kZSBkZXRhaWxzIGhlcmU+Ig0KPiA+ID4gDQo+ID4gPiBE
YW4gaXMgY29ycmVjdCB0aGF0IHRoZXJlIGlzIG5vIHVzZXIgdmlzaWJsZSBjaGFuZ2UgZm9yIHRo
aXMuICBJdCBpcyB0aGUgcmlnaHQNCj4gPiA+IHRoaW5nIHRvIGRvIGZvciBjb25zaXN0ZW5jeSBh
bmQgc2FuaXR5LCBidXQgaXQgZG9lc24ndCBhY3R1YWxseSBoYXZlIHVzZXINCj4gPiA+IHZpc2li
bGUgYmVoYXZpb3IgdGhhdCBuZWVkcyB0byBiZSBiYWNrcG9ydGVkIHRvIHN0YWJsZS4NCj4gPiA+
IA0KPiA+ID4gVG9zaGkgaXMgY29ycmVjdCB0aGF0IHRoaXMgY2hhbmdlIGlzIG9ubHkgZm9yIHJh
dyBtb2RlIG5hbWVzcGFjZXMsIG5vdCBidHQNCj4gPiA+IG5hbWVzcGFjZXMuDQo+ID4gPiANCj4g
PiA+IEknbGwgYWRqdXN0IHRoZSBjaGFuZ2Vsb2cgYW5kIHJlbW92ZSB0aGUgc3RhYmxlIGZsYWcg
Zm9yIHY1LCBhbmQgSSdsbCBhZGQgYQ0KPiA+ID4gRml4ZXM6IHRhZyBmb3IgcGF0Y2ggMi4NCj4g
PiANCj4gPiBIaSBSb3NzLA0KPiA+IA0KPiA+IFlvdXIgcGF0Y2hlcyBsb29rIGdvb2QuICBCdXQg
SSBhbSBzdGlsbCBub3QgY2xlYXIgYWJvdXQgdGhlIEZpeGVzICYNCj4gPiBzdGFibGUgaGFuZGxp
bmcuICBUYWxraW5nIGFib3V0IHVzZXIgdmlzaWJsZSBiZWhhdmlvciwgSSBkbyBub3QgdGhpbmsg
d2UNCj4gPiBoYWQgYW55IGlzc3VlIHVudGlsIGRheCBzdXBwb3J0IHdhcyBkcm9wcGVkIGZyb20g
cmF3IG1vZGUuICBVbnRpbCB0aGVuLA0KPiA+IHRoZSBwbWVtIGRyaXZlciBzdXBwb3J0ZWQgZGF4
IGZvciBhbGwgbW9kZXMsIGFuZCB0aGUgY2hlY2sgZm9yDQo+ID4gZGlyZWN0X2FjY2VzcyB3b3Jr
ZWQuICAgIA0KPiANCj4gSSd2ZSBzdGFnZWQgdGhlIGNoYW5nZXMgdG8gc2VuZCB0byBMaW51cyBz
aG9ydGx5Lg0KPiANCj4gVGhlIGZpcnN0IHBhdGNoIGhhczoNCj4gDQo+IEZpeGVzOiA1NjlkMDM2
NWY1NzEgKCJkYXg6IHJlcXVpcmUgJ3N0cnVjdCBwYWdlJyBieSBkZWZhdWx0IGZvciBmaWxlc3lz
dGVtIGRheCIpDQo+IENjOiBzdGFibGVAdmdlci5rZXJuZWwub3JnDQo+IA0KPiBBcyB0aGF0IGlz
IHRoZSByaWdodCB0aGluZyB0byBkbyBnaXZlbiB0aGUgb3RoZXIgMiBwYXRjaGVzIGFyZSBtYXJr
ZWQNCj4gZm9yIHN0YWJsZS4gIFdlIGRvbid0IHdhbnQgdG8gaGF2ZSBhIHN0YWJsZSBrZXJuZWwg
d2l0aCB0aGUgbGFzdCAyDQo+IHBhdGNoZXMgYnV0IG5vdCB0aGUgZmlyc3QuDQoNCkFncmVlZC4N
Cg0KVGVjaG5pY2FsbHksIGFsbCAzIHBhdGNoZXMgbWF5IGhhdmUgIkZpeGVzOiA1NjlkMDM2NWY1
NzEgZGF4Li4iLCBidXQgSQ0KdGhpbmsgaGF2aW5nICJGaXhlcyA1NDVlZDIwZTZkZjYgZG0uLiIg
Zm9yIHBhdGNoIDIgJiAzIHByb3ZpZGUgYQ0KcHJvdGVjdGlvbiBpbiBjYXNlIDU2OWQwMzY1ZjU3
MSBnZXRzIGJhY2twb3J0ZWQgaW4gZnV0dXJlLg0KDQpGb3IgdGhlIHNlcmllczoNClJldmlld2Vk
LWJ5OiBUb3NoaSBLYW5pIDx0b3NoaS5rYW5pQGhwZS5jb20+DQoNClRoYW5rcywNCi1Ub3NoaQ0K
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ross Zwisler June 28, 2018, 7:04 p.m. UTC | #19
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kani, Toshi June 28, 2018, 7:40 p.m. UTC | #20
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
diff mbox

Patch

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);