[PATCH] vfs: pass struct file to do_truncate on O_TRUNC opens

[PATCH] vfs: pass struct file to do_truncate on O_TRUNC opens

[Jeff Layton]

When a file is opened with O_TRUNC, the truncate processing is handled
by handle_truncate(). This function however doesn't receive any info
about the newly instantiated filp, and therefore can't pass that info
along so that the setattr can use it.

This makes NFSv4 misbehave. The client does an open and gets a valid
stateid, and then doesn't use that stateid on the subsequent truncate.
It uses the zero-stateid instead. Most servers ignore this fact and
just do the truncate anyway, but some don't like it (notably, RHEL4).

It seems more correct that since we have a fully instantiated file at
the time that handle_truncate is called, that we pass that along so
that the truncate operation can properly use it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/namei.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 1c0fca6..965a3bb 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1472,8 +1472,9 @@ int may_open(struct path *path, int acc_mode, int flag)
 	return break_lease(inode, flag);
 }
 
-static int handle_truncate(struct path *path)
+static int handle_truncate(struct file *filp)
 {
+	struct path *path = &filp->f_path;
 	struct inode *inode = path->dentry->d_inode;
 	int error = get_write_access(inode);
 	if (error)
@@ -1488,7 +1489,7 @@ static int handle_truncate(struct path *path)
 	if (!error) {
 		error = do_truncate(path->dentry, 0,
 				    ATTR_MTIME|ATTR_CTIME|ATTR_OPEN,
-				    NULL);
+				    filp);
 	}
 	put_write_access(inode);
 	return error;
@@ -1585,7 +1586,7 @@ static struct file *finish_open(struct nameidata *nd,
 	}
 	if (!IS_ERR(filp)) {
 		if (will_truncate) {
-			error = handle_truncate(&nd->path);
+			error = handle_truncate(filp);
 			if (error) {
 				fput(filp);
 				filp = ERR_PTR(error);
-- 
1.6.6.1

Re: [PATCH] vfs: pass struct file to do_truncate on O_TRUNC opens

[Trond Myklebust]

On Mon, 2010-03-15 at 17:39 -0400, Jeff Layton wrote: 
> When a file is opened with O_TRUNC, the truncate processing is handled
> by handle_truncate(). This function however doesn't receive any info
> about the newly instantiated filp, and therefore can't pass that info
> along so that the setattr can use it.
> 
> This makes NFSv4 misbehave. The client does an open and gets a valid
> stateid, and then doesn't use that stateid on the subsequent truncate.
> It uses the zero-stateid instead. Most servers ignore this fact and
> just do the truncate anyway, but some don't like it (notably, RHEL4).
> 
> It seems more correct that since we have a fully instantiated file at
> the time that handle_truncate is called, that we pass that along so
> that the truncate operation can properly use it.

At least in the O_CREAT|O_TRUNC case, we really should modify
nfs4_opendata_alloc() to set the attrs.ia_size to zero, so that the
server does an atomic open(O_CREAT|O_TRUNC) for us (see the DESCRIPTION
paragraph in RFC3530 section 14.2.16). There shouldn't be any need for
an extra truncate RPC call.

Plain open(O_TRUNC) probably does need something along the lines of what
you propose, however.

Cheers
  Trond 

Re: [PATCH] vfs: pass struct file to do_truncate on O_TRUNC opens

[Jeff Layton]

On Mon, 15 Mar 2010 18:31:28 -0400
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> On Mon, 2010-03-15 at 17:39 -0400, Jeff Layton wrote: 
> > When a file is opened with O_TRUNC, the truncate processing is handled
> > by handle_truncate(). This function however doesn't receive any info
> > about the newly instantiated filp, and therefore can't pass that info
> > along so that the setattr can use it.
> > 
> > This makes NFSv4 misbehave. The client does an open and gets a valid
> > stateid, and then doesn't use that stateid on the subsequent truncate.
> > It uses the zero-stateid instead. Most servers ignore this fact and
> > just do the truncate anyway, but some don't like it (notably, RHEL4).
> > 
> > It seems more correct that since we have a fully instantiated file at
> > the time that handle_truncate is called, that we pass that along so
> > that the truncate operation can properly use it.
> 
> At least in the O_CREAT|O_TRUNC case, we really should modify
> nfs4_opendata_alloc() to set the attrs.ia_size to zero, so that the
> server does an atomic open(O_CREAT|O_TRUNC) for us (see the DESCRIPTION
> paragraph in RFC3530 section 14.2.16). There shouldn't be any need for
> an extra truncate RPC call.
> 
> Plain open(O_TRUNC) probably does need something along the lines of what
> you propose, however.
> 

Yeah that would be cleaner on the wire for the create case. It's not
clear to me how to implement that though. There doesn't appear to be
a straightforward way to tell the VFS to skip the truncate.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] vfs: pass struct file to do_truncate on O_TRUNC opens

[Al Viro]

On Mon, Mar 15, 2010 at 06:47:02PM -0400, Jeff Layton wrote:

> Yeah that would be cleaner on the wire for the create case. It's not
> clear to me how to implement that though. There doesn't appear to be
> a straightforward way to tell the VFS to skip the truncate.

The plan is to turn most of the guts of do_last() into fs method...

Re: [PATCH] vfs: pass struct file to do_truncate on O_TRUNC opens

[Jeff Layton]

On Mon, 15 Mar 2010 22:50:37 +0000
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Mon, Mar 15, 2010 at 06:47:02PM -0400, Jeff Layton wrote:
> 
> > Yeah that would be cleaner on the wire for the create case. It's not
> > clear to me how to implement that though. There doesn't appear to be
> > a straightforward way to tell the VFS to skip the truncate.
> 
> The plan is to turn most of the guts of do_last() into fs method...

Thanks Al, that makes sense.

Does the patch I've proposed look reasonable in the interim?
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] vfs: pass struct file to do_truncate on O_TRUNC opens

[Valerie Aurora]

On Mon, Mar 15, 2010 at 10:50:37PM +0000, Al Viro wrote:
> On Mon, Mar 15, 2010 at 06:47:02PM -0400, Jeff Layton wrote:
> 
> > Yeah that would be cleaner on the wire for the create case. It's not
> > clear to me how to implement that though. There doesn't appear to be
> > a straightforward way to tell the VFS to skip the truncate.
> 
> The plan is to turn most of the guts of do_last() into fs method...

For union mounts, we will want to skip the truncate too (or else
insinuate ourselves into do_truncate() - less attractive).  This is
because we want to only copy up the bytes that are not being
truncated, instead of copying up the file contents and then throwing
them away.

What's the timing on the do_last() change?  Do you know what branch it
will be in?

Thanks,

-VAL