* [REGRESSION] NFSv4: open(O_TRUNC) hangs
@ 2012-04-03 16:26 Miklos Szeredi
2012-04-05 17:31 ` Myklebust, Trond
0 siblings, 1 reply; 5+ messages in thread
From: Miklos Szeredi @ 2012-04-03 16:26 UTC (permalink / raw)
To: Trond.Myklebust; +Cc: bfields, linux-nfs, linux-fsdevel, linux-kernel
Now this test program hangs on latest git.
Thanks,
Miklos
---
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
int main(int argc, char *argv[])
{
int res;
char *name = argv[1];
int fd;
unlink(name);
fd = creat(name, 0644);
write(fd, "123", 3);
close(fd);
close(open(name, O_RDONLY | O_TRUNC));
return 0;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [REGRESSION] NFSv4: open(O_TRUNC) hangs
2012-04-03 16:26 [REGRESSION] NFSv4: open(O_TRUNC) hangs Miklos Szeredi
@ 2012-04-05 17:31 ` Myklebust, Trond
2012-04-06 9:43 ` Miklos Szeredi
0 siblings, 1 reply; 5+ messages in thread
From: Myklebust, Trond @ 2012-04-05 17:31 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: bfields, linux-nfs, linux-fsdevel, linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2063 bytes --]
On Tue, 2012-04-03 at 18:26 +0200, Miklos Szeredi wrote:
> Now this test program hangs on latest git.
>
> Thanks,
> Miklos
> ---
>
> #include <stdio.h>
> #include <unistd.h>
> #include <fcntl.h>
>
> int main(int argc, char *argv[])
> {
> int res;
> char *name = argv[1];
> int fd;
>
> unlink(name);
> fd = creat(name, 0644);
> write(fd, "123", 3);
> close(fd);
> close(open(name, O_RDONLY | O_TRUNC));
>
> return 0;
> }
Hi Miklos,
The following client patch fixes the regression for me.
Cheers
Trond
8<----------------------------------------------------
>From 5ec0ed6cd88c65464092533787706d9ecfb85320 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Wed, 4 Apr 2012 10:51:33 -0700
Subject: [PATCH 1/2] NFSv4: Don't use open stateids that have the wrong open
mode
If the client is doing an operation that needs a particular open
mode, then nfs4_select_rw_stateid() should only copy the
open stateid if the latter has the correct open mode.
Otherwise we should just use the zero stateid.
Reported-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
fs/nfs/nfs4state.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 0f43414..71dcfd4 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -932,6 +932,13 @@ void nfs4_select_rw_stateid(nfs4_stateid *dst, struct nfs4_state *state,
{
if (nfs4_copy_delegation_stateid(dst, state->inode, fmode))
return;
+
+ fmode &= FMODE_READ|FMODE_WRITE;
+ if ((state->state & fmode) != fmode) {
+ nfs4_stateid_copy(dst, &zero_stateid);
+ return;
+ }
+
if (nfs4_copy_lock_stateid(dst, state, fl_owner, fl_pid))
return;
nfs4_copy_open_stateid(dst, state);
--
1.7.7.6
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@netapp.com
www.netapp.com
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [REGRESSION] NFSv4: open(O_TRUNC) hangs
2012-04-05 17:31 ` Myklebust, Trond
@ 2012-04-06 9:43 ` Miklos Szeredi
2012-04-18 22:03 ` Myklebust, Trond
0 siblings, 1 reply; 5+ messages in thread
From: Miklos Szeredi @ 2012-04-06 9:43 UTC (permalink / raw)
To: Myklebust, Trond; +Cc: bfields, linux-nfs, linux-fsdevel, linux-kernel
"Myklebust, Trond" <Trond.Myklebust@netapp.com> writes:
> The following client patch fixes the regression for me.
It fixes the hang, but it still doesn't work 100% correctly. Try the
following test program.
BTW, do you run any fs test suits? All these were caught with one I use
to quick test fuse (it's in the fuse git tree(*) under the "test"
directory). But I guess others like LTP would catch these as well.
Thanks,
Miklos
(*) git://fuse.git.sourceforge.net/gitroot/fuse/fuse
---
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
int main(int argc, char *argv[])
{
int res;
char *name = argv[1];
unlink(name);
close(creat(name, 0400));
res = open(name, O_RDONLY | O_TRUNC);
if (res != -1 && errno != EPERM)
fprintf(stderr, "should have failed!\n");
return 0;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [REGRESSION] NFSv4: open(O_TRUNC) hangs
2012-04-06 9:43 ` Miklos Szeredi
@ 2012-04-18 22:03 ` Myklebust, Trond
2012-04-19 11:27 ` Miklos Szeredi
0 siblings, 1 reply; 5+ messages in thread
From: Myklebust, Trond @ 2012-04-18 22:03 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: bfields, linux-nfs, linux-fsdevel, linux-kernel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4169 bytes --]
Hi Miklos
On Fri, 2012-04-06 at 11:43 +0200, Miklos Szeredi wrote:
> "Myklebust, Trond" <Trond.Myklebust@netapp.com> writes:
>
> > The following client patch fixes the regression for me.
>
> It fixes the hang, but it still doesn't work 100% correctly. Try the
> following test program.
>
> BTW, do you run any fs test suits? All these were caught with one I use
> to quick test fuse (it's in the fuse git tree(*) under the "test"
> directory). But I guess others like LTP would catch these as well.
Thanks! I'll look into that. Bryan has been helping me to set up a test
rig for the NFS client, but for now we don't have much coverage of basic
POSIX tests.
> Thanks,
> Miklos
>
> (*) git://fuse.git.sourceforge.net/gitroot/fuse/fuse
> ---
>
> #include <stdio.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <errno.h>
>
> int main(int argc, char *argv[])
> {
> int res;
> char *name = argv[1];
>
> unlink(name);
> close(creat(name, 0400));
> res = open(name, O_RDONLY | O_TRUNC);
> if (res != -1 && errno != EPERM)
> fprintf(stderr, "should have failed!\n");
Shouldn't that be EACCES? As far as I know, POSIX doesn't list EPERM as
an allowed return value for open().
>
> return 0;
> }
I'm trying to find out what the correct behaviour should be. Clearly we
should not be hanging, and so we definitely do need a fix. However
according to POSIX, the behaviour of open(O_RDONLY|O_TRUNC) is
undefined. I'm therefore thinking that returning EACCES should be
acceptable in the case where the server returns NFS4ERR_OPENMODE to our
open stateid (which is the case for the Linux server).
Something like the following will 'break' your previous test, in that it
returns EACCES to the open(O_RDONLY|O_TRUNC) instead of truncating the
file. However as I said, that appears to be POSIX-compliant.
Cheers
Trond
8<-----------------------------------------------------------------------
>From f3efde63559c33a906dc325b192ecdb6c590c0f6 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Wed, 18 Apr 2012 16:29:11 -0400
Subject: [PATCH] NFSv4: Fix open(O_TRUNC) and ftruncate() error handling
If the file wasn't opened for writing, then truncate and ftruncate
need to report the appropriate errors.
Reported-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: stable@vger.kernel.org
---
fs/nfs/dir.c | 4 ++--
fs/nfs/nfs4proc.c | 15 ++++++++++++---
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 4aaf031..8789210 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1429,7 +1429,7 @@ static struct dentry *nfs_atomic_lookup(struct inode *dir, struct dentry *dentry
}
open_flags = nd->intent.open.flags;
- attr.ia_valid = 0;
+ attr.ia_valid = ATTR_OPEN;
ctx = create_nfs_open_context(dentry, open_flags);
res = ERR_CAST(ctx);
@@ -1536,7 +1536,7 @@ static int nfs_open_revalidate(struct dentry *dentry, struct nameidata *nd)
if (IS_ERR(ctx))
goto out;
- attr.ia_valid = 0;
+ attr.ia_valid = ATTR_OPEN;
if (openflags & O_TRUNC) {
attr.ia_valid |= ATTR_SIZE;
attr.ia_size = 0;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ba837d9..f875cf3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1954,10 +1954,19 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
};
int err;
do {
- err = nfs4_handle_exception(server,
- _nfs4_do_setattr(inode, cred, fattr, sattr, state),
- &exception);
+ err = _nfs4_do_setattr(inode, cred, fattr, sattr, state);
+ switch (err) {
+ case -NFS4ERR_OPENMODE:
+ if (state && !(state->state & FMODE_WRITE)) {
+ err = -EBADF;
+ if (sattr->ia_valid & ATTR_OPEN)
+ err = -EACCES;
+ goto out;
+ }
+ }
+ err = nfs4_handle_exception(server, err, &exception);
} while (exception.retry);
+out:
return err;
}
--
1.7.7.6
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@netapp.com
www.netapp.com
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [REGRESSION] NFSv4: open(O_TRUNC) hangs
2012-04-18 22:03 ` Myklebust, Trond
@ 2012-04-19 11:27 ` Miklos Szeredi
0 siblings, 0 replies; 5+ messages in thread
From: Miklos Szeredi @ 2012-04-19 11:27 UTC (permalink / raw)
To: Myklebust, Trond; +Cc: bfields, linux-nfs, linux-fsdevel, linux-kernel
"Myklebust, Trond" <Trond.Myklebust@netapp.com> writes:
> Hi Miklos
>
> On Fri, 2012-04-06 at 11:43 +0200, Miklos Szeredi wrote:
>> "Myklebust, Trond" <Trond.Myklebust@netapp.com> writes:
>>
>> > The following client patch fixes the regression for me.
>>
>> It fixes the hang, but it still doesn't work 100% correctly. Try the
>> following test program.
>>
>> BTW, do you run any fs test suits? All these were caught with one I use
>> to quick test fuse (it's in the fuse git tree(*) under the "test"
>> directory). But I guess others like LTP would catch these as well.
>
> Thanks! I'll look into that. Bryan has been helping me to set up a test
> rig for the NFS client, but for now we don't have much coverage of basic
> POSIX tests.
>
>> Thanks,
>> Miklos
>>
>> (*) git://fuse.git.sourceforge.net/gitroot/fuse/fuse
>> ---
>>
>> #include <stdio.h>
>> #include <unistd.h>
>> #include <fcntl.h>
>> #include <errno.h>
>>
>> int main(int argc, char *argv[])
>> {
>> int res;
>> char *name = argv[1];
>>
>> unlink(name);
>> close(creat(name, 0400));
>> res = open(name, O_RDONLY | O_TRUNC);
>> if (res != -1 && errno != EPERM)
>> fprintf(stderr, "should have failed!\n");
>
> Shouldn't that be EACCES? As far as I know, POSIX doesn't list EPERM as
> an allowed return value for open().
Yes it should be EACCESS. That test is broken, it should be:
if (res != -1 || errno != EACCES)
The one in the fuse tree is correct though...
Thanks,
Miklos
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-04-19 11:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-03 16:26 [REGRESSION] NFSv4: open(O_TRUNC) hangs Miklos Szeredi
2012-04-05 17:31 ` Myklebust, Trond
2012-04-06 9:43 ` Miklos Szeredi
2012-04-18 22:03 ` Myklebust, Trond
2012-04-19 11:27 ` Miklos Szeredi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).