[PATCH] cifs: fix O_APPEND on directio mounts

[PATCH] cifs: fix O_APPEND on directio mounts

[Jeff Layton]

The direct I/O write codepath for CIFS is done through
cifs_user_write(). That function does not currently call
generic_write_checks() so the file position isn't being properly set
when the file is opened with O_APPEND.  It's also not doing the other
"normal" checks that should be done for a write call.

The problem is currently that when you open a file with O_APPEND on a
mount with the directio mount option, the file position is set to the
beginning of the file. This makes any subsequent writes clobber the data
in the file starting at the beginning.

This seems to fix the problem in cursory testing. It is, however
important to note that NFS disallows the combination of
(O_DIRECT|O_APPEND). If my understanding is correct, the concern is
races with multiple clients appending to a file clobbering each others'
data. Since the write model for CIFS and NFS is pretty similar in this
regard, CIFS is probably subject to the same sort of races. What's
unclear to me is why this is a particular problem with O_DIRECT and not
with buffered writes...

Regardless, disallowing O_APPEND on an entire mount is probably not
reasonable, so we'll probably just have to deal with it and reevaluate
this flag combination when we get proper support for O_DIRECT. In the
meantime this patch at least fixes the existing problem.

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

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index ff14d14..cbefe1f 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -833,6 +833,10 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 		return -EBADF;
 	open_file = (struct cifsFileInfo *) file->private_data;
 
+	rc = generic_write_checks(file, poffset, &write_size, 0);
+	if (rc)
+		return rc;
+
 	xid = GetXid();
 
 	if (*poffset > file->f_path.dentry->d_inode->i_size)
-- 
1.5.5.1

Re: [PATCH] cifs: fix O_APPEND on directio mounts

[Steve French]

On Wed, Aug 27, 2008 at 7:24 PM, Jeff Layton <jlayton@redhat.com> wrote:
> The direct I/O write codepath for CIFS is done through
> cifs_user_write(). That function does not currently call
> generic_write_checks() so the file position isn't being properly set
> when the file is opened with O_APPEND.  It's also not doing the other
> "normal" checks that should be done for a write call.
>
> The problem is currently that when you open a file with O_APPEND on a
> mount with the directio mount option, the file position is set to the
> beginning of the file. This makes any subsequent writes clobber the data
> in the file starting at the beginning.

Is this important enough to go into stable after it goes to mainline
(seems like a good candidate)?

> This seems to fix the problem in cursory testing. It is, however
> important to note that NFS disallows the combination of
> (O_DIRECT|O_APPEND). If my understanding is correct, the concern is
> races with multiple clients appending to a file clobbering each others'
> data. Since the write model for CIFS and NFS is pretty similar in this
> regard,
At least for oplocked files (which NFSv3 does not have support for) we
could allow append.   (when appending on an O_DIRECT open or mount,
also might be possible to open with DENY_WRITE mode rather than
failing if races with other clients must be prevented but that could
break apps).

-- 
Thanks,

Steve

Re: [PATCH] cifs: fix O_APPEND on directio mounts

[Jeff Layton]

On Wed, 27 Aug 2008 21:24:29 -0500
"Steve French" <smfrench@gmail.com> wrote:

> On Wed, Aug 27, 2008 at 7:24 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > The direct I/O write codepath for CIFS is done through
> > cifs_user_write(). That function does not currently call
> > generic_write_checks() so the file position isn't being properly set
> > when the file is opened with O_APPEND.  It's also not doing the other
> > "normal" checks that should be done for a write call.
> >
> > The problem is currently that when you open a file with O_APPEND on a
> > mount with the directio mount option, the file position is set to the
> > beginning of the file. This makes any subsequent writes clobber the data
> > in the file starting at the beginning.
> 
> Is this important enough to go into stable after it goes to mainline
> (seems like a good candidate)?
> 

Possibly, though I'll couch this with the statement that this has seen
only very light testing (basically testing that it fixed the reported
problem). I've not regression tested it much at all. Still, it seems
unlikely to make anything worse and should only affect directio mounts.

> > This seems to fix the problem in cursory testing. It is, however
> > important to note that NFS disallows the combination of
> > (O_DIRECT|O_APPEND). If my understanding is correct, the concern is
> > races with multiple clients appending to a file clobbering each others'
> > data. Since the write model for CIFS and NFS is pretty similar in this
> > regard,
> At least for oplocked files (which NFSv3 does not have support for) we
> could allow append.   (when appending on an O_DIRECT open or mount,
> also might be possible to open with DENY_WRITE mode rather than
> failing if races with other clients must be prevented but that could
> break apps).
> 

Interesting. I hadn't considered oplocks or share/deny modes. We
probably could use those to our advantage here. I want to do a bit more
research into why NFS specifically denies this, and then maybe we can
see what can be done to mitigate it.

It's probably also time to consider properly handling O_DIRECT. I don't
think that will be too tough, but we'll probably have to consider the
best way to test it.
-- 
Jeff Layton <jlayton@redhat.com>