| Message ID | 1415025403-31914-1-git-send-email-kolga@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Trond, can you please respond to the patch? As per earlier conversation, in this solution, state recovery is initiated which marks the locks lost. Please either accept this patch or let me know what needs to be fixed. Thank you. On Mon, Nov 3, 2014 at 9:36 AM, Olga Kornievskaia <kolga@netapp.com> wrote: > If we get a bad-stateid-type of error when we send OPEN with delegate_cur > to return currently held delegation, we shouldn't be trying to reclaim locks > associated with that delegation state_id because we don't have an > open_stateid to be used for the LOCK operation. Thus, we should > return an error from the nfs4_open_delegation_recall() in that case. > > Furthermore, if an error occurs the delegation code will call > nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN > flags in the state and it leads the state manager to into an infinite loop > for trying to reclaim the delegated state. > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > --- > fs/nfs/delegation.c | 5 +++-- > fs/nfs/nfs4proc.c | 2 +- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c > index 5853f53..8016d89 100644 > --- a/fs/nfs/delegation.c > +++ b/fs/nfs/delegation.c > @@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation > err = nfs4_wait_clnt_recover(clp); > } while (err == 0); > > - if (err) { > + if (err && err != -EIO) { > nfs_abort_delegation_return(delegation, clp); > goto out; > } > @@ -458,7 +458,8 @@ restart: > iput(inode); > if (!err) > goto restart; > - set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); > + if (err != -EIO) > + set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); > return err; > } > } > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 5aa55c1..6871055 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -1655,7 +1655,7 @@ static int nfs4_handle_delegation_recall_error(struct nfs_server *server, struct > nfs_inode_find_state_and_recover(state->inode, > stateid); > nfs4_schedule_stateid_recovery(server, state); > - return 0; > + return -EIO; > case -NFS4ERR_DELAY: > case -NFS4ERR_GRACE: > set_bit(NFS_DELEGATED_STATE, &state->flags); > -- > 1.7.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 4, 2014 at 6:22 PM, Olga Kornievskaia <aglo@umich.edu> wrote: > Trond, can you please respond to the patch? > > As per earlier conversation, in this solution, state recovery is > initiated which marks the locks lost. > > Please either accept this patch or let me know what needs to be fixed. > Please see the 3 fixes I just sent out concerning delegation recovery w.r.t. NFSv4+NFSv4.1. In addition, we need to handle the case you patch attempts to address (however see the question I have below). > Thank you. > > On Mon, Nov 3, 2014 at 9:36 AM, Olga Kornievskaia <kolga@netapp.com> wrote: >> If we get a bad-stateid-type of error when we send OPEN with delegate_cur >> to return currently held delegation, we shouldn't be trying to reclaim locks >> associated with that delegation state_id because we don't have an >> open_stateid to be used for the LOCK operation. Thus, we should >> return an error from the nfs4_open_delegation_recall() in that case. >> >> Furthermore, if an error occurs the delegation code will call >> nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN >> flags in the state and it leads the state manager to into an infinite loop >> for trying to reclaim the delegated state. >> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com> >> --- >> fs/nfs/delegation.c | 5 +++-- >> fs/nfs/nfs4proc.c | 2 +- >> 2 files changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c >> index 5853f53..8016d89 100644 >> --- a/fs/nfs/delegation.c >> +++ b/fs/nfs/delegation.c >> @@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation >> err = nfs4_wait_clnt_recover(clp); >> } while (err == 0); >> >> - if (err) { >> + if (err && err != -EIO) { >> nfs_abort_delegation_return(delegation, clp); This exception for EIO now has me worried. If we detach the delegation, then it looks to me as if we will never send a FREE_STATEID, as required for the case of NFSv4.1. >> goto out; >> } >> @@ -458,7 +458,8 @@ restart: >> iput(inode); >> if (!err) >> goto restart; >> - set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); >> + if (err != -EIO) >> + set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); Please explain why this is needed. If we've cleared the bad delegation, then why should we not attempt to return any others that may be pending? >> return err; >> } >> } >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index 5aa55c1..6871055 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -1655,7 +1655,7 @@ static int nfs4_handle_delegation_recall_error(struct nfs_server *server, struct >> nfs_inode_find_state_and_recover(state->inode, >> stateid); >> nfs4_schedule_stateid_recovery(server, state); >> - return 0; >> + return -EIO; >> case -NFS4ERR_DELAY: >> case -NFS4ERR_GRACE: >> set_bit(NFS_DELEGATED_STATE, &state->flags); >> -- >> 1.7.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thank you for looking into this! On Tue, Nov 4, 2014 at 7:14 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Tue, Nov 4, 2014 at 6:22 PM, Olga Kornievskaia <aglo@umich.edu> wrote: >> Trond, can you please respond to the patch? >> >> As per earlier conversation, in this solution, state recovery is >> initiated which marks the locks lost. >> >> Please either accept this patch or let me know what needs to be fixed. >> > > Please see the 3 fixes I just sent out concerning delegation recovery > w.r.t. NFSv4+NFSv4.1. In addition, we need to handle the case you > patch attempts to address (however see the question I have below). > >> Thank you. >> >> On Mon, Nov 3, 2014 at 9:36 AM, Olga Kornievskaia <kolga@netapp.com> wrote: >>> If we get a bad-stateid-type of error when we send OPEN with delegate_cur >>> to return currently held delegation, we shouldn't be trying to reclaim locks >>> associated with that delegation state_id because we don't have an >>> open_stateid to be used for the LOCK operation. Thus, we should >>> return an error from the nfs4_open_delegation_recall() in that case. >>> >>> Furthermore, if an error occurs the delegation code will call >>> nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN >>> flags in the state and it leads the state manager to into an infinite loop >>> for trying to reclaim the delegated state. >>> >>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com> >>> --- >>> fs/nfs/delegation.c | 5 +++-- >>> fs/nfs/nfs4proc.c | 2 +- >>> 2 files changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c >>> index 5853f53..8016d89 100644 >>> --- a/fs/nfs/delegation.c >>> +++ b/fs/nfs/delegation.c >>> @@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation >>> err = nfs4_wait_clnt_recover(clp); >>> } while (err == 0); >>> >>> - if (err) { >>> + if (err && err != -EIO) { >>> nfs_abort_delegation_return(delegation, clp); > > This exception for EIO now has me worried. If we detach the > delegation, then it looks to me as if we will never send a > FREE_STATEID, as required for the case of NFSv4.1. I'd need to brush up on the FREE_STATEID op to confidently answer that. But can't this be taken care of in state_recovery when locks are marked lost? > >>> goto out; >>> } >>> @@ -458,7 +458,8 @@ restart: >>> iput(inode); >>> if (!err) >>> goto restart; >>> - set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); >>> + if (err != -EIO) >>> + set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); > > Please explain why this is needed. If we've cleared the bad > delegation, then why should we not attempt to return any others that > may be pending? You are right, I think this is unnecessary because after we return the delegation the bit can be set again for any other leftover delegations. I have tested my patch without that chuck and it still fixes the issues. > >>> return err; >>> } >>> } >>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>> index 5aa55c1..6871055 100644 >>> --- a/fs/nfs/nfs4proc.c >>> +++ b/fs/nfs/nfs4proc.c >>> @@ -1655,7 +1655,7 @@ static int nfs4_handle_delegation_recall_error(struct nfs_server *server, struct >>> nfs_inode_find_state_and_recover(state->inode, >>> stateid); >>> nfs4_schedule_stateid_recovery(server, state); >>> - return 0; >>> + return -EIO; >>> case -NFS4ERR_DELAY: >>> case -NFS4ERR_GRACE: >>> set_bit(NFS_DELEGATED_STATE, &state->flags); >>> -- >>> 1.7.1 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Trond Myklebust > > Linux NFS client maintainer, PrimaryData > > trond.myklebust@primarydata.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 5, 2014 at 12:53 PM, Olga Kornievskaia <aglo@umich.edu> wrote: > Thank you for looking into this! > > On Tue, Nov 4, 2014 at 7:14 PM, Trond Myklebust > <trond.myklebust@primarydata.com> wrote: >> On Tue, Nov 4, 2014 at 6:22 PM, Olga Kornievskaia <aglo@umich.edu> wrote: >>> Trond, can you please respond to the patch? >>> >>> As per earlier conversation, in this solution, state recovery is >>> initiated which marks the locks lost. >>> >>> Please either accept this patch or let me know what needs to be fixed. >>> >> >> Please see the 3 fixes I just sent out concerning delegation recovery >> w.r.t. NFSv4+NFSv4.1. In addition, we need to handle the case you >> patch attempts to address (however see the question I have below). >> >>> Thank you. >>> >>> On Mon, Nov 3, 2014 at 9:36 AM, Olga Kornievskaia <kolga@netapp.com> wrote: >>>> If we get a bad-stateid-type of error when we send OPEN with delegate_cur >>>> to return currently held delegation, we shouldn't be trying to reclaim locks >>>> associated with that delegation state_id because we don't have an >>>> open_stateid to be used for the LOCK operation. Thus, we should >>>> return an error from the nfs4_open_delegation_recall() in that case. >>>> >>>> Furthermore, if an error occurs the delegation code will call >>>> nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN >>>> flags in the state and it leads the state manager to into an infinite loop >>>> for trying to reclaim the delegated state. >>>> >>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com> >>>> --- >>>> fs/nfs/delegation.c | 5 +++-- >>>> fs/nfs/nfs4proc.c | 2 +- >>>> 2 files changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c >>>> index 5853f53..8016d89 100644 >>>> --- a/fs/nfs/delegation.c >>>> +++ b/fs/nfs/delegation.c >>>> @@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation >>>> err = nfs4_wait_clnt_recover(clp); >>>> } while (err == 0); >>>> >>>> - if (err) { >>>> + if (err && err != -EIO) { >>>> nfs_abort_delegation_return(delegation, clp); >> >> This exception for EIO now has me worried. If we detach the >> delegation, then it looks to me as if we will never send a >> FREE_STATEID, as required for the case of NFSv4.1. > > I'd need to brush up on the FREE_STATEID op to confidently answer > that. But can't this be taken care of in state_recovery when locks are > marked lost? No, because in the case of EIO we now fall through to nfs_detach_delegation() and nfs_do_return_delegation(). What we really want to do is call TEST_STATEID + FREE_STATEID (see RFC5661 sections 8.2.4, 8.5 and 18.38.3), which will now be done as part of state recovery.
On Wed, Nov 5, 2014 at 1:38 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > On Wed, Nov 5, 2014 at 12:53 PM, Olga Kornievskaia <aglo@umich.edu> wrote: >> Thank you for looking into this! >> >> On Tue, Nov 4, 2014 at 7:14 PM, Trond Myklebust >> <trond.myklebust@primarydata.com> wrote: >>> On Tue, Nov 4, 2014 at 6:22 PM, Olga Kornievskaia <aglo@umich.edu> wrote: >>>> Trond, can you please respond to the patch? >>>> >>>> As per earlier conversation, in this solution, state recovery is >>>> initiated which marks the locks lost. >>>> >>>> Please either accept this patch or let me know what needs to be fixed. >>>> >>> >>> Please see the 3 fixes I just sent out concerning delegation recovery >>> w.r.t. NFSv4+NFSv4.1. In addition, we need to handle the case you >>> patch attempts to address (however see the question I have below). >>> >>>> Thank you. >>>> >>>> On Mon, Nov 3, 2014 at 9:36 AM, Olga Kornievskaia <kolga@netapp.com> wrote: >>>>> If we get a bad-stateid-type of error when we send OPEN with delegate_cur >>>>> to return currently held delegation, we shouldn't be trying to reclaim locks >>>>> associated with that delegation state_id because we don't have an >>>>> open_stateid to be used for the LOCK operation. Thus, we should >>>>> return an error from the nfs4_open_delegation_recall() in that case. >>>>> >>>>> Furthermore, if an error occurs the delegation code will call >>>>> nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN >>>>> flags in the state and it leads the state manager to into an infinite loop >>>>> for trying to reclaim the delegated state. >>>>> >>>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com> >>>>> --- >>>>> fs/nfs/delegation.c | 5 +++-- >>>>> fs/nfs/nfs4proc.c | 2 +- >>>>> 2 files changed, 4 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c >>>>> index 5853f53..8016d89 100644 >>>>> --- a/fs/nfs/delegation.c >>>>> +++ b/fs/nfs/delegation.c >>>>> @@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation >>>>> err = nfs4_wait_clnt_recover(clp); >>>>> } while (err == 0); >>>>> >>>>> - if (err) { >>>>> + if (err && err != -EIO) { >>>>> nfs_abort_delegation_return(delegation, clp); >>> >>> This exception for EIO now has me worried. If we detach the >>> delegation, then it looks to me as if we will never send a >>> FREE_STATEID, as required for the case of NFSv4.1. >> >> I'd need to brush up on the FREE_STATEID op to confidently answer >> that. But can't this be taken care of in state_recovery when locks are >> marked lost? > > No, because in the case of EIO we now fall through to > nfs_detach_delegation() and nfs_do_return_delegation(). What we really > want to do is call TEST_STATEID + FREE_STATEID (see RFC5661 sections > 8.2.4, 8.5 and 18.38.3), which will now be done as part of state > recovery. I don't see it. :-/ Is it because there is a race between returning a delegation and state recovery that was initiated? Because as far as I can see, the stateid recovery initiated by the handle_delegation_recall_error() will call nfs4_test_stateid() via nfs41_open_expire(). > > -- > Trond Myklebust > > Linux NFS client maintainer, PrimaryData > > trond.myklebust@primarydata.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 6, 2014 at 1:54 PM, Olga Kornievskaia <aglo@umich.edu> wrote: > On Wed, Nov 5, 2014 at 1:38 PM, Trond Myklebust > <trond.myklebust@primarydata.com> wrote: >> On Wed, Nov 5, 2014 at 12:53 PM, Olga Kornievskaia <aglo@umich.edu> wrote: >>> Thank you for looking into this! >>> >>> On Tue, Nov 4, 2014 at 7:14 PM, Trond Myklebust >>> <trond.myklebust@primarydata.com> wrote: >>>> On Tue, Nov 4, 2014 at 6:22 PM, Olga Kornievskaia <aglo@umich.edu> wrote: >>>>> Trond, can you please respond to the patch? >>>>> >>>>> As per earlier conversation, in this solution, state recovery is >>>>> initiated which marks the locks lost. >>>>> >>>>> Please either accept this patch or let me know what needs to be fixed. >>>>> >>>> >>>> Please see the 3 fixes I just sent out concerning delegation recovery >>>> w.r.t. NFSv4+NFSv4.1. In addition, we need to handle the case you >>>> patch attempts to address (however see the question I have below). >>>> >>>>> Thank you. >>>>> >>>>> On Mon, Nov 3, 2014 at 9:36 AM, Olga Kornievskaia <kolga@netapp.com> wrote: >>>>>> If we get a bad-stateid-type of error when we send OPEN with delegate_cur >>>>>> to return currently held delegation, we shouldn't be trying to reclaim locks >>>>>> associated with that delegation state_id because we don't have an >>>>>> open_stateid to be used for the LOCK operation. Thus, we should >>>>>> return an error from the nfs4_open_delegation_recall() in that case. >>>>>> >>>>>> Furthermore, if an error occurs the delegation code will call >>>>>> nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN >>>>>> flags in the state and it leads the state manager to into an infinite loop >>>>>> for trying to reclaim the delegated state. >>>>>> >>>>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com> >>>>>> --- >>>>>> fs/nfs/delegation.c | 5 +++-- >>>>>> fs/nfs/nfs4proc.c | 2 +- >>>>>> 2 files changed, 4 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c >>>>>> index 5853f53..8016d89 100644 >>>>>> --- a/fs/nfs/delegation.c >>>>>> +++ b/fs/nfs/delegation.c >>>>>> @@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation >>>>>> err = nfs4_wait_clnt_recover(clp); >>>>>> } while (err == 0); >>>>>> >>>>>> - if (err) { >>>>>> + if (err && err != -EIO) { >>>>>> nfs_abort_delegation_return(delegation, clp); >>>> >>>> This exception for EIO now has me worried. If we detach the >>>> delegation, then it looks to me as if we will never send a >>>> FREE_STATEID, as required for the case of NFSv4.1. >>> >>> I'd need to brush up on the FREE_STATEID op to confidently answer >>> that. But can't this be taken care of in state_recovery when locks are >>> marked lost? >> >> No, because in the case of EIO we now fall through to >> nfs_detach_delegation() and nfs_do_return_delegation(). What we really >> want to do is call TEST_STATEID + FREE_STATEID (see RFC5661 sections >> 8.2.4, 8.5 and 18.38.3), which will now be done as part of state >> recovery. > > I don't see it. :-/ Is it because there is a race between returning a > delegation and state recovery that was initiated? Because as far as I > can see, the stateid recovery initiated by the > handle_delegation_recall_error() will call nfs4_test_stateid() via > nfs41_open_expire(). > No. My point is that once we call nfs_detach_delegation(), then nobody will be able to find the delegation from either the struct nfs_inode or from the server->delegations list. IOW: the question is how would the state recovery thread find it in the first place?
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c index 5853f53..8016d89 100644 --- a/fs/nfs/delegation.c +++ b/fs/nfs/delegation.c @@ -394,7 +394,7 @@ static int nfs_end_delegation_return(struct inode *inode, struct nfs_delegation err = nfs4_wait_clnt_recover(clp); } while (err == 0); - if (err) { + if (err && err != -EIO) { nfs_abort_delegation_return(delegation, clp); goto out; } @@ -458,7 +458,8 @@ restart: iput(inode); if (!err) goto restart; - set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); + if (err != -EIO) + set_bit(NFS4CLNT_DELEGRETURN, &clp->cl_state); return err; } } diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 5aa55c1..6871055 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -1655,7 +1655,7 @@ static int nfs4_handle_delegation_recall_error(struct nfs_server *server, struct nfs_inode_find_state_and_recover(state->inode, stateid); nfs4_schedule_stateid_recovery(server, state); - return 0; + return -EIO; case -NFS4ERR_DELAY: case -NFS4ERR_GRACE: set_bit(NFS_DELEGATED_STATE, &state->flags);
If we get a bad-stateid-type of error when we send OPEN with delegate_cur to return currently held delegation, we shouldn't be trying to reclaim locks associated with that delegation state_id because we don't have an open_stateid to be used for the LOCK operation. Thus, we should return an error from the nfs4_open_delegation_recall() in that case. Furthermore, if an error occurs the delegation code will call nfs_abort_delegation_return() which sets again the NFS4CLNT_DELEGRETURN flags in the state and it leads the state manager to into an infinite loop for trying to reclaim the delegated state. Signed-off-by: Olga Kornievskaia <kolga@netapp.com> --- fs/nfs/delegation.c | 5 +++-- fs/nfs/nfs4proc.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-)