diff mbox series

[RFC,2/4] nfs/blocklayout: Report only when /no/ device is found

Message ID 20240619173929.177818-8-cel@kernel.org (mailing list archive)
State New
Headers show
Series Snapshot of fixes for SCSI PR key registration | expand

Commit Message

Chuck Lever June 19, 2024, 5:39 p.m. UTC
From: Chuck Lever <chuck.lever@oracle.com>

Since f931d8374cad ("nfs/blocklayout: refactor block device
opening"), an error is reported when no multi-path device is
found. But this isn't a fatal error if the subsequent device open
is successful. On systems without multi-path devices, this message
always appears whether there is a problem or not.

Instead, generate less system journal noise by reporting an error
only when both open attempts fail. The new error message is more
actionable since it indicates that there is a real configuration
issue to be addressed.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/blocklayout/dev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig June 20, 2024, 4:36 a.m. UTC | #1
On Wed, Jun 19, 2024 at 01:39:32PM -0400, cel@kernel.org wrote:
>  	if (IS_ERR(bdev_file)) {
> -		pr_warn("pNFS: failed to open device %s (%ld)\n",
> +		dprintk("failed to open device %s (%ld)\n",
>  			devname, PTR_ERR(bdev_file));

I'd just drop this one entirely.  Otherwise this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Benjamin Coddington June 20, 2024, 12:17 p.m. UTC | #2
On 19 Jun 2024, at 13:39, cel@kernel.org wrote:

> From: Chuck Lever <chuck.lever@oracle.com>
>
> Since f931d8374cad ("nfs/blocklayout: refactor block device
> opening"), an error is reported when no multi-path device is
> found. But this isn't a fatal error if the subsequent device open
> is successful. On systems without multi-path devices, this message
> always appears whether there is a problem or not.
>
> Instead, generate less system journal noise by reporting an error
> only when both open attempts fail. The new error message is more
> actionable since it indicates that there is a real configuration
> issue to be addressed.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/blocklayout/dev.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
> index b3828e5ee079..356bc967fb5d 100644
> --- a/fs/nfs/blocklayout/dev.c
> +++ b/fs/nfs/blocklayout/dev.c
> @@ -318,7 +318,7 @@ bl_open_path(struct pnfs_block_volume *v, const char *prefix)
>  	bdev_file = bdev_file_open_by_path(devname, BLK_OPEN_READ | BLK_OPEN_WRITE,
>  					NULL, NULL);
>  	if (IS_ERR(bdev_file)) {
> -		pr_warn("pNFS: failed to open device %s (%ld)\n",
> +		dprintk("failed to open device %s (%ld)\n",
>  			devname, PTR_ERR(bdev_file));
>  	}
>
> @@ -348,8 +348,11 @@ bl_parse_scsi(struct nfs_server *server, struct pnfs_block_dev *d,
>  	bdev_file = bl_open_path(v, "dm-uuid-mpath-0x");
>  	if (IS_ERR(bdev_file))
>  		bdev_file = bl_open_path(v, "wwn-0x");
> -	if (IS_ERR(bdev_file))
> +	if (IS_ERR(bdev_file)) {
> +		pr_err("pNFS: no device found for volume %*phN\n",

pr_warn is more appropriate here because there can be clients mounting that
that don't have block devices for the filesystem.  That's not necessarily a
configuration problem.

Ben

> +			v->scsi.designator_len, v->scsi.designator);
>  		return PTR_ERR(bdev_file);
> +	}
>  	d->bdev_file = bdev_file;
>  	bdev = file_bdev(bdev_file);
>
> -- 
> 2.45.1
Christoph Hellwig June 20, 2024, 2:10 p.m. UTC | #3
On Thu, Jun 20, 2024 at 08:17:12AM -0400, Benjamin Coddington wrote:
> > -	if (IS_ERR(bdev_file))
> > +	if (IS_ERR(bdev_file)) {
> > +		pr_err("pNFS: no device found for volume %*phN\n",
> 
> pr_warn is more appropriate here because there can be clients mounting that
> that don't have block devices for the filesystem.  That's not necessarily a
> configuration problem.

Yes, agreed.
Chuck Lever June 20, 2024, 2:59 p.m. UTC | #4
On Thu, Jun 20, 2024 at 06:36:05AM +0200, Christoph Hellwig wrote:
> On Wed, Jun 19, 2024 at 01:39:32PM -0400, cel@kernel.org wrote:
> >  	if (IS_ERR(bdev_file)) {
> > -		pr_warn("pNFS: failed to open device %s (%ld)\n",
> > +		dprintk("failed to open device %s (%ld)\n",
> >  			devname, PTR_ERR(bdev_file));
> 
> I'd just drop this one entirely.

Actually I'd like to keep this dprintk because it displays the whole
path to the device. That has diagnostic value, IMO.
diff mbox series

Patch

diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
index b3828e5ee079..356bc967fb5d 100644
--- a/fs/nfs/blocklayout/dev.c
+++ b/fs/nfs/blocklayout/dev.c
@@ -318,7 +318,7 @@  bl_open_path(struct pnfs_block_volume *v, const char *prefix)
 	bdev_file = bdev_file_open_by_path(devname, BLK_OPEN_READ | BLK_OPEN_WRITE,
 					NULL, NULL);
 	if (IS_ERR(bdev_file)) {
-		pr_warn("pNFS: failed to open device %s (%ld)\n",
+		dprintk("failed to open device %s (%ld)\n",
 			devname, PTR_ERR(bdev_file));
 	}
 
@@ -348,8 +348,11 @@  bl_parse_scsi(struct nfs_server *server, struct pnfs_block_dev *d,
 	bdev_file = bl_open_path(v, "dm-uuid-mpath-0x");
 	if (IS_ERR(bdev_file))
 		bdev_file = bl_open_path(v, "wwn-0x");
-	if (IS_ERR(bdev_file))
+	if (IS_ERR(bdev_file)) {
+		pr_err("pNFS: no device found for volume %*phN\n",
+			v->scsi.designator_len, v->scsi.designator);
 		return PTR_ERR(bdev_file);
+	}
 	d->bdev_file = bdev_file;
 	bdev = file_bdev(bdev_file);