Re: [PATCH v3] dax: print error message by pr_info() in __generic_fsdax_supported()

2020-07-27 Thread Jane Chu

On 7/27/2020 2:44 PM, Jan Kara wrote:

On Mon 27-07-20 10:02:11, Jane Chu wrote:

Hi,

On 7/25/2020 9:24 AM, Coly Li wrote:

It is not simple to make dax_supported() from struct dax_operations
or __generic_fsdax_supported() to return exact failure type right now.
So the simplest fix is to use pr_info() to print all the error messages
inside __generic_fsdax_supported(). Then users may find informative clue
from the kernel message at least.


I happen to notice that some servers set their printk levels at 4 by default
to minimize console messages:
# cat /proc/sys/kernel/printk
  4   4   1  7
So I'm wondering if you would consider pr_error() instead of pr_info() ?


I don't think this is a good reason to raise priority of this message -
with this logic applied, all info messages should be raised to error level
because someone may find them useful :). And then people raise printk
loglevel because the kernel is too noisy... Personally I think that
pr_info() is fine because there will be error message about unsupported dax
setup from the filesystem and if sysadmin wishes, (s)he can always lookup
info messages in dmesg.



Okay, sounds like the nature of the error isn't severe enough, and it 
isn't rare and random, rather, it's reproducible such that sysadmin can 
easily catch when raising the printk level.


thanks,
-jane


Honza


___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [PATCH v3] dax: print error message by pr_info() in __generic_fsdax_supported()

2020-07-27 Thread Jan Kara
On Mon 27-07-20 10:02:11, Jane Chu wrote:
> Hi,
> 
> On 7/25/2020 9:24 AM, Coly Li wrote:
> > It is not simple to make dax_supported() from struct dax_operations
> > or __generic_fsdax_supported() to return exact failure type right now.
> > So the simplest fix is to use pr_info() to print all the error messages
> > inside __generic_fsdax_supported(). Then users may find informative clue
> > from the kernel message at least.
> 
> I happen to notice that some servers set their printk levels at 4 by default
> to minimize console messages:
> # cat /proc/sys/kernel/printk
>  4   4   1  7
> So I'm wondering if you would consider pr_error() instead of pr_info() ?

I don't think this is a good reason to raise priority of this message -
with this logic applied, all info messages should be raised to error level
because someone may find them useful :). And then people raise printk
loglevel because the kernel is too noisy... Personally I think that
pr_info() is fine because there will be error message about unsupported dax
setup from the filesystem and if sysadmin wishes, (s)he can always lookup
info messages in dmesg.

Honza
-- 
Jan Kara 
SUSE Labs, CR
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


Re: [PATCH v3] dax: print error message by pr_info() in __generic_fsdax_supported()

2020-07-27 Thread Jane Chu

Hi,

On 7/25/2020 9:24 AM, Coly Li wrote:

It is not simple to make dax_supported() from struct dax_operations
or __generic_fsdax_supported() to return exact failure type right now.
So the simplest fix is to use pr_info() to print all the error messages
inside __generic_fsdax_supported(). Then users may find informative clue
from the kernel message at least.


I happen to notice that some servers set their printk levels at 4 by 
default to minimize console messages:

# cat /proc/sys/kernel/printk
 4   4   1  7
So I'm wondering if you would consider pr_error() instead of pr_info() ?

thanks,
-jane
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org


[PATCH v3] dax: print error message by pr_info() in __generic_fsdax_supported()

2020-07-25 Thread Coly Li
In struct dax_operations, the callback routine dax_supported() returns
a bool type result. For false return value, the caller has no idea
whether the device does not support dax at all, or it is just some mis-
configuration issue.

An example is formatting an Ext4 file system on pmem device on top of
a NVDIMM namespace by,
 # mkfs.ext4 /dev/pmem0
If the fs block size does not match kernel space memory page size (which
is possible on non-x86 platform), mount this Ext4 file system will fail,
  # mount -o dax /dev/pmem0 /mnt
  mount: /mnt: wrong fs type, bad option, bad superblock on /dev/pmem0,
  missing codepage or helper program, or other error.
And from the dmesg output there is only the following information,
  [  307.853148] EXT4-fs (pmem0): DAX unsupported by block device.

The above information is quite confusing. Because definitely the pmem0
device supports dax operation, and the super block is consistent as how
it was created by mkfs.ext4.

Indeed the failure is from __generic_fsdax_supported() by the following
code piece,
if (blocksize != PAGE_SIZE) {
   pr_debug("%s: error: unsupported blocksize for dax\n",
bdevname(bdev, buf));
return false;
}
It is because the Ext4 block size is 4KB and kernel page size is 8KB or
16KB.

It is not simple to make dax_supported() from struct dax_operations
or __generic_fsdax_supported() to return exact failure type right now.
So the simplest fix is to use pr_info() to print all the error messages
inside __generic_fsdax_supported(). Then users may find informative clue
from the kernel message at least.

Message printed by pr_debug() is very easy to be ignored by users. This
patch prints error message by pr_info() in __generic_fsdax_supported(),
when then mount fails, following lines can be found from dmesg output,
 [ 2705.500885] pmem0: error: unsupported blocksize for dax
 [ 2705.500888] EXT4-fs (pmem0): DAX unsupported by block device.
Now the users may have idea the mount failure is from pmem driver for
unsupported block size.

Reported-by: Michal Suchanek 
Suggested-by: Jan Kara 
Signed-off-by: Coly Li 
Reviewed-by: Jan Kara 
Reviewed-by: Ira Weiny 
Reviewed-by: Pankaj Gupta 
Cc: Dan Williams 
Cc: Anthony Iliopoulos 
---
Changelog:
v3: Fix a typo in commit log, add reviewed-by from Ira Weiny and
Pankaj Gupta.
v2: Add reviewed-by from Jan Kara
v1: initial version.

 drivers/dax/super.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index 8e32345be0f7..de0d02ec0347 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -80,14 +80,14 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev,
int err, id;
 
if (blocksize != PAGE_SIZE) {
-   pr_debug("%s: error: unsupported blocksize for dax\n",
+   pr_info("%s: error: unsupported blocksize for dax\n",
bdevname(bdev, buf));
return false;
}
 
err = bdev_dax_pgoff(bdev, start, PAGE_SIZE, );
if (err) {
-   pr_debug("%s: error: unaligned partition for dax\n",
+   pr_info("%s: error: unaligned partition for dax\n",
bdevname(bdev, buf));
return false;
}
@@ -95,7 +95,7 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev,
last_page = PFN_DOWN((start + sectors - 1) * 512) * PAGE_SIZE / 512;
err = bdev_dax_pgoff(bdev, last_page, PAGE_SIZE, _end);
if (err) {
-   pr_debug("%s: error: unaligned partition for dax\n",
+   pr_info("%s: error: unaligned partition for dax\n",
bdevname(bdev, buf));
return false;
}
@@ -106,7 +106,7 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev,
dax_read_unlock(id);
 
if (len < 1 || len2 < 1) {
-   pr_debug("%s: error: dax access failed (%ld)\n",
+   pr_info("%s: error: dax access failed (%ld)\n",
bdevname(bdev, buf), len < 1 ? len : len2);
return false;
}
@@ -139,7 +139,7 @@ bool __generic_fsdax_supported(struct dax_device *dax_dev,
}
 
if (!dax_enabled) {
-   pr_debug("%s: error: dax support not enabled\n",
+   pr_info("%s: error: dax support not enabled\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-le...@lists.01.org