On Mon, 27 Dec 2010 18:38:56 +0300
Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> 2010/12/27 Jeff Layton <jlayton@xxxxxxxxxx>:
> > On Sun, 26 Dec 2010 13:04:17 +0300
> > Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> >
> >> Read from the cache if we have at least Level II oplock - otherwise
> >> read from the server. Add cifs_user_readv to let the client read into
> >> iovec buffers.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastryyy@xxxxxxxxx>
> >> ---
> >> fs/cifs/cifsfs.c | 4 +-
> >> fs/cifs/cifsfs.h | 6 ++-
> >> fs/cifs/file.c | 112 ++++++++++++++++++++++++++++++++++++++---------------
> >> 3 files changed, 87 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> >> index 2b52020..0d6b005 100644
> >> --- a/fs/cifs/cifsfs.c
> >> +++ b/fs/cifs/cifsfs.c
> >> @@ -714,7 +714,7 @@ const struct file_operations cifs_file_ops = {
> >> const struct file_operations cifs_file_strict_ops = {
> >> .read = do_sync_read,
> >> .write = do_sync_write,
> >> - .aio_read = generic_file_aio_read,
> >> + .aio_read = cifs_strict_readv,
> >> .aio_write = cifs_file_aio_write,
> >> .open = cifs_open,
> >> .release = cifs_close,
> >> @@ -770,7 +770,7 @@ const struct file_operations cifs_file_nobrl_ops = {
> >> const struct file_operations cifs_file_strict_nobrl_ops = {
> >> .read = do_sync_read,
> >> .write = do_sync_write,
> >> - .aio_read = generic_file_aio_read,
> >> + .aio_read = cifs_strict_readv,
> >> .aio_write = cifs_file_aio_write,
> >> .open = cifs_open,
> >> .release = cifs_close,
> >> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
> >> index 657738f..6ff3ca2 100644
> >> --- a/fs/cifs/cifsfs.h
> >> +++ b/fs/cifs/cifsfs.h
> >> @@ -81,7 +81,11 @@ extern int cifs_open(struct inode *inode, struct file *file);
> >> extern int cifs_close(struct inode *inode, struct file *file);
> >> extern int cifs_closedir(struct inode *inode, struct file *file);
> >> extern ssize_t cifs_user_read(struct file *file, char __user *read_data,
> >> - size_t read_size, loff_t *poffset);
> >> + size_t read_size, loff_t *poffset);
> >> +extern ssize_t cifs_user_readv(struct kiocb *iocb, const struct iovec *iov,
> >> + unsigned long nr_segs, loff_t pos);
> > ^^^^
> > this can be static. There are no callers of cifs_user_readv.
> >
> >> +extern ssize_t cifs_strict_readv(struct kiocb *iocb, const struct iovec *iov,
> >> + unsigned long nr_segs, loff_t pos);
> >> extern ssize_t cifs_user_write(struct file *file, const char __user *write_data,
> >> size_t write_size, loff_t *poffset);
> >> extern int cifs_lock(struct file *, int, struct file_lock *);
> >> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> >> index d48723c..c3bc65c 100644
> >> --- a/fs/cifs/file.c
> >> +++ b/fs/cifs/file.c
> >> @@ -1677,20 +1677,27 @@ int cifs_flush(struct file *file, fl_owner_t id)
> >> return rc;
> >> }
> >>
> >> -ssize_t cifs_user_read(struct file *file, char __user *read_data,
> >> - size_t read_size, loff_t *poffset)
> >> +static ssize_t
> >> +cifs_iovec_read(struct file *file, const struct iovec *iov,
> >> + unsigned long nr_segs, loff_t *poffset)
> >> {
> >> - int rc = -EACCES;
> >> - unsigned int bytes_read = 0;
> >> - unsigned int total_read = 0;
> >> - unsigned int current_read_size;
> >> + int rc;
> >> + int xid;
> >> + unsigned int total_read, bytes_read = 0;
> >> + size_t len, cur_len;
> >> + int iov_offset = 0;
> >> struct cifs_sb_info *cifs_sb;
> >> struct cifsTconInfo *pTcon;
> >> - int xid;
> >> struct cifsFileInfo *open_file;
> >> - char *smb_read_data;
> >> - char __user *current_offset;
> >> struct smb_com_read_rsp *pSMBr;
> >> + char *read_data;
> >> +
> >> + if (!nr_segs)
> >> + return 0;
> >> +
> >> + len = iov_length(iov, nr_segs);
> >> + if (!len)
> >> + return 0;
> >>
> >> xid = GetXid();
> >> cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
> >> @@ -1700,19 +1707,18 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
> >> FreeXid(xid);
> >> return rc;
> >> }
> >> +
> >> open_file = file->private_data;
> >> pTcon = tlink_tcon(open_file->tlink);
> >>
> >> if ((file->f_flags & O_ACCMODE) == O_WRONLY)
> >> cFYI(1, "attempting read on write only file instance");
> >>
> >> - for (total_read = 0, current_offset = read_data;
> >> - read_size > total_read;
> >> - total_read += bytes_read, current_offset += bytes_read) {
> >> - current_read_size = min_t(const int, read_size - total_read,
> >> - cifs_sb->rsize);
> >> + for (total_read = 0; total_read < len; total_read += bytes_read) {
> >> + cur_len = min_t(const size_t, len - total_read, cifs_sb->rsize);
> >> rc = -EAGAIN;
> >> - smb_read_data = NULL;
> >> + read_data = NULL;
> >> +
> >> while (rc == -EAGAIN) {
> >> int buf_type = CIFS_NO_BUFFER;
> >> if (open_file->invalidHandle) {
> >> @@ -1720,27 +1726,25 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
> >> if (rc != 0)
> >> break;
> >> }
> >> - rc = CIFSSMBRead(xid, pTcon,
> >> - open_file->netfid,
> >> - current_read_size, *poffset,
> >> - &bytes_read, &smb_read_data,
> >> - &buf_type);
> >> - pSMBr = (struct smb_com_read_rsp *)smb_read_data;
> >> - if (smb_read_data) {
> >> - if (copy_to_user(current_offset,
> >> - smb_read_data +
> >> - 4 /* RFC1001 length field */ +
> >> - le16_to_cpu(pSMBr->DataOffset),
> >> - bytes_read))
> >> + rc = CIFSSMBRead(xid, pTcon, open_file->netfid,
> >> + cur_len, *poffset, &bytes_read,
> >> + &read_data, &buf_type);
> >> + pSMBr = (struct smb_com_read_rsp *)read_data;
> >> + if (read_data) {
> >> + char *data_offset = read_data + 4 +
> >> + le16_to_cpu(pSMBr->DataOffset);
> >> + if (memcpy_toiovecend(iov, data_offset,
> >> + iov_offset, bytes_read))
> > ^^^^^
> > Much better. It's always better to use existing
> > infrastructure.
> >
> >> rc = -EFAULT;
> >> -
> >> if (buf_type == CIFS_SMALL_BUFFER)
> >> - cifs_small_buf_release(smb_read_data);
> >> + cifs_small_buf_release(read_data);
> >> else if (buf_type == CIFS_LARGE_BUFFER)
> >> - cifs_buf_release(smb_read_data);
> >> - smb_read_data = NULL;
> >> + cifs_buf_release(read_data);
> >> + read_data = NULL;
> >> + iov_offset += bytes_read;
> >> }
> >> }
> >> +
> >> if (rc || (bytes_read == 0)) {
> >> if (total_read) {
> >> break;
> >> @@ -1753,13 +1757,57 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
> >> *poffset += bytes_read;
> >> }
> >> }
> >> +
> >> FreeXid(xid);
> >> return total_read;
> >> }
> >>
> >> +ssize_t cifs_user_read(struct file *file, char __user *read_data,
> >> + size_t read_size, loff_t *poffset)
> >> +{
> >> + struct iovec iov;
> >> + iov.iov_base = read_data;
> >> + iov.iov_len = read_size;
> >> +
> >> + return cifs_iovec_read(file, &iov, 1, poffset);
> >> +}
> >> +
> >> +ssize_t cifs_user_readv(struct kiocb *iocb, const struct iovec *iov,
> >> + unsigned long nr_segs, loff_t pos)
> >> +{
> >> + ssize_t read;
> >> +
> >> + read = cifs_iovec_read(iocb->ki_filp, iov, nr_segs, &pos);
> >> + if (read > 0)
> >> + iocb->ki_pos = pos;
> >> +
> >> + return read;
> >> +}
> >> +
> >
> > Again, I'd make the above static. If you do that, the compiler will
> > likely just inline the function into the only caller below.
> >
> >> +ssize_t cifs_strict_readv(struct kiocb *iocb, const struct iovec *iov,
> >> + unsigned long nr_segs, loff_t pos)
> >> +{
> >> + struct inode *inode;
> >> +
> >> + inode = iocb->ki_filp->f_path.dentry->d_inode;
> >> +
> >> + if (CIFS_I(inode)->clientCanCacheRead)
> >> + return generic_file_aio_read(iocb, iov, nr_segs, pos);
> >> +
> >> + /*
> >> + * In strict cache mode we need to read from the server all the time
> >> + * if we don't have level II oplock because the server can delay mtime
> >> + * change - so we can't make a decision about inode invalidating.
> >> + * And we can also fail with pagereading if there are mandatory locks
> >> + * on pages affected by this read but not on the region from pos to
> >> + * pos+len-1.
> >> + */
> >> +
> >> + return cifs_user_readv(iocb, iov, nr_segs, pos);
> >> +}
> >>
> >> static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
> >> - loff_t *poffset)
> >> + loff_t *poffset)
> >> {
> >> int rc = -EACCES;
> >> unsigned int bytes_read = 0;
> >
> >
> > Nice work. Other than making that function static, this looks good to
> > me, so you can add my Reviewed-by once that's been fixed...
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxxx>
> >
>
> Jeff, I want to add cifs_user_readv/cifs_user_writev to direct_ops
> structure further - that's why I did these functions global.
>
Makes sense, but in general, it would be better to go ahead and make
it static and then convert it to a global when you have callers that
need it. That way we're not left with extra global symbols if that
conversion gets put off for some reason. Linux CIFS is rife with
examples of future plans that didn't pan out. :)
--
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
