[PATCH 1/1] exportfs: Stop racing exportfs on clusters

[PATCH 1/1] exportfs: Stop racing exportfs on clusters

[Steve Dickson]

This problem can occur when multiple cluster services fail over
at the same time, causing missing high-available exports.
Having a lot of nfs-exports will trigger this issue easier.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 support/include/exportfs.h |    4 ++++
 utils/exportfs/exportfs.c  |   24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/support/include/exportfs.h b/support/include/exportfs.h
index 01e87dd..99916e5 100644
--- a/support/include/exportfs.h
+++ b/support/include/exportfs.h
@@ -32,6 +32,10 @@ enum {
 	FSLOC_STUB
 };
 
+#ifndef EXP_LOCKFILE
+#define EXP_LOCKFILE "/var/lib/nfs/export-lock"
+#endif
+
 typedef struct mclient {
 	struct mclient *	m_next;
 	char *			m_hostname;
diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
index 7432a65..769d438 100644
--- a/utils/exportfs/exportfs.c
+++ b/utils/exportfs/exportfs.c
@@ -16,6 +16,7 @@
 #include <sys/stat.h>
 #include <sys/vfs.h>
 #include <sys/stat.h>
+#include <sys/file.h>
 #include <unistd.h>
 #include <stdbool.h>
 #include <stdlib.h>
@@ -44,6 +45,23 @@ static void	validate_export(nfs_export *exp);
 static int	matchhostname(const char *hostname1, const char *hostname2);
 static void	export_d_read(const char *dname);
 
+static const char *lockfile = EXP_LOCKFILE;
+static int _lockfd = -1;
+
+void 
+grab_lockfile()
+{
+	_lockfd = open(lockfile, O_CREAT|O_RDWR, 0666);
+	if (_lockfd != -1) 
+		lockf(_lockfd, F_LOCK, 0);
+}
+void 
+release_lockfile()
+{
+	if (_lockfd != -1)
+		lockf(_lockfd, F_ULOCK, 0);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -129,6 +147,12 @@ main(int argc, char **argv)
 			return 0;
 		}
 	}
+	/*
+	 * Serialize things as best we can
+	 */
+	grab_lockfile();
+	atexit(release_lockfile);
+
 	if (f_export && ! f_ignore) {
 		export_read(_PATH_EXPORTS);
 		export_d_read(_PATH_EXPORTS_D);
-- 
1.7.7.6

Re: [PATCH 1/1] exportfs: Stop racing exportfs on clusters

[Chuck Lever]

On Mar 7, 2012, at 2:56 PM, Steve Dickson wrote:

> This problem can occur when multiple cluster services fail over
> at the same time, causing missing high-available exports.
> Having a lot of nfs-exports will trigger this issue easier.

Does this address https://bugzilla.linux-nfs.org/show_bug.cgi?id=224 ?

> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
> support/include/exportfs.h |    4 ++++
> utils/exportfs/exportfs.c  |   24 ++++++++++++++++++++++++
> 2 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/support/include/exportfs.h b/support/include/exportfs.h
> index 01e87dd..99916e5 100644
> --- a/support/include/exportfs.h
> +++ b/support/include/exportfs.h
> @@ -32,6 +32,10 @@ enum {
> 	FSLOC_STUB
> };
> 
> +#ifndef EXP_LOCKFILE
> +#define EXP_LOCKFILE "/var/lib/nfs/export-lock"
> +#endif
> +
> typedef struct mclient {
> 	struct mclient *	m_next;
> 	char *			m_hostname;
> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> index 7432a65..769d438 100644
> --- a/utils/exportfs/exportfs.c
> +++ b/utils/exportfs/exportfs.c
> @@ -16,6 +16,7 @@
> #include <sys/stat.h>
> #include <sys/vfs.h>
> #include <sys/stat.h>
> +#include <sys/file.h>
> #include <unistd.h>
> #include <stdbool.h>
> #include <stdlib.h>
> @@ -44,6 +45,23 @@ static void	validate_export(nfs_export *exp);
> static int	matchhostname(const char *hostname1, const char *hostname2);
> static void	export_d_read(const char *dname);
> 
> +static const char *lockfile = EXP_LOCKFILE;
> +static int _lockfd = -1;
> +
> +void 
> +grab_lockfile()
> +{
> +	_lockfd = open(lockfile, O_CREAT|O_RDWR, 0666);
> +	if (_lockfd != -1) 
> +		lockf(_lockfd, F_LOCK, 0);
> +}
> +void 
> +release_lockfile()
> +{
> +	if (_lockfd != -1)
> +		lockf(_lockfd, F_ULOCK, 0);
> +}
> +
> int
> main(int argc, char **argv)
> {
> @@ -129,6 +147,12 @@ main(int argc, char **argv)
> 			return 0;
> 		}
> 	}
> +	/*
> +	 * Serialize things as best we can
> +	 */
> +	grab_lockfile();
> +	atexit(release_lockfile);
> +
> 	if (f_export && ! f_ignore) {
> 		export_read(_PATH_EXPORTS);
> 		export_d_read(_PATH_EXPORTS_D);
> -- 
> 1.7.7.6
> 
> --
> 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

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 1/1] exportfs: Stop racing exportfs on clusters

[Steve Dickson]

On 03/07/2012 03:07 PM, Chuck Lever wrote:
> 
> On Mar 7, 2012, at 2:56 PM, Steve Dickson wrote:
> 
>> This problem can occur when multiple cluster services fail over
>> at the same time, causing missing high-available exports.
>> Having a lot of nfs-exports will trigger this issue easier.
> 
> Does this address https://bugzilla.linux-nfs.org/show_bug.cgi?id=224 ?
Yes, I believe so. It also addresses 
    https://bugzilla.redhat.com/show_bug.cgi?id=800335

I guess I make this fix back in the RHEL4 days and the
patch never made into upstream. 

steved.

> 
>> Signed-off-by: Steve Dickson <steved@redhat.com>
>> ---
>> support/include/exportfs.h |    4 ++++
>> utils/exportfs/exportfs.c  |   24 ++++++++++++++++++++++++
>> 2 files changed, 28 insertions(+), 0 deletions(-)
>>
>> diff --git a/support/include/exportfs.h b/support/include/exportfs.h
>> index 01e87dd..99916e5 100644
>> --- a/support/include/exportfs.h
>> +++ b/support/include/exportfs.h
>> @@ -32,6 +32,10 @@ enum {
>> 	FSLOC_STUB
>> };
>>
>> +#ifndef EXP_LOCKFILE
>> +#define EXP_LOCKFILE "/var/lib/nfs/export-lock"
>> +#endif
>> +
>> typedef struct mclient {
>> 	struct mclient *	m_next;
>> 	char *			m_hostname;
>> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
>> index 7432a65..769d438 100644
>> --- a/utils/exportfs/exportfs.c
>> +++ b/utils/exportfs/exportfs.c
>> @@ -16,6 +16,7 @@
>> #include <sys/stat.h>
>> #include <sys/vfs.h>
>> #include <sys/stat.h>
>> +#include <sys/file.h>
>> #include <unistd.h>
>> #include <stdbool.h>
>> #include <stdlib.h>
>> @@ -44,6 +45,23 @@ static void	validate_export(nfs_export *exp);
>> static int	matchhostname(const char *hostname1, const char *hostname2);
>> static void	export_d_read(const char *dname);
>>
>> +static const char *lockfile = EXP_LOCKFILE;
>> +static int _lockfd = -1;
>> +
>> +void 
>> +grab_lockfile()
>> +{
>> +	_lockfd = open(lockfile, O_CREAT|O_RDWR, 0666);
>> +	if (_lockfd != -1) 
>> +		lockf(_lockfd, F_LOCK, 0);
>> +}
>> +void 
>> +release_lockfile()
>> +{
>> +	if (_lockfd != -1)
>> +		lockf(_lockfd, F_ULOCK, 0);
>> +}
>> +
>> int
>> main(int argc, char **argv)
>> {
>> @@ -129,6 +147,12 @@ main(int argc, char **argv)
>> 			return 0;
>> 		}
>> 	}
>> +	/*
>> +	 * Serialize things as best we can
>> +	 */
>> +	grab_lockfile();
>> +	atexit(release_lockfile);
>> +
>> 	if (f_export && ! f_ignore) {
>> 		export_read(_PATH_EXPORTS);
>> 		export_d_read(_PATH_EXPORTS_D);
>> -- 
>> 1.7.7.6
>>
>> --
>> 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
> 

Re: [PATCH 1/1] exportfs: Stop racing exportfs on clusters

[J. Bruce Fields]

On Wed, Mar 07, 2012 at 02:56:30PM -0500, Steve Dickson wrote:
> This problem can occur when multiple cluster services fail over
> at the same time, causing missing high-available exports.
> Having a lot of nfs-exports will trigger this issue easier.

Isn't the locking in support/export/xtab.c supposed to take care of
this?

I may be confused--I forget what all these files are for....

--b.

> 
> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
>  support/include/exportfs.h |    4 ++++
>  utils/exportfs/exportfs.c  |   24 ++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/support/include/exportfs.h b/support/include/exportfs.h
> index 01e87dd..99916e5 100644
> --- a/support/include/exportfs.h
> +++ b/support/include/exportfs.h
> @@ -32,6 +32,10 @@ enum {
>  	FSLOC_STUB
>  };
>  
> +#ifndef EXP_LOCKFILE
> +#define EXP_LOCKFILE "/var/lib/nfs/export-lock"
> +#endif
> +
>  typedef struct mclient {
>  	struct mclient *	m_next;
>  	char *			m_hostname;
> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> index 7432a65..769d438 100644
> --- a/utils/exportfs/exportfs.c
> +++ b/utils/exportfs/exportfs.c
> @@ -16,6 +16,7 @@
>  #include <sys/stat.h>
>  #include <sys/vfs.h>
>  #include <sys/stat.h>
> +#include <sys/file.h>
>  #include <unistd.h>
>  #include <stdbool.h>
>  #include <stdlib.h>
> @@ -44,6 +45,23 @@ static void	validate_export(nfs_export *exp);
>  static int	matchhostname(const char *hostname1, const char *hostname2);
>  static void	export_d_read(const char *dname);
>  
> +static const char *lockfile = EXP_LOCKFILE;
> +static int _lockfd = -1;
> +
> +void 
> +grab_lockfile()
> +{
> +	_lockfd = open(lockfile, O_CREAT|O_RDWR, 0666);
> +	if (_lockfd != -1) 
> +		lockf(_lockfd, F_LOCK, 0);
> +}
> +void 
> +release_lockfile()
> +{
> +	if (_lockfd != -1)
> +		lockf(_lockfd, F_ULOCK, 0);
> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> @@ -129,6 +147,12 @@ main(int argc, char **argv)
>  			return 0;
>  		}
>  	}
> +	/*
> +	 * Serialize things as best we can
> +	 */
> +	grab_lockfile();
> +	atexit(release_lockfile);
> +
>  	if (f_export && ! f_ignore) {
>  		export_read(_PATH_EXPORTS);
>  		export_d_read(_PATH_EXPORTS_D);
> -- 
> 1.7.7.6
> 
> --
> 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

Re: [PATCH 1/1] exportfs: Stop racing exportfs on clusters

[J. Bruce Fields]

On Wed, Mar 07, 2012 at 03:36:49PM -0500, bfields wrote:
> On Wed, Mar 07, 2012 at 02:56:30PM -0500, Steve Dickson wrote:
> > This problem can occur when multiple cluster services fail over
> > at the same time, causing missing high-available exports.
> > Having a lot of nfs-exports will trigger this issue easier.
> 
> Isn't the locking in support/export/xtab.c supposed to take care of
> this?

Huh, maybe that's just meant to prevent etab from getting corrupted, or
mountd from seeing a partially-written file.

But it probably doesn't prevent two exportfs processes from both reading
etab, adding an export, and each writing out an etab with its one export
added, with the last writer winning.

Was that the problem?

I think it'd be worth a comment above grab/release_lockfile().

--b.

> 
> I may be confused--I forget what all these files are for....
> 
> --b.
> 
> > 
> > Signed-off-by: Steve Dickson <steved@redhat.com>
> > ---
> >  support/include/exportfs.h |    4 ++++
> >  utils/exportfs/exportfs.c  |   24 ++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+), 0 deletions(-)
> > 
> > diff --git a/support/include/exportfs.h b/support/include/exportfs.h
> > index 01e87dd..99916e5 100644
> > --- a/support/include/exportfs.h
> > +++ b/support/include/exportfs.h
> > @@ -32,6 +32,10 @@ enum {
> >  	FSLOC_STUB
> >  };
> >  
> > +#ifndef EXP_LOCKFILE
> > +#define EXP_LOCKFILE "/var/lib/nfs/export-lock"
> > +#endif
> > +
> >  typedef struct mclient {
> >  	struct mclient *	m_next;
> >  	char *			m_hostname;
> > diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
> > index 7432a65..769d438 100644
> > --- a/utils/exportfs/exportfs.c
> > +++ b/utils/exportfs/exportfs.c
> > @@ -16,6 +16,7 @@
> >  #include <sys/stat.h>
> >  #include <sys/vfs.h>
> >  #include <sys/stat.h>
> > +#include <sys/file.h>
> >  #include <unistd.h>
> >  #include <stdbool.h>
> >  #include <stdlib.h>
> > @@ -44,6 +45,23 @@ static void	validate_export(nfs_export *exp);
> >  static int	matchhostname(const char *hostname1, const char *hostname2);
> >  static void	export_d_read(const char *dname);
> >  
> > +static const char *lockfile = EXP_LOCKFILE;
> > +static int _lockfd = -1;
> > +
> > +void 
> > +grab_lockfile()
> > +{
> > +	_lockfd = open(lockfile, O_CREAT|O_RDWR, 0666);
> > +	if (_lockfd != -1) 
> > +		lockf(_lockfd, F_LOCK, 0);
> > +}
> > +void 
> > +release_lockfile()
> > +{
> > +	if (_lockfd != -1)
> > +		lockf(_lockfd, F_ULOCK, 0);
> > +}
> > +
> >  int
> >  main(int argc, char **argv)
> >  {
> > @@ -129,6 +147,12 @@ main(int argc, char **argv)
> >  			return 0;
> >  		}
> >  	}
> > +	/*
> > +	 * Serialize things as best we can
> > +	 */
> > +	grab_lockfile();
> > +	atexit(release_lockfile);
> > +
> >  	if (f_export && ! f_ignore) {
> >  		export_read(_PATH_EXPORTS);
> >  		export_d_read(_PATH_EXPORTS_D);
> > -- 
> > 1.7.7.6
> > 
> > --
> > 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

Re: [PATCH 1/1] exportfs: Stop racing exportfs on clusters

[Chuck Lever]

On Mar 7, 2012, at 3:50 PM, J. Bruce Fields wrote:

> On Wed, Mar 07, 2012 at 03:36:49PM -0500, bfields wrote:
>> On Wed, Mar 07, 2012 at 02:56:30PM -0500, Steve Dickson wrote:
>>> This problem can occur when multiple cluster services fail over
>>> at the same time, causing missing high-available exports.
>>> Having a lot of nfs-exports will trigger this issue easier.
>> 
>> Isn't the locking in support/export/xtab.c supposed to take care of
>> this?
> 
> Huh, maybe that's just meant to prevent etab from getting corrupted, or
> mountd from seeing a partially-written file.
> 
> But it probably doesn't prevent two exportfs processes from both reading
> etab, adding an export, and each writing out an etab with its one export
> added, with the last writer winning.
> 
> Was that the problem?

For bug 224, yes.  After disparate concurrent exportfs commands race, the etab file ends up missing some exports.

> 
> I think it'd be worth a comment above grab/release_lockfile().
> 
> --b.
> 
>> 
>> I may be confused--I forget what all these files are for....
>> 
>> --b.
>> 
>>> 
>>> Signed-off-by: Steve Dickson <steved@redhat.com>
>>> ---
>>> support/include/exportfs.h |    4 ++++
>>> utils/exportfs/exportfs.c  |   24 ++++++++++++++++++++++++
>>> 2 files changed, 28 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/support/include/exportfs.h b/support/include/exportfs.h
>>> index 01e87dd..99916e5 100644
>>> --- a/support/include/exportfs.h
>>> +++ b/support/include/exportfs.h
>>> @@ -32,6 +32,10 @@ enum {
>>> 	FSLOC_STUB
>>> };
>>> 
>>> +#ifndef EXP_LOCKFILE
>>> +#define EXP_LOCKFILE "/var/lib/nfs/export-lock"
>>> +#endif
>>> +
>>> typedef struct mclient {
>>> 	struct mclient *	m_next;
>>> 	char *			m_hostname;
>>> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
>>> index 7432a65..769d438 100644
>>> --- a/utils/exportfs/exportfs.c
>>> +++ b/utils/exportfs/exportfs.c
>>> @@ -16,6 +16,7 @@
>>> #include <sys/stat.h>
>>> #include <sys/vfs.h>
>>> #include <sys/stat.h>
>>> +#include <sys/file.h>
>>> #include <unistd.h>
>>> #include <stdbool.h>
>>> #include <stdlib.h>
>>> @@ -44,6 +45,23 @@ static void	validate_export(nfs_export *exp);
>>> static int	matchhostname(const char *hostname1, const char *hostname2);
>>> static void	export_d_read(const char *dname);
>>> 
>>> +static const char *lockfile = EXP_LOCKFILE;
>>> +static int _lockfd = -1;
>>> +
>>> +void 
>>> +grab_lockfile()
>>> +{
>>> +	_lockfd = open(lockfile, O_CREAT|O_RDWR, 0666);
>>> +	if (_lockfd != -1) 
>>> +		lockf(_lockfd, F_LOCK, 0);
>>> +}
>>> +void 
>>> +release_lockfile()
>>> +{
>>> +	if (_lockfd != -1)
>>> +		lockf(_lockfd, F_ULOCK, 0);
>>> +}
>>> +
>>> int
>>> main(int argc, char **argv)
>>> {
>>> @@ -129,6 +147,12 @@ main(int argc, char **argv)
>>> 			return 0;
>>> 		}
>>> 	}
>>> +	/*
>>> +	 * Serialize things as best we can
>>> +	 */
>>> +	grab_lockfile();
>>> +	atexit(release_lockfile);
>>> +
>>> 	if (f_export && ! f_ignore) {
>>> 		export_read(_PATH_EXPORTS);
>>> 		export_d_read(_PATH_EXPORTS_D);
>>> -- 
>>> 1.7.7.6
>>> 
>>> --
>>> 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

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 1/1] exportfs: Stop racing exportfs on clusters

[Steve Dickson]

On 03/07/2012 03:36 PM, J. Bruce Fields wrote:
> On Wed, Mar 07, 2012 at 02:56:30PM -0500, Steve Dickson wrote:
>> This problem can occur when multiple cluster services fail over
>> at the same time, causing missing high-available exports.
>> Having a lot of nfs-exports will trigger this issue easier.
> 
> Isn't the locking in support/export/xtab.c supposed to take care of
> this?
> 
> I may be confused--I forget what all these files are for....
hmm... your right... those do take out flock()s during I/O..
There must be a hole somewhere they can get corrupted... 

steved.

> 
> --b.
> 
>>
>> Signed-off-by: Steve Dickson <steved@redhat.com>
>> ---
>>  support/include/exportfs.h |    4 ++++
>>  utils/exportfs/exportfs.c  |   24 ++++++++++++++++++++++++
>>  2 files changed, 28 insertions(+), 0 deletions(-)
>>
>> diff --git a/support/include/exportfs.h b/support/include/exportfs.h
>> index 01e87dd..99916e5 100644
>> --- a/support/include/exportfs.h
>> +++ b/support/include/exportfs.h
>> @@ -32,6 +32,10 @@ enum {
>>  	FSLOC_STUB
>>  };
>>  
>> +#ifndef EXP_LOCKFILE
>> +#define EXP_LOCKFILE "/var/lib/nfs/export-lock"
>> +#endif
>> +
>>  typedef struct mclient {
>>  	struct mclient *	m_next;
>>  	char *			m_hostname;
>> diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
>> index 7432a65..769d438 100644
>> --- a/utils/exportfs/exportfs.c
>> +++ b/utils/exportfs/exportfs.c
>> @@ -16,6 +16,7 @@
>>  #include <sys/stat.h>
>>  #include <sys/vfs.h>
>>  #include <sys/stat.h>
>> +#include <sys/file.h>
>>  #include <unistd.h>
>>  #include <stdbool.h>
>>  #include <stdlib.h>
>> @@ -44,6 +45,23 @@ static void	validate_export(nfs_export *exp);
>>  static int	matchhostname(const char *hostname1, const char *hostname2);
>>  static void	export_d_read(const char *dname);
>>  
>> +static const char *lockfile = EXP_LOCKFILE;
>> +static int _lockfd = -1;
>> +
>> +void 
>> +grab_lockfile()
>> +{
>> +	_lockfd = open(lockfile, O_CREAT|O_RDWR, 0666);
>> +	if (_lockfd != -1) 
>> +		lockf(_lockfd, F_LOCK, 0);
>> +}
>> +void 
>> +release_lockfile()
>> +{
>> +	if (_lockfd != -1)
>> +		lockf(_lockfd, F_ULOCK, 0);
>> +}
>> +
>>  int
>>  main(int argc, char **argv)
>>  {
>> @@ -129,6 +147,12 @@ main(int argc, char **argv)
>>  			return 0;
>>  		}
>>  	}
>> +	/*
>> +	 * Serialize things as best we can
>> +	 */
>> +	grab_lockfile();
>> +	atexit(release_lockfile);
>> +
>>  	if (f_export && ! f_ignore) {
>>  		export_read(_PATH_EXPORTS);
>>  		export_d_read(_PATH_EXPORTS_D);
>> -- 
>> 1.7.7.6
>>
>> --
>> 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

Re: [PATCH 1/1] exportfs: Stop racing exportfs on clusters

[Steve Dickson]

On 03/07/2012 03:52 PM, Chuck Lever wrote:
> 
> On Mar 7, 2012, at 3:50 PM, J. Bruce Fields wrote:
> 
>> On Wed, Mar 07, 2012 at 03:36:49PM -0500, bfields wrote:
>>> On Wed, Mar 07, 2012 at 02:56:30PM -0500, Steve Dickson wrote:
>>>> This problem can occur when multiple cluster services fail over
>>>> at the same time, causing missing high-available exports.
>>>> Having a lot of nfs-exports will trigger this issue easier.
>>>
>>> Isn't the locking in support/export/xtab.c supposed to take care of
>>> this?
>>
>> Huh, maybe that's just meant to prevent etab from getting corrupted, or
>> mountd from seeing a partially-written file.
>>
>> But it probably doesn't prevent two exportfs processes from both reading
>> etab, adding an export, and each writing out an etab with its one export
>> added, with the last writer winning.
>>
>> Was that the problem?
> 
> For bug 224, yes.  After disparate concurrent exportfs commands race, the etab file ends up missing some exports.
This morning I took another look at this and there does indeed need to
be some global lock to stop to serialize dueling exportfs commands. The
locks in xtab.c are just to granular....

steved.

Re: [PATCH 1/1] exportfs: Stop racing exportfs on clusters

[J. Bruce Fields]

On Thu, Mar 08, 2012 at 10:11:26AM -0500, Steve Dickson wrote:
> 
> 
> On 03/07/2012 03:52 PM, Chuck Lever wrote:
> > 
> > On Mar 7, 2012, at 3:50 PM, J. Bruce Fields wrote:
> > 
> >> On Wed, Mar 07, 2012 at 03:36:49PM -0500, bfields wrote:
> >>> On Wed, Mar 07, 2012 at 02:56:30PM -0500, Steve Dickson wrote:
> >>>> This problem can occur when multiple cluster services fail over
> >>>> at the same time, causing missing high-available exports.
> >>>> Having a lot of nfs-exports will trigger this issue easier.
> >>>
> >>> Isn't the locking in support/export/xtab.c supposed to take care of
> >>> this?
> >>
> >> Huh, maybe that's just meant to prevent etab from getting corrupted, or
> >> mountd from seeing a partially-written file.
> >>
> >> But it probably doesn't prevent two exportfs processes from both reading
> >> etab, adding an export, and each writing out an etab with its one export
> >> added, with the last writer winning.
> >>
> >> Was that the problem?
> > 
> > For bug 224, yes.  After disparate concurrent exportfs commands race, the etab file ends up missing some exports.
> This morning I took another look at this and there does indeed need to
> be some global lock to stop to serialize dueling exportfs commands. The
> locks in xtab.c are just to granular....

Right, makes sense.  So just in hopes that I don't ask the same question
next time this comes up, could we find a good spot for a comment, maybe
something like this?:

	/*
	 * If we aren't careful, changes made by exportfs can be lost
	 * when multiple exports process run at once:
	 *
	 *	exportfs process 1	exportfs process 2
	 *	------------------------------------------
	 *	reads etab version A	reads etab version A
	 *	adds new export B	adds new export C
	 *	writes A+B		writes A+C
	 
	 * The locking in support/export/xtab.c will prevent mountd from
	 * seeing a partially written version of etab, and will prevent 
	 * the two writers above from writing simultaneously and
	 * corrupting etab, but to prevent problems like the above we
	 * need this additional lock:
	 */

Uh, or maybe you can come up with something shorter.

--b.