From: Jeff Layton Subject: Re: [PATCH 1/3] mount.nfs: fix retry option settings with binary mount options Date: Wed, 9 Apr 2008 15:30:21 -0400 Message-ID: <20080409153021.3947925b@barsoom.rdu.redhat.com> References: <1206822608-20154-1-git-send-email-jlayton@redhat.com> <1206822608-20154-2-git-send-email-jlayton@redhat.com> <47FD0447.9030704@RedHat.com> <20080409141323.0c594185@barsoom.rdu.redhat.com> <47FD15FB.6090208@RedHat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: linux-nfs@vger.kernel.org, nfsv4@linux-nfs.org To: Steve Dickson Return-path: In-Reply-To: <47FD15FB.6090208@RedHat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: nfsv4-bounces@linux-nfs.org Errors-To: nfsv4-bounces@linux-nfs.org List-ID: On Wed, 09 Apr 2008 15:16:11 -0400 Steve Dickson wrote: > > > Jeff Layton wrote: > > On Wed, 09 Apr 2008 14:00:39 -0400 > > Steve Dickson wrote: > > > >> Jeff Layton wrote: > >>> Currently nfs4mount() sets the retry value to 10000 on both fg and bg > >>> mounts. It should be 2 for fg and 10000 for bg. nfsmount() sets it > >>> properly, but there is a potential corner case. If someone explicitly > >>> sets retry=10000 on a fg mount, then it will be reset to 2. > >>> > >>> Fix this by having retry default to -1 for both flavors, and then reset if > >>> needed after the mount options have been parsed. > >>> > >>> Signed-off-by: Jeff Layton > >>> --- > >>> utils/mount/nfs4mount.c | 10 +++++++++- > >>> utils/mount/nfsmount.c | 12 ++++++++---- > >>> 2 files changed, 17 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c > >>> index 311e5a0..af70551 100644 > >>> --- a/utils/mount/nfs4mount.c > >>> +++ b/utils/mount/nfs4mount.c > >>> @@ -238,7 +238,7 @@ int nfs4mount(const char *spec, const char *node, int flags, > >>> nocto = 0; > >>> noac = 0; > >>> unshared = 0; > >>> - retry = 10000; /* 10000 minutes ~ 1 week */ > >>> + retry = -1; > >>> > >>> /* > >>> * NFSv4 specifies that the default port should be 2049 > >>> @@ -332,6 +332,14 @@ int nfs4mount(const char *spec, const char *node, int flags, > >>> } > >>> } > >>> > >>> + /* if retry is still -1, then it wasn't set via an option */ > >>> + if (retry == -1) { > >>> + if (bg) > >>> + retry = 10000; /* 10000 mins == ~1 week */ > >>> + else > >>> + retry = 2; /* 2 min default on fg mounts */ > >>> + } > >>> + > >>> data.flags = (soft ? NFS4_MOUNT_SOFT : 0) > >>> | (intr ? NFS4_MOUNT_INTR : 0) > >>> | (nocto ? NFS4_MOUNT_NOCTO : 0) > >>> diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c > >>> index ff0ff93..27c46a7 100644 > >>> --- a/utils/mount/nfsmount.c > >>> +++ b/utils/mount/nfsmount.c > >>> @@ -571,7 +571,7 @@ nfsmount(const char *spec, const char *node, int flags, > >>> #endif > >>> > >>> bg = 0; > >>> - retry = 10000; /* 10000 minutes ~ 1 week */ > >>> + retry = -1; > >>> > >>> memset(mnt_pmap, 0, sizeof(*mnt_pmap)); > >>> mnt_pmap->pm_prog = MOUNTPROG; > >>> @@ -585,9 +585,13 @@ nfsmount(const char *spec, const char *node, int flags, > >>> goto fail; > >>> if (!nfsmnt_check_compat(nfs_pmap, mnt_pmap)) > >>> goto fail; > >>> - > >>> - if (retry == 10000 && !bg) > >>> - retry = 2; /* reset for fg mounts */ > >>> + > >>> + if (retry == -1) { > >>> + if (bg) > >>> + retry = 10000; /* 10000 mins == ~1 week*/ > >>> + else > >>> + retry = 2; /* 2 min default on fg mounts */ > >>> + } > >>> > >>> #ifdef NFS_MOUNT_DEBUG > >>> printf(_("rsize = %d, wsize = %d, timeo = %d, retrans = %d\n"), > >> Jeff, > >> > >> I believe all thats needed is to add the same code nfsmount() uses > >> reset retry into nfs4mount(). Something like: > >> > >> --- a/utils/mount/nfs4mount.c > >> +++ b/utils/mount/nfs4mount.c > >> @@ -332,6 +332,9 @@ int nfs4mount(const char *spec, const char *node, int flags, > >> } > >> } > >> > >> + if (retry == 10000 && !bg) > >> + retry = 2; /* reset for fg mounts */ > >> + > >> data.flags = (soft ? NFS4_MOUNT_SOFT : 0) > >> | (intr ? NFS4_MOUNT_INTR : 0) > >> | (nocto ? NFS4_MOUNT_NOCTO : 0) > >> > >> or missing something... > >> > >> > >> steved. > > > > That shouldn't be needed. > You misunderstood or I was not clear... only these three lines > are needed to do the same thing your entire patch does... > > > With this patch, retry defaults to -1. After > > we parse the options if it's still -1, then we reset it to whatever the > > default should be. > I guess I don't see much difference if retry is initialized to 10000 > or -1... its still initialized... Yes. But like I mentioned in the description, there is a potential corner case here. If someone does a foreground mount and explicitly sets retry=10000 then it will be reset to 2. It's not very likely, but is simple enough to prevent... -- Jeff Layton