[PATCH] NFS: have string-based mount options default to "intr" mounts

[PATCH] NFS: have string-based mount options default to "intr" mounts

[Jeff Layton]

The current string-based mount options default to "nointr" mounts. This
is consistent with the current behavior of the struct-based mount
options for NFSv2/3, but inconsistent with v4.

I've just sent a nfs-utils patch to make the default be "intr" for all
NFS flavors. This patch makes the default the same when using
string-based mount options.

Signed-off-by: Jeff Layton <jlayton@redhat.com>

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index b2a851c..55f9b8b 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1122,7 +1122,9 @@ static int nfs_validate_mount_data(struct nfs_mount_data **options,
 		char *c;
 		int status;
 		struct nfs_parsed_mount_data args = {
-			.flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP),
+			.flags		= (NFS_MOUNT_INTR |
+					   NFS_MOUNT_VER3 |
+					   NFS_MOUNT_TCP),
 			.rsize		= NFS_MAX_FILE_IO_SIZE,
 			.wsize		= NFS_MAX_FILE_IO_SIZE,
 			.timeo		= 600,
@@ -1608,6 +1610,7 @@ static int nfs4_validate_mount_data(struct nfs4_mount_data **options,
 	default: {
 		unsigned int len;
 		struct nfs_parsed_mount_data args = {
+			.flags		= NFS4_MOUNT_INTR,
 			.rsize		= NFS_MAX_FILE_IO_SIZE,
 			.wsize		= NFS_MAX_FILE_IO_SIZE,
 			.timeo		= 600,

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

I thought "nointr" was the default because it is safest.

Jeff Layton wrote:
> The current string-based mount options default to "nointr" mounts. This
> is consistent with the current behavior of the struct-based mount
> options for NFSv2/3, but inconsistent with v4.
> 
> I've just sent a nfs-utils patch to make the default be "intr" for all
> NFS flavors. This patch makes the default the same when using
> string-based mount options.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index b2a851c..55f9b8b 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1122,7 +1122,9 @@ static int nfs_validate_mount_data(struct nfs_mount_data **options,
>  		char *c;
>  		int status;
>  		struct nfs_parsed_mount_data args = {
> -			.flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP),
> +			.flags		= (NFS_MOUNT_INTR |
> +					   NFS_MOUNT_VER3 |
> +					   NFS_MOUNT_TCP),
>  			.rsize		= NFS_MAX_FILE_IO_SIZE,
>  			.wsize		= NFS_MAX_FILE_IO_SIZE,
>  			.timeo		= 600,
> @@ -1608,6 +1610,7 @@ static int nfs4_validate_mount_data(struct nfs4_mount_data **options,
>  	default: {
>  		unsigned int len;
>  		struct nfs_parsed_mount_data args = {
> +			.flags		= NFS4_MOUNT_INTR,
>  			.rsize		= NFS_MAX_FILE_IO_SIZE,
>  			.wsize		= NFS_MAX_FILE_IO_SIZE,
>  			.timeo		= 600,
> _______________________________________________
> NFSv4 mailing list
> NFSv4@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Jeff Layton]

On Wed, 25 Jul 2007 15:36:18 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> I thought "nointr" was the default because it is safest.
> 

I could see maybe with soft mounts, but in conjunction with a hard
mount (which is the default) is there danger of corruption?


> Jeff Layton wrote:
> > The current string-based mount options default to "nointr" mounts. This
> > is consistent with the current behavior of the struct-based mount
> > options for NFSv2/3, but inconsistent with v4.
> > 
> > I've just sent a nfs-utils patch to make the default be "intr" for all
> > NFS flavors. This patch makes the default the same when using
> > string-based mount options.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > 
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index b2a851c..55f9b8b 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -1122,7 +1122,9 @@ static int nfs_validate_mount_data(struct nfs_mount_data **options,
> >  		char *c;
> >  		int status;
> >  		struct nfs_parsed_mount_data args = {
> > -			.flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP),
> > +			.flags		= (NFS_MOUNT_INTR |
> > +					   NFS_MOUNT_VER3 |
> > +					   NFS_MOUNT_TCP),
> >  			.rsize		= NFS_MAX_FILE_IO_SIZE,
> >  			.wsize		= NFS_MAX_FILE_IO_SIZE,
> >  			.timeo		= 600,
> > @@ -1608,6 +1610,7 @@ static int nfs4_validate_mount_data(struct nfs4_mount_data **options,
> >  	default: {
> >  		unsigned int len;
> >  		struct nfs_parsed_mount_data args = {
> > +			.flags		= NFS4_MOUNT_INTR,
> >  			.rsize		= NFS_MAX_FILE_IO_SIZE,
> >  			.wsize		= NFS_MAX_FILE_IO_SIZE,
> >  			.timeo		= 600,
> > _______________________________________________
> > NFSv4 mailing list
> > NFSv4@linux-nfs.org
> > http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
> 


-- 
Jeff Layton <jlayton@redhat.com>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]

Jeff Layton wrote:
> On Wed, 25 Jul 2007 15:36:18 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> I thought "nointr" was the default because it is safest.
>>
> 
> I could see maybe with soft mounts, but in conjunction with a hard
> mount (which is the default) is there danger of corruption?

If you hit ^C during a multi-segment write, I believe there is the 
possibility of data corruption.

>> Jeff Layton wrote:
>>> The current string-based mount options default to "nointr" mounts. This
>>> is consistent with the current behavior of the struct-based mount
>>> options for NFSv2/3, but inconsistent with v4.
>>>
>>> I've just sent a nfs-utils patch to make the default be "intr" for all
>>> NFS flavors. This patch makes the default the same when using
>>> string-based mount options.
>>>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>>
>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>> index b2a851c..55f9b8b 100644
>>> --- a/fs/nfs/super.c
>>> +++ b/fs/nfs/super.c
>>> @@ -1122,7 +1122,9 @@ static int nfs_validate_mount_data(struct nfs_mount_data **options,
>>>  		char *c;
>>>  		int status;
>>>  		struct nfs_parsed_mount_data args = {
>>> -			.flags		= (NFS_MOUNT_VER3 | NFS_MOUNT_TCP),
>>> +			.flags		= (NFS_MOUNT_INTR |
>>> +					   NFS_MOUNT_VER3 |
>>> +					   NFS_MOUNT_TCP),
>>>  			.rsize		= NFS_MAX_FILE_IO_SIZE,
>>>  			.wsize		= NFS_MAX_FILE_IO_SIZE,
>>>  			.timeo		= 600,
>>> @@ -1608,6 +1610,7 @@ static int nfs4_validate_mount_data(struct nfs4_mount_data **options,
>>>  	default: {
>>>  		unsigned int len;
>>>  		struct nfs_parsed_mount_data args = {
>>> +			.flags		= NFS4_MOUNT_INTR,
>>>  			.rsize		= NFS_MAX_FILE_IO_SIZE,
>>>  			.wsize		= NFS_MAX_FILE_IO_SIZE,
>>>  			.timeo		= 600,
>>> _______________________________________________
>>> NFSv4 mailing list
>>> NFSv4@linux-nfs.org
>>> http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4
> 
> 

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
NFSv4 mailing list
NFSv4@linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Trond Myklebust]

On Wed, 2007-07-25 at 15:41 -0400, Jeff Layton wrote:
> On Wed, 25 Jul 2007 15:36:18 -0400
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> > I thought "nointr" was the default because it is safest.
> > 
> 
> I could see maybe with soft mounts, but in conjunction with a hard
> mount (which is the default) is there danger of corruption?

You may cause the sync to disk on close() to be aborted. The writes will
eventually get flushed to disk by the flushd() daemon, but failing to
sync writes to disk prior to closing the file is a violation of
close-to-open caching semantics and may cause corruption issues.

Trond

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Peter Staubach]

Trond Myklebust wrote:
> On Wed, 2007-07-25 at 15:41 -0400, Jeff Layton wrote:
>   
>> On Wed, 25 Jul 2007 15:36:18 -0400
>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>>     
>>> I thought "nointr" was the default because it is safest.
>>>
>>>       
>> I could see maybe with soft mounts, but in conjunction with a hard
>> mount (which is the default) is there danger of corruption?
>>     
>
> You may cause the sync to disk on close() to be aborted. The writes will
> eventually get flushed to disk by the flushd() daemon, but failing to
> sync writes to disk prior to closing the file is a violation of
> close-to-open caching semantics and may cause corruption issues.

It sounds like the sync to disk on close() needs to be done in a
separate context and have the process in close() wait on it?  That
way, the process which received the signal could abort and return
without waiting on all of the data to get flushed, but the data
would still get flushed in a timely fashion?

I suspect that as soon as you throw in a close() which returns
EINTR, then all assumptions about close-to-open become null and
void.

    Thanx...

       ps

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Trond Myklebust]

On Wed, 2007-07-25 at 16:36 -0400, Peter Staubach wrote:

> It sounds like the sync to disk on close() needs to be done in a
> separate context and have the process in close() wait on it?  That
> way, the process which received the signal could abort and return
> without waiting on all of the data to get flushed, but the data
> would still get flushed in a timely fashion?

We could do that, but what kind of extra guarantees would that provide?
The whole question is what you mean by "timely fashion" here. Normally,
I'd expect that to mean "before any locks are freed", in which case I
don't see how a separate flush context will help here...

> I suspect that as soon as you throw in a close() which returns
> EINTR, then all assumptions about close-to-open become null and
> void.

Exactly. Byte range locks will be automatically freed as part of the
POSIX close() process, and the application itself may still survive for
long enough to release other locks (dotlocks etc...) that it may hold.

Trond

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Jeff Layton]

On Wed, 25 Jul 2007 16:16:57 -0400
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> On Wed, 2007-07-25 at 15:41 -0400, Jeff Layton wrote:
> > On Wed, 25 Jul 2007 15:36:18 -0400
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> > 
> > > I thought "nointr" was the default because it is safest.
> > > 
> > 
> > I could see maybe with soft mounts, but in conjunction with a hard
> > mount (which is the default) is there danger of corruption?
> 
> You may cause the sync to disk on close() to be aborted. The writes will
> eventually get flushed to disk by the flushd() daemon, but failing to
> sync writes to disk prior to closing the file is a violation of
> close-to-open caching semantics and may cause corruption issues.
> 
> Trond
> 

The close would return EINTR in this case though, wouldn't it? The app
should then be aware that it didn't actually happen and should
reattempt it. Presuming that the app handles this correctly, is it
really a data corruption issue?


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Peter Staubach]

Jeff Layton wrote:
> On Wed, 25 Jul 2007 16:16:57 -0400
> Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
>
>   
>> On Wed, 2007-07-25 at 15:41 -0400, Jeff Layton wrote:
>>     
>>> On Wed, 25 Jul 2007 15:36:18 -0400
>>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>>
>>>       
>>>> I thought "nointr" was the default because it is safest.
>>>>
>>>>         
>>> I could see maybe with soft mounts, but in conjunction with a hard
>>> mount (which is the default) is there danger of corruption?
>>>       
>> You may cause the sync to disk on close() to be aborted. The writes will
>> eventually get flushed to disk by the flushd() daemon, but failing to
>> sync writes to disk prior to closing the file is a violation of
>> close-to-open caching semantics and may cause corruption issues.
>>
>> Trond
>>
>>     
>
> The close would return EINTR in this case though, wouldn't it? The app
> should then be aware that it didn't actually happen and should
> reattempt it. Presuming that the app handles this correctly, is it
> really a data corruption issue?

You can't retry the operation.  Even if close() returns an error,
the file descriptor is really closed.

But, the application can take some other form of error recovery.

----

I think that I object the data corruption characterization.  The
data doesn't become corrupted.  It, eventually, is written out to
stable storage.  However, an application which concerned with
consistency needs to be able to do something valid when a close
operation returns an error, whether it is EINTR or something else.

    Thanx...

       ps

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Jeff Layton]

On Wed, 25 Jul 2007 17:07:15 -0400
Peter Staubach <staubach@redhat.com> wrote:

> Jeff Layton wrote:
> > On Wed, 25 Jul 2007 16:16:57 -0400
> > Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> >
> >   
> >> On Wed, 2007-07-25 at 15:41 -0400, Jeff Layton wrote:
> >>     
> >>> On Wed, 25 Jul 2007 15:36:18 -0400
> >>> Chuck Lever <chuck.lever@oracle.com> wrote:
> >>>
> >>>       
> >>>> I thought "nointr" was the default because it is safest.
> >>>>
> >>>>         
> >>> I could see maybe with soft mounts, but in conjunction with a hard
> >>> mount (which is the default) is there danger of corruption?
> >>>       
> >> You may cause the sync to disk on close() to be aborted. The writes will
> >> eventually get flushed to disk by the flushd() daemon, but failing to
> >> sync writes to disk prior to closing the file is a violation of
> >> close-to-open caching semantics and may cause corruption issues.
> >>
> >> Trond
> >>
> >>     
> >
> > The close would return EINTR in this case though, wouldn't it? The app
> > should then be aware that it didn't actually happen and should
> > reattempt it. Presuming that the app handles this correctly, is it
> > really a data corruption issue?
> 
> You can't retry the operation.  Even if close() returns an error,
> the file descriptor is really closed.
> 

Interesting. The close(2) manpage is unclear on this on this point.

> But, the application can take some other form of error recovery.
> 
> ----
> 
> I think that I object the data corruption characterization.  The
> data doesn't become corrupted.  It, eventually, is written out to
> stable storage.  However, an application which concerned with
> consistency needs to be able to do something valid when a close
> operation returns an error, whether it is EINTR or something else.
> 

That's my thinking too, though if you close and get an error, it's
not clear to me what that should be. There doesn't seem to be a way to
block waiting for the data to be flushed out. If the fd is closed, I
suppose you would have released any locks you have. Other machines
would be able to get in there before your data gets flushed to the
server.

Now that I think about it, I guess we have similar issues with nointr
if the server reboots at just the wrong time during a close call too.

In any case, I guess the question here is "what should the default be"?
Currently, it's nointr for nfs2/3 and intr for nfs4. If you use
string-based options, it's nointr everywhere. I think we ought to try to
make it consistent unless there's good reason not to. We should also
make sure the nfs manpage is correct.

--
Jeff Layton <jlayton@redhat.com>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS: have string-based mount options default to "intr" mounts

[Jeff Layton]

On Wed, 25 Jul 2007 19:48:06 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Wed, 25 Jul 2007 17:07:15 -0400
> Peter Staubach <staubach@redhat.com> wrote:
> 
> > Jeff Layton wrote:
> > > On Wed, 25 Jul 2007 16:16:57 -0400
> > > Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> > >
> > >   
> > >> On Wed, 2007-07-25 at 15:41 -0400, Jeff Layton wrote:
> > >>     
> > >>> On Wed, 25 Jul 2007 15:36:18 -0400
> > >>> Chuck Lever <chuck.lever@oracle.com> wrote:
> > >>>
> > >>>       
> > >>>> I thought "nointr" was the default because it is safest.
> > >>>>
> > >>>>         
> > >>> I could see maybe with soft mounts, but in conjunction with a hard
> > >>> mount (which is the default) is there danger of corruption?
> > >>>       
> > >> You may cause the sync to disk on close() to be aborted. The writes will
> > >> eventually get flushed to disk by the flushd() daemon, but failing to
> > >> sync writes to disk prior to closing the file is a violation of
> > >> close-to-open caching semantics and may cause corruption issues.
> > >>
> > >> Trond
> > >>
> > >>     
> > >
> > > The close would return EINTR in this case though, wouldn't it? The app
> > > should then be aware that it didn't actually happen and should
> > > reattempt it. Presuming that the app handles this correctly, is it
> > > really a data corruption issue?
> > 
> > You can't retry the operation.  Even if close() returns an error,
> > the file descriptor is really closed.
> > 
> 
> Interesting. The close(2) manpage is unclear on this on this point.
> 
> > But, the application can take some other form of error recovery.
> > 
> > ----
> > 
> > I think that I object the data corruption characterization.  The
> > data doesn't become corrupted.  It, eventually, is written out to
> > stable storage.  However, an application which concerned with
> > consistency needs to be able to do something valid when a close
> > operation returns an error, whether it is EINTR or something else.
> > 
> 
> That's my thinking too, though if you close and get an error, it's
> not clear to me what that should be. There doesn't seem to be a way to
> block waiting for the data to be flushed out. If the fd is closed, I
> suppose you would have released any locks you have. Other machines
> would be able to get in there before your data gets flushed to the
> server.
> 
> Now that I think about it, I guess we have similar issues with nointr
> if the server reboots at just the wrong time during a close call too.
> 

Actually, this is probably not correct. I was thinking that if the
server rebooted during a commit call issued on a close then the app
would need to reissue the writes. I don't think this is the case
though...

> In any case, I guess the question here is "what should the default be"?
> Currently, it's nointr for nfs2/3 and intr for nfs4. If you use
> string-based options, it's nointr everywhere. I think we ought to try to
> make it consistent unless there's good reason not to. We should also
> make sure the nfs manpage is correct.
> 
> --
> Jeff Layton <jlayton@redhat.com>
> 
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> NFS maillist  -  NFS@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs
> 


-- 
Jeff Layton <jlayton@poochiereds.net>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] NFS: have string-based mount options default to intr mounts

[Rick Macklem]

> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> > I thought "nointr" was the default because it is safest.
> > 

I was actually impressed that "intr" mounts work for NFSv4. I would have
thought interrupting an RPC that has state related (Open, Lock,...) op(s)
in it would cause grief later. (NFSERR_BADSEQID or similar)

Do you spin off kernel threads to babysit those outstanding RPCs? rick

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs