| Message ID | 20130228120959.6764.17787.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 28, 2013 at 03:09:59PM +0300, Stanislav Kinsbursky wrote: > Function nfsd4_client_tracking_init() can return error. Before, I think that we the nfsd4_client_* functions just became no-ops in this case. With the result that no client records get written, and so clients are unable to reclaim on the next boot. Which is annoying, but possibly not as annoying as your server completely refusing to start. It's arguably more helpful in the long run to fail immediately when we recognize reboot recovery isn't going to work. But in practice this may mean people that never knew they had a problem suddenly have servers that don't start at all. So I'm inclined to be more forgiving and leave this as it is. But maybe something like a warning printk would be appropriate. --b. > > Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com> > --- > fs/nfsd/nfs4state.c | 9 ++++++++- > 1 files changed, 8 insertions(+), 1 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index f194f86..fc4b81b 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -69,6 +69,7 @@ static u64 current_sessionid = 1; > > /* forward declarations */ > static int check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner); > +void nfs4_state_shutdown_net(struct net *net); > > /* Locking: */ > > @@ -4942,7 +4943,9 @@ nfs4_state_start_net(struct net *net) > ret = nfs4_state_create_net(net); > if (ret) > return ret; > - nfsd4_client_tracking_init(net); > + ret = nfsd4_client_tracking_init(net); > + if (ret) > + goto out_tracking; > nn->boot_time = get_seconds(); > locks_start_grace(net, &nn->nfsd4_manager); > nn->grace_ended = false; > @@ -4950,6 +4953,10 @@ nfs4_state_start_net(struct net *net) > nn->nfsd4_grace, net); > queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ); > return 0; > + > +out_tracking: > + nfs4_state_destroy_net(net); > + return ret; > } > > /* initialization to perform when the nfsd service is started: */ > -- 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
06.03.2013 01:20, J. Bruce Fields ?????: > On Thu, Feb 28, 2013 at 03:09:59PM +0300, Stanislav Kinsbursky wrote: >> Function nfsd4_client_tracking_init() can return error. > > Before, I think that we the nfsd4_client_* functions just became no-ops > in this case. With the result that no client records get written, and > so clients are unable to reclaim on the next boot. > > Which is annoying, but possibly not as annoying as your server > completely refusing to start. > > It's arguably more helpful in the long run to fail immediately when we > recognize reboot recovery isn't going to work. But in practice this may > mean people that never knew they had a problem suddenly have servers > that don't start at all. > > So I'm inclined to be more forgiving and leave this as it is. But maybe > something like a warning printk would be appropriate. > Ok then. I'll add the warning anf convert the function to be "void" rather then "int". Thanks! > --b. > >> >> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com> >> --- >> fs/nfsd/nfs4state.c | 9 ++++++++- >> 1 files changed, 8 insertions(+), 1 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index f194f86..fc4b81b 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -69,6 +69,7 @@ static u64 current_sessionid = 1; >> >> /* forward declarations */ >> static int check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner); >> +void nfs4_state_shutdown_net(struct net *net); >> >> /* Locking: */ >> >> @@ -4942,7 +4943,9 @@ nfs4_state_start_net(struct net *net) >> ret = nfs4_state_create_net(net); >> if (ret) >> return ret; >> - nfsd4_client_tracking_init(net); >> + ret = nfsd4_client_tracking_init(net); >> + if (ret) >> + goto out_tracking; >> nn->boot_time = get_seconds(); >> locks_start_grace(net, &nn->nfsd4_manager); >> nn->grace_ended = false; >> @@ -4950,6 +4953,10 @@ nfs4_state_start_net(struct net *net) >> nn->nfsd4_grace, net); >> queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ); >> return 0; >> + >> +out_tracking: >> + nfs4_state_destroy_net(net); >> + return ret; >> } >> >> /* initialization to perform when the nfsd service is started: */ >>
06.03.2013 09:06, Stanislav Kinsbursky ?????: > 06.03.2013 01:20, J. Bruce Fields ?????: >> On Thu, Feb 28, 2013 at 03:09:59PM +0300, Stanislav Kinsbursky wrote: >>> Function nfsd4_client_tracking_init() can return error. >> >> Before, I think that we the nfsd4_client_* functions just became no-ops >> in this case. With the result that no client records get written, and >> so clients are unable to reclaim on the next boot. >> >> Which is annoying, but possibly not as annoying as your server >> completely refusing to start. >> >> It's arguably more helpful in the long run to fail immediately when we >> recognize reboot recovery isn't going to work. But in practice this may >> mean people that never knew they had a problem suddenly have servers >> that don't start at all. >> >> So I'm inclined to be more forgiving and leave this as it is. But maybe >> something like a warning printk would be appropriate. >> > > Ok then. > I'll add the warning anf convert the function to be "void" rather then "int". > Thanks! > We already have a warning. So, probably all we need is just to change a prototype to make it less confusing. Is it ok?
On Wed, Mar 06, 2013 at 10:18:38AM +0400, Stanislav Kinsbursky wrote: > 06.03.2013 09:06, Stanislav Kinsbursky ?????: > >06.03.2013 01:20, J. Bruce Fields ?????: > >>On Thu, Feb 28, 2013 at 03:09:59PM +0300, Stanislav Kinsbursky wrote: > >>>Function nfsd4_client_tracking_init() can return error. > >> > >>Before, I think that we the nfsd4_client_* functions just became no-ops > >>in this case. With the result that no client records get written, and > >>so clients are unable to reclaim on the next boot. > >> > >>Which is annoying, but possibly not as annoying as your server > >>completely refusing to start. > >> > >>It's arguably more helpful in the long run to fail immediately when we > >>recognize reboot recovery isn't going to work. But in practice this may > >>mean people that never knew they had a problem suddenly have servers > >>that don't start at all. > >> > >>So I'm inclined to be more forgiving and leave this as it is. But maybe > >>something like a warning printk would be appropriate. > >> > > > >Ok then. > >I'll add the warning anf convert the function to be "void" rather then "int". > >Thanks! > > > > We already have a warning. > So, probably all we need is just to change a prototype to make it less confusing. > Is it ok? Sure. And/or add a comment if you think it would be useful. --b. -- 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
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index f194f86..fc4b81b 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -69,6 +69,7 @@ static u64 current_sessionid = 1; /* forward declarations */ static int check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner); +void nfs4_state_shutdown_net(struct net *net); /* Locking: */ @@ -4942,7 +4943,9 @@ nfs4_state_start_net(struct net *net) ret = nfs4_state_create_net(net); if (ret) return ret; - nfsd4_client_tracking_init(net); + ret = nfsd4_client_tracking_init(net); + if (ret) + goto out_tracking; nn->boot_time = get_seconds(); locks_start_grace(net, &nn->nfsd4_manager); nn->grace_ended = false; @@ -4950,6 +4953,10 @@ nfs4_state_start_net(struct net *net) nn->nfsd4_grace, net); queue_delayed_work(laundry_wq, &nn->laundromat_work, nn->nfsd4_grace * HZ); return 0; + +out_tracking: + nfs4_state_destroy_net(net); + return ret; } /* initialization to perform when the nfsd service is started: */
Function nfsd4_client_tracking_init() can return error. Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com> --- fs/nfsd/nfs4state.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) -- 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