On Tue, 2 Nov 2010 20:59:00 +0300
Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> 2010/11/2 Jeff Layton <jlayton@xxxxxxxxxx>:
> > On Tue, 2 Nov 2010 12:02:24 +0300
> > Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> >
> >> Modify cifs_file_aio_write and cifs_write_end to let the client works with
> >> strict cache mode.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastryyy@xxxxxxxxx>
> >> ---
> >> fs/cifs/cifsfs.c | 35 ++++++++++++++++++++++++++++++-----
> >> fs/cifs/file.c | 14 ++++++++++++--
> >> 2 files changed, 42 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> >> index 1b44a92..e1ecd35 100644
> >> --- a/fs/cifs/cifsfs.c
> >> +++ b/fs/cifs/cifsfs.c
> >> @@ -602,12 +602,37 @@ static ssize_t cifs_file_aio_read(struct kiocb
> >> *iocb, const struct iovec *iov,
> >> static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
> >> unsigned long nr_segs, loff_t pos)
> >> {
> >> - struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
> >> - ssize_t written;
> >> + struct inode *inode;
> >> + struct cifs_sb_info *cifs_sb;
> >> + ssize_t written, cache_written;
> >> + loff_t saved_pos;
> >> +
> >> + inode = iocb->ki_filp->f_path.dentry->d_inode;
> >> +
> >> + if (CIFS_I(inode)->clientCanCacheAll)
> >> + return generic_file_aio_write(iocb, iov, nr_segs, pos);
> >> +
> >> + cifs_sb = CIFS_SB(iocb->ki_filp->f_path.dentry->d_sb);
> >> +
> >> + if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) == 0) {
> >> + written = generic_file_aio_write(iocb, iov, nr_segs, pos);
> >> + filemap_write_and_wait(inode->i_mapping);
> > ^^^^^^^^^^^^^^^^^^
> > You can't ignore the return code from this. That function
> > may return an error if writeback fails. Also, I don't
> > see any need to wait on the result in this case. Why
> > not just kick off the I/O and return (do a
> > filemap_fdatawrite, IOW).
>
> I don't change non-strict variant for writing - we have this code now
> in the git tree. But I agree - we should think about return value in
> this case.
>
> >
> >> + return written;
> >> + }
> >> +
> >> + saved_pos = pos;
> >> + written = cifs_user_write(iocb->ki_filp, iov->iov_base,
> >> + iov->iov_len, &pos);
> >> +
> >> + if (written > 0) {
> >> + cache_written = generic_file_aio_write(iocb, iov,
> >> + nr_segs, saved_pos);
> >> + if (cache_written != written)
> >> + cERROR(1, "Cache written and server written data "
> >> + "lengths are different");
> >> + } else
> >> + iocb->ki_pos = pos;
> >>
> > ^^^^^
> > This seems awfully complicated. Why not just do a
> > generic_file_aio_write to get this into the cache and then just do a
> > filemap_write_and_wait and deal with the result?
>
> The main reason of doing this is mandatory byte-range locks. If we
> simply do generic_file_aio_write and then filemap_write_and_wait we
> can fail in the following situation:
> 1) process1 opens file and sets a mandatory lock from 0 to 1.
> 2) process2 opens file and writes a data from 1 to 2.
>
> If we do like you suggest we fail on page writing (on
> filemap_write_and_wait which writes whole page - from 0 to 2). That's
> why I do cifs_user_write which writes the data from 1 to 2 (it's what
> we need) and then store it in the cache by generic_file_aio_write
> (with the little change in cifs_write_end that doesn't write the same
> data twice to the server). So, right working of write and read ops
> with mandatory locks is one of the reasons to provide srict cache
> semantic.
>
Why store it in the cache at all at that point then? Is that for mmap?
This seems really ugly and doesn't pass the "sniff test" -- something
smells foul here. There must be a better way to handle this sort of
thing...
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
