On Thu, 2012-03-22 at 13:23 +0000, Adamson, Andy wrote:
> On Mar 21, 2012, at 4:39 PM, Myklebust, Trond wrote:
>
> > On Wed, 2012-03-21 at 15:46 -0400, andros@xxxxxxxxxx wrote:
> >> From: Andy Adamson <andros@xxxxxxxxxx>
> >>
> >> This prevents the use of any layout for i/o that references the deviceid.
> >> I/O is redirected through the MDS.
> >>
> >> Redirect the unhandled failed I/O to the MDS without marking either the
> >> layout or the deviceid invalid.
> >>
> >> Signed-off-by: Andy Adamson <andros@xxxxxxxxxx>
> >> ---
> >> fs/nfs/nfs4filelayout.c | 65 ++++++++++++++++++++++++++++++++++------------
> >> fs/nfs/nfs4filelayout.h | 6 ++++
> >> 2 files changed, 54 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> >> index 3802937..1f1be26 100644
> >> --- a/fs/nfs/nfs4filelayout.c
> >> +++ b/fs/nfs/nfs4filelayout.c
> >> @@ -116,7 +116,7 @@ void filelayout_reset_read(struct rpc_task *task, struct nfs_read_data *data)
> >> static int filelayout_async_handle_error(struct rpc_task *task,
> >> struct nfs4_state *state,
> >> struct nfs_client *clp,
> >> - int *reset)
> >> + unsigned long *reset)
> >> {
> >> struct nfs_server *mds_server = NFS_SERVER(state->inode);
> >> struct nfs_client *mds_client = mds_server->nfs_client;
> >> @@ -158,10 +158,23 @@ static int filelayout_async_handle_error(struct rpc_task *task,
> >> break;
> >> case -NFS4ERR_RETRY_UNCACHED_REP:
> >> break;
> >> + /* RPC connection errors */
> >> + case -ECONNREFUSED:
> >> + case -EHOSTDOWN:
> >> + case -EHOSTUNREACH:
> >> + case -ENETUNREACH:
> >> + case -EIO:
> >> + case -ETIMEDOUT:
> >> + case -EPIPE:
> >> + dprintk("%s DS connection error. Retry through MDS %d\n",
> >> + __func__, task->tk_status);
> >> + set_bit(NFS4_RESET_DEVICEID, reset);
> >> + set_bit(NFS4_RESET_TO_MDS, reset);
> >> + break;
> >> default:
> >> - dprintk("%s DS error. Retry through MDS %d\n", __func__,
> >> - task->tk_status);
> >> - *reset = 1;
> >> + dprintk("%s Unhandled DS error. Retry through MDS %d\n",
> >> + __func__, task->tk_status);
> >> + set_bit(NFS4_RESET_TO_MDS, reset);
> >> break;
> >> }
> >> out:
> >> @@ -179,16 +192,22 @@ wait_on_recovery:
> >> static int filelayout_read_done_cb(struct rpc_task *task,
> >> struct nfs_read_data *data)
> >> {
> >> - int reset = 0;
> >> + struct nfs4_deviceid_node *devid = FILELAYOUT_DEVID_NODE(data->lseg);
> >> + unsigned long reset = 0;
> >>
> >> dprintk("%s DS read\n", __func__);
> >>
> >> if (filelayout_async_handle_error(task, data->args.context->state,
> >> data->ds_clp, &reset) == -EAGAIN) {
> >> - dprintk("%s calling restart ds_clp %p ds_clp->cl_session %p\n",
> >> - __func__, data->ds_clp, data->ds_clp->cl_session);
> >> - if (reset)
> >> +
> >> + dprintk("%s reset 0x%lx ds_clp %p session %p\n", __func__,
> >> + reset, data->ds_clp, data->ds_clp->cl_session);
> >> +
> >> + if (test_bit(NFS4_RESET_TO_MDS, &reset)) {
> >> filelayout_reset_read(task, data);
> >> + if (test_bit(NFS4_RESET_DEVICEID, &reset))
> >> + filelayout_mark_devid_invalid(devid);
> >
> > Is there any reason why we shouldn't just do the
> > filelayout_mark_devid_invalid() within filelayout_async_handle_error()
> > instead of having the caller do it?
>
> We would have to pass in the lseg argument.
No. You'd only have to pass in the device id.
> >
> > That should also enable us to get rid of the whole 'reset' argument and
> > replace it with a return value != 0 && != -EAGAIN.
>
> We would need to pass in the operation because READ/WRITE/COMMIT call different reset functions.
No. The caller would still do the reset. It's just that you would have a
special return value to indicate it.
> I chose the 'reset' argument method instead of passing in the operation because I thought it cleaner to keep the per operation logic in the per operation rpc functions instead of having a switch(operation) statement in the async handler for each group of errors.
My point is that you don't need an extra parameter to do this. You just
need a special return value.
> >
> >> + }
> >> rpc_restart_call_prepare(task);
> >
> > This can probably also be done inside filelayout_async_handle_error(),
> > BTW.
>
> COMMIT does not call rpc_restart_call_prepare on reset or invalid deviceid errors because we want the release function to fail with a verifier mismatch.
Which is a grotesque hack in itself... (the verifier hack, that is). I'm
hoping that Fred will fix that in the new code. It doesn't take much:
just flag in the struct nfs_write_data (or in his case: struct
nfs_commit_data).
> So it's up to you: I could move all the per operation logic into the async handler by passing in the operation and using it to dereferencing the tk_callback to get the lseg and other needed parameters - then moving the code from the done_cb routines into the async handler under a switch(operation) for both the default reset to mds and for the invalid deviced.
Then let's keep the rpc_restart_call_prepare in the caller, but move the
deviceid invalidation into the async_hander. Let's also replace the
reset argument with a return value...
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
