Re: [PATCH] pNFS: make DS availability problems visible in log

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2021-03-04 at 20:20 +0000, Chuck Lever wrote:
> Hello Roberto!
> 
> > On Mar 3, 2021, at 10:33 AM, Roberto Bergantinos Corpas <
> > rbergant@redhat.com> wrote:
> > 
> > Would be interesting to promote DS availability logging outside
> > debug
> > so that we are more aware that I/O is diverted to MDS and some part
> > of the infraestructure failed.
> > 
> > Also added logging for failed DS connection attempts.
> 
> Given that this enables remote system behavior to generate
> kernel log traffic that can fill the local root partition,
> I'd like to see either:
> 
> - the explicit use of rate limiting, or
> 
> - these dprintks replaced with tracepoints

I cannot accept a pr_warn(), even a rate limited one, for a timeout
error or for a connection error in the data path. Those are just too
nasty to deal with in a syslog.

Tracepoints would be acceptable.

> 
> 
> > Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
> > ---
> > fs/nfs/filelayout/filelayout.c         | 4 ++--
> > fs/nfs/flexfilelayout/flexfilelayout.c | 6 +++---
> > fs/nfs/pnfs_nfs.c                      | 6 +++++-
> > 3 files changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/nfs/filelayout/filelayout.c
> > b/fs/nfs/filelayout/filelayout.c
> > index 7f5aa0403e16..fef2d31a501a 100644
> > --- a/fs/nfs/filelayout/filelayout.c
> > +++ b/fs/nfs/filelayout/filelayout.c
> > @@ -181,7 +181,7 @@ static int filelayout_async_handle_error(struct
> > rpc_task *task,
> >         case -EIO:
> >         case -ETIMEDOUT:
> >         case -EPIPE:
> > -               dprintk("%s DS connection error %d\n", __func__,
> > +               pr_warn("%s DS connection error %d\n", __func__,
> >                         task->tk_status);
> >                 nfs4_mark_deviceid_unavailable(devid);
> >                 pnfs_error_mark_layout_for_return(inode, lseg);
> > @@ -190,7 +190,7 @@ static int filelayout_async_handle_error(struct
> > rpc_task *task,
> >                 fallthrough;
> >         default:
> > reset:
> > -               dprintk("%s Retry through MDS. Error %d\n",
> > __func__,
> > +               pr_warn("%s Retry through MDS. Error %d\n",
> > __func__,
> >                         task->tk_status);
> >                 return -NFS4ERR_RESET_TO_MDS;
> >         }
> > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c
> > b/fs/nfs/flexfilelayout/flexfilelayout.c
> > index a163533446fa..7150d94e80e6 100644
> > --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> > +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> > @@ -1129,7 +1129,7 @@ static int
> > ff_layout_async_handle_error_v4(struct rpc_task *task,
> >         case -EIO:
> >         case -ETIMEDOUT:
> >         case -EPIPE:
> > -               dprintk("%s DS connection error %d\n", __func__,
> > +               pr_warn("%s DS connection error %d\n", __func__,
> >                         task->tk_status);
> >                 nfs4_delete_deviceid(devid->ld, devid->nfs_client,
> >                                 &devid->deviceid);
> > @@ -1139,7 +1139,7 @@ static int
> > ff_layout_async_handle_error_v4(struct rpc_task *task,
> >                 if (ff_layout_avoid_mds_available_ds(lseg))
> >                         return -NFS4ERR_RESET_TO_PNFS;
> > reset:
> > -               dprintk("%s Retry through MDS. Error %d\n",
> > __func__,
> > +               pr_warn("%s Retry through MDS. Error %d\n",
> > __func__,
> >                         task->tk_status);
> >                 return -NFS4ERR_RESET_TO_MDS;
> >         }
> > @@ -1167,7 +1167,7 @@ static int
> > ff_layout_async_handle_error_v3(struct rpc_task *task,
> >                 nfs_inc_stats(lseg->pls_layout->plh_inode,
> > NFSIOS_DELAY);
> >                 goto out_retry;
> >         default:
> > -               dprintk("%s DS connection error %d\n", __func__,
> > +               pr_warn("%s DS connection error %d\n", __func__,
> >                         task->tk_status);
> >                 nfs4_delete_deviceid(devid->ld, devid->nfs_client,
> >                                 &devid->deviceid);
> > diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
> > index 679767ac258d..322661a48348 100644
> > --- a/fs/nfs/pnfs_nfs.c
> > +++ b/fs/nfs/pnfs_nfs.c
> > @@ -934,8 +934,11 @@ static int _nfs4_pnfs_v4_ds_connect(struct
> > nfs_server *mds_srv,
> >                                                 (struct sockaddr
> > *)&da->da_addr,
> >                                                 da->da_addrlen,
> > IPPROTO_TCP,
> >                                                 timeo, retrans,
> > minor_version);
> > -                       if (IS_ERR(clp))
> > +                       if (IS_ERR(clp)) {
> > +                               pr_warn("%s: DS: %s unable to
> > connect with address %s, error: %ld\n",
> > +                                       __func__, ds->ds_remotestr,
> > da->da_remotestr, PTR_ERR(clp));
> >                                 continue;
> > +                       }
> > 
> >                         status = nfs4_init_ds_session(clp,
> >                                         mds_srv->nfs_client-
> > >cl_lease_time);
> > @@ -949,6 +952,7 @@ static int _nfs4_pnfs_v4_ds_connect(struct
> > nfs_server *mds_srv,
> >         }
> > 
> >         if (IS_ERR(clp)) {
> > +               pr_warn("%s: no DS available\n", __func__);
> >                 status = PTR_ERR(clp);
> >                 goto out;
> >         }
> > -- 
> > 2.21.0
> > 
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com