| Message ID | 20200903115549.17845-1-colyli@suse.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | dax: fix for do not print error message for non-persistent memory block device | expand |
> -----Original Message----- > From: Coly Li <colyli@suse.de> > Sent: Thursday, September 3, 2020 7:56 PM > To: linux-nvdimm@lists.01.org > Cc: dm-devel@redhat.com; Coly Li <colyli@suse.de>; Adrian Huang12 > <ahuang12@lenovo.com>; Ira Weiny <ira.weiny@intel.com>; Jan Kara > <jack@suse.com>; Mike Snitzer <snitzer@redhat.com>; Pankaj Gupta > <pankaj.gupta.linux@gmail.com>; Vishal Verma <vishal.l.verma@intel.com> > Subject: [External] [PATCH] dax: fix for do not print error message for non- > persistent memory block device > > When calling __generic_fsdax_supported(), a dax-unsupported device may not > have dax_dev as NULL, e.g. the dax related code block is not enabled by Kconfig. > > Therefore in __generic_fsdax_supported(), to check whether a device supports > DAX or not, the following order should be performed, > - If dax_dev pointer is NULL, it means the device driver explicitly > announce it doesn't support DAX. Then it is OK to directly return > false from __generic_fsdax_supported(). > - If dax_dev pointer is NOT NULL, it might be because the driver doesn't > support DAX and not explicitly initialize related data structure. Then > bdev_dax_supported() should be called for further check. > > IMHO if device driver desn't explicitly set its dax_dev pointer to NULL, this is not > a bug. Calling bdev_dax_supported() makes sure they can be recognized as dax- > unsupported eventually. > > This patch does the following change for the above purpose, > - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { > + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { > > > Fixes: c2affe920b0e ("dax: do not print error message for non-persistent > memory block device") > Signed-off-by: Coly Li <colyli@suse.de> The dax error messages ("dm-X: error: dax access failed (-95)") are gone away when executing the command 'lvm2-testsuite --only activate-minor'. Reviewed-and-Tested-by: Adrian Huang <ahuang12@lenovo.com> > Cc: Adrian Huang <ahuang12@lenovo.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Jan Kara <jack@suse.com> > Cc: Mike Snitzer <snitzer@redhat.com> > Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > --- > drivers/dax/super.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dax/super.c b/drivers/dax/super.c index > 32642634c1bb..e5767c83ea23 100644 > --- a/drivers/dax/super.c > +++ b/drivers/dax/super.c > @@ -100,7 +100,7 @@ bool __generic_fsdax_supported(struct dax_device > *dax_dev, > return false; > } > > - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { > + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { > pr_debug("%s: error: dax unsupported by block device\n", > bdevname(bdev, buf)); > return false; > -- > 2.26.2 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Sep 03, 2020 at 07:55:49PM +0800, Coly Li wrote: > When calling __generic_fsdax_supported(), a dax-unsupported device may > not have dax_dev as NULL, e.g. the dax related code block is not enabled > by Kconfig. > > Therefore in __generic_fsdax_supported(), to check whether a device > supports DAX or not, the following order should be performed, > - If dax_dev pointer is NULL, it means the device driver explicitly > announce it doesn't support DAX. Then it is OK to directly return > false from __generic_fsdax_supported(). > - If dax_dev pointer is NOT NULL, it might be because the driver doesn't > support DAX and not explicitly initialize related data structure. Then > bdev_dax_supported() should be called for further check. > > IMHO if device driver desn't explicitly set its dax_dev pointer to NULL, > this is not a bug. Calling bdev_dax_supported() makes sure they can be > recognized as dax-unsupported eventually. > > This patch does the following change for the above purpose, > - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { > + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { > > > Fixes: c2affe920b0e ("dax: do not print error message for non-persistent memory block device") > Signed-off-by: Coly Li <colyli@suse.de> I hate to do this because I realize this is a bug which people really need fixed. However, shouldn't we also check (!dax_dev || !bdev_dax_supported()) as the _first_ check in __generic_fsdax_supported()? It seems like the other pr_info's could also be called when DAX is not supported and we probably don't want them to be? Perhaps that should be a follow on patch though. So... As a direct fix to c2affe920b0e Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Cc: Adrian Huang <ahuang12@lenovo.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Jan Kara <jack@suse.com> > Cc: Mike Snitzer <snitzer@redhat.com> > Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > --- > drivers/dax/super.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dax/super.c b/drivers/dax/super.c > index 32642634c1bb..e5767c83ea23 100644 > --- a/drivers/dax/super.c > +++ b/drivers/dax/super.c > @@ -100,7 +100,7 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev, > return false; > } > > - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { > + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { > pr_debug("%s: error: dax unsupported by block device\n", > bdevname(bdev, buf)); > return false; > -- > 2.26.2 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 2020/9/4 00:06, Ira Weiny wrote: > On Thu, Sep 03, 2020 at 07:55:49PM +0800, Coly Li wrote: >> When calling __generic_fsdax_supported(), a dax-unsupported device may >> not have dax_dev as NULL, e.g. the dax related code block is not enabled >> by Kconfig. >> >> Therefore in __generic_fsdax_supported(), to check whether a device >> supports DAX or not, the following order should be performed, >> - If dax_dev pointer is NULL, it means the device driver explicitly >> announce it doesn't support DAX. Then it is OK to directly return >> false from __generic_fsdax_supported(). >> - If dax_dev pointer is NOT NULL, it might be because the driver doesn't >> support DAX and not explicitly initialize related data structure. Then >> bdev_dax_supported() should be called for further check. >> >> IMHO if device driver desn't explicitly set its dax_dev pointer to NULL, >> this is not a bug. Calling bdev_dax_supported() makes sure they can be >> recognized as dax-unsupported eventually. >> >> This patch does the following change for the above purpose, >> - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { >> + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { >> >> >> Fixes: c2affe920b0e ("dax: do not print error message for non-persistent memory block device") >> Signed-off-by: Coly Li <colyli@suse.de> > > I hate to do this because I realize this is a bug which people really need > fixed. > > However, shouldn't we also check (!dax_dev || !bdev_dax_supported()) as the > _first_ check in __generic_fsdax_supported()? > > It seems like the other pr_info's could also be called when DAX is not > supported and we probably don't want them to be? > > Perhaps that should be a follow on patch though. So... I am not author of c2affe920b0e, but I guess it was because bdev_dax_supported() needed blocksize, so blocksize should pass previous checks firstly to make sure bdev_dax_supported() has a correct blocksize to check. > > As a direct fix to c2affe920b0e > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> Thanks. Coly Li > >> Cc: Adrian Huang <ahuang12@lenovo.com> >> Cc: Ira Weiny <ira.weiny@intel.com> >> Cc: Jan Kara <jack@suse.com> >> Cc: Mike Snitzer <snitzer@redhat.com> >> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> >> Cc: Vishal Verma <vishal.l.verma@intel.com> >> --- >> drivers/dax/super.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/dax/super.c b/drivers/dax/super.c >> index 32642634c1bb..e5767c83ea23 100644 >> --- a/drivers/dax/super.c >> +++ b/drivers/dax/super.c >> @@ -100,7 +100,7 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev, >> return false; >> } >> >> - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { >> + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { >> pr_debug("%s: error: dax unsupported by block device\n", >> bdevname(bdev, buf)); >> return false; >> -- >> 2.26.2 >> -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
But it should be moved prior to the two bdev_dax_pgoff() checks right? Else a misaligned partition on a dax unsupported block device can print the below messages. kernel: sda1: error: unaligned partition for dax kernel: sda2: error: unaligned partition for dax kernel: sda3: error: unaligned partition for dax Reviewed-by: John Pittman <jpittman@redhat.com> On Thu, Sep 3, 2020 at 12:12 PM Coly Li <colyli@suse.de> wrote: > > On 2020/9/4 00:06, Ira Weiny wrote: > > On Thu, Sep 03, 2020 at 07:55:49PM +0800, Coly Li wrote: > >> When calling __generic_fsdax_supported(), a dax-unsupported device may > >> not have dax_dev as NULL, e.g. the dax related code block is not enabled > >> by Kconfig. > >> > >> Therefore in __generic_fsdax_supported(), to check whether a device > >> supports DAX or not, the following order should be performed, > >> - If dax_dev pointer is NULL, it means the device driver explicitly > >> announce it doesn't support DAX. Then it is OK to directly return > >> false from __generic_fsdax_supported(). > >> - If dax_dev pointer is NOT NULL, it might be because the driver doesn't > >> support DAX and not explicitly initialize related data structure. Then > >> bdev_dax_supported() should be called for further check. > >> > >> IMHO if device driver desn't explicitly set its dax_dev pointer to NULL, > >> this is not a bug. Calling bdev_dax_supported() makes sure they can be > >> recognized as dax-unsupported eventually. > >> > >> This patch does the following change for the above purpose, > >> - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { > >> + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { > >> > >> > >> Fixes: c2affe920b0e ("dax: do not print error message for non-persistent memory block device") > >> Signed-off-by: Coly Li <colyli@suse.de> > > > > I hate to do this because I realize this is a bug which people really need > > fixed. > > > > However, shouldn't we also check (!dax_dev || !bdev_dax_supported()) as the > > _first_ check in __generic_fsdax_supported()? > > > > It seems like the other pr_info's could also be called when DAX is not > > supported and we probably don't want them to be? > > > > Perhaps that should be a follow on patch though. So... > > I am not author of c2affe920b0e, but I guess it was because > bdev_dax_supported() needed blocksize, so blocksize should pass previous > checks firstly to make sure bdev_dax_supported() has a correct blocksize > to check. > > > > > As a direct fix to c2affe920b0e > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > Thanks. > > Coly Li > > > > > >> Cc: Adrian Huang <ahuang12@lenovo.com> > >> Cc: Ira Weiny <ira.weiny@intel.com> > >> Cc: Jan Kara <jack@suse.com> > >> Cc: Mike Snitzer <snitzer@redhat.com> > >> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> > >> Cc: Vishal Verma <vishal.l.verma@intel.com> > >> --- > >> drivers/dax/super.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/dax/super.c b/drivers/dax/super.c > >> index 32642634c1bb..e5767c83ea23 100644 > >> --- a/drivers/dax/super.c > >> +++ b/drivers/dax/super.c > >> @@ -100,7 +100,7 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev, > >> return false; > >> } > >> > >> - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { > >> + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { > >> pr_debug("%s: error: dax unsupported by block device\n", > >> bdevname(bdev, buf)); > >> return false; > >> -- > >> 2.26.2 > >> > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 2020/9/11 04:29, John Pittman wrote: > But it should be moved prior to the two bdev_dax_pgoff() checks right? > Else a misaligned partition on a dax unsupported block device can > print the below messages. > > kernel: sda1: error: unaligned partition for dax > kernel: sda2: error: unaligned partition for dax > kernel: sda3: error: unaligned partition for dax > Aha, yes you are right, I agree with you. Coly Li > Reviewed-by: John Pittman <jpittman@redhat.com> > > On Thu, Sep 3, 2020 at 12:12 PM Coly Li <colyli@suse.de> wrote: >> >> On 2020/9/4 00:06, Ira Weiny wrote: >>> On Thu, Sep 03, 2020 at 07:55:49PM +0800, Coly Li wrote: >>>> When calling __generic_fsdax_supported(), a dax-unsupported device may >>>> not have dax_dev as NULL, e.g. the dax related code block is not enabled >>>> by Kconfig. >>>> >>>> Therefore in __generic_fsdax_supported(), to check whether a device >>>> supports DAX or not, the following order should be performed, >>>> - If dax_dev pointer is NULL, it means the device driver explicitly >>>> announce it doesn't support DAX. Then it is OK to directly return >>>> false from __generic_fsdax_supported(). >>>> - If dax_dev pointer is NOT NULL, it might be because the driver doesn't >>>> support DAX and not explicitly initialize related data structure. Then >>>> bdev_dax_supported() should be called for further check. >>>> >>>> IMHO if device driver desn't explicitly set its dax_dev pointer to NULL, >>>> this is not a bug. Calling bdev_dax_supported() makes sure they can be >>>> recognized as dax-unsupported eventually. >>>> >>>> This patch does the following change for the above purpose, >>>> - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { >>>> + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { >>>> >>>> >>>> Fixes: c2affe920b0e ("dax: do not print error message for non-persistent memory block device") >>>> Signed-off-by: Coly Li <colyli@suse.de> >>> >>> I hate to do this because I realize this is a bug which people really need >>> fixed. >>> >>> However, shouldn't we also check (!dax_dev || !bdev_dax_supported()) as the >>> _first_ check in __generic_fsdax_supported()? >>> >>> It seems like the other pr_info's could also be called when DAX is not >>> supported and we probably don't want them to be? >>> >>> Perhaps that should be a follow on patch though. So... >> >> I am not author of c2affe920b0e, but I guess it was because >> bdev_dax_supported() needed blocksize, so blocksize should pass previous >> checks firstly to make sure bdev_dax_supported() has a correct blocksize >> to check. >> >>> >>> As a direct fix to c2affe920b0e >>> >>> Reviewed-by: Ira Weiny <ira.weiny@intel.com> >> >> Thanks. >> >> Coly Li >> [snipped] -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/dax/super.c b/drivers/dax/super.c index 32642634c1bb..e5767c83ea23 100644 --- a/drivers/dax/super.c +++ b/drivers/dax/super.c @@ -100,7 +100,7 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev, return false; } - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { pr_debug("%s: error: dax unsupported by block device\n", bdevname(bdev, buf)); return false;
When calling __generic_fsdax_supported(), a dax-unsupported device may not have dax_dev as NULL, e.g. the dax related code block is not enabled by Kconfig. Therefore in __generic_fsdax_supported(), to check whether a device supports DAX or not, the following order should be performed, - If dax_dev pointer is NULL, it means the device driver explicitly announce it doesn't support DAX. Then it is OK to directly return false from __generic_fsdax_supported(). - If dax_dev pointer is NOT NULL, it might be because the driver doesn't support DAX and not explicitly initialize related data structure. Then bdev_dax_supported() should be called for further check. IMHO if device driver desn't explicitly set its dax_dev pointer to NULL, this is not a bug. Calling bdev_dax_supported() makes sure they can be recognized as dax-unsupported eventually. This patch does the following change for the above purpose, - if (!dax_dev && !bdev_dax_supported(bdev, blocksize)) { + if (!dax_dev || !bdev_dax_supported(bdev, blocksize)) { Fixes: c2affe920b0e ("dax: do not print error message for non-persistent memory block device") Signed-off-by: Coly Li <colyli@suse.de> Cc: Adrian Huang <ahuang12@lenovo.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Jan Kara <jack@suse.com> Cc: Mike Snitzer <snitzer@redhat.com> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com> Cc: Vishal Verma <vishal.l.verma@intel.com> --- drivers/dax/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)