Jeff,
We should cleanup compile warning - was planning to add a tiny patch
to fix if no objection.
Look correct?
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index daaaca8..460d87b 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2114,7 +2114,7 @@ cifs_uncached_marshal_iov(struct kvec *iov,
struct cifs_writedata *wdata)
/* marshal up the pages into iov array */
for (i = 0; i < wdata->nr_pages; i++) {
- iov[i + 1].iov_len = min(bytes, PAGE_SIZE);
+ iov[i + 1].iov_len = min_t(size_t, bytes, PAGE_SIZE);
iov[i + 1].iov_base = kmap(wdata->pages[i]);
bytes -= iov[i + 1].iov_len;
}
On Tue, Feb 21, 2012 at 6:02 AM, Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> 2012/2/21 Jeff Layton <jlayton@xxxxxxxxxx>:
>> On Tue, 21 Feb 2012 12:06:49 +0400
>> Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
>>
>>> 2012/2/3 Jeff Layton <jlayton@xxxxxxxxxx>:
>>> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>> > ---
>>> > fs/cifs/cifsproto.h | 2 +
>>> > fs/cifs/cifssmb.c | 4 +-
>>> > fs/cifs/file.c | 191 ++++++++++++++++++++++++++++++++-------------------
>>> > 3 files changed, 126 insertions(+), 71 deletions(-)
>>> >
>>> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>>> > index bceadc5..a131ded 100644
>>> > --- a/fs/cifs/cifsproto.h
>>> > +++ b/fs/cifs/cifsproto.h
>>> > @@ -473,6 +473,8 @@ int cifs_async_readv(struct cifs_readdata *rdata);
>>> > /* asynchronous write support */
>>> > struct cifs_writedata {
>>> > struct kref refcount;
>>> > + struct list_head list;
>>> > + struct completion done;
>>> > enum writeback_sync_modes sync_mode;
>>> > struct work_struct work;
>>> > struct cifsFileInfo *cfile;
>>> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>>> > index 113b5af..6e93c9e 100644
>>> > --- a/fs/cifs/cifssmb.c
>>> > +++ b/fs/cifs/cifssmb.c
>>> > @@ -2034,8 +2034,10 @@ cifs_writedata_alloc(unsigned int nr_pages, work_func_t complete)
>>> > wdata = kzalloc(sizeof(*wdata) +
>>> > sizeof(struct page *) * (nr_pages - 1), GFP_NOFS);
>>> > if (wdata != NULL) {
>>> > - INIT_WORK(&wdata->work, complete);
>>> > kref_init(&wdata->refcount);
>>> > + INIT_LIST_HEAD(&wdata->list);
>>> > + init_completion(&wdata->done);
>>> > + INIT_WORK(&wdata->work, complete);
>>> > }
>>> > return wdata;
>>> > }
>>> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>>> > index c7e47a2..ef25fc5 100644
>>> > --- a/fs/cifs/file.c
>>> > +++ b/fs/cifs/file.c
>>> > @@ -2057,23 +2057,55 @@ size_t get_numpages(const size_t wsize, const size_t len, size_t *cur_len)
>>> > return num_pages;
>>> > }
>>> >
>>> > +static void
>>> > +cifs_uncached_marshal_iov(struct kvec *iov, struct cifs_writedata *wdata)
>>> > +{
>>> > + int i;
>>> > + size_t bytes = wdata->bytes;
>>> > +
>>> > + /* marshal up the pages into iov array */
>>> > + for (i = 0; i < wdata->nr_pages; i++) {
>>> > + iov[i + 1].iov_len = min(bytes, PAGE_SIZE);
>>> > + iov[i + 1].iov_base = kmap(wdata->pages[i]);
>>> > + bytes -= iov[i + 1].iov_len;
>>> > + }
>>> > +}
>>> > +
>>> > +static void
>>> > +cifs_uncached_writev_complete(struct work_struct *work)
>>> > +{
>>> > + int i;
>>> > + struct cifs_writedata *wdata = container_of(work,
>>> > + struct cifs_writedata, work);
>>> > +
>>> > + complete(&wdata->done);
>>> > +
>>> > + if (wdata->result != -EAGAIN) {
>>> > + for (i = 0; i < wdata->nr_pages; i++)
>>> > + put_page(wdata->pages[i]);
>>> > + }
>>> > +
>>> > + kref_put(&wdata->refcount, cifs_writedata_release);
>>> > +}
>>> > +
>>> > static ssize_t
>>> > cifs_iovec_write(struct file *file, const struct iovec *iov,
>>> > unsigned long nr_segs, loff_t *poffset)
>>> > {
>>> > - unsigned int written;
>>> > - unsigned long num_pages, npages, i;
>>> > + unsigned long nr_pages, i;
>>> > size_t copied, len, cur_len;
>>> > ssize_t total_written = 0;
>>> > - struct kvec *to_send;
>>> > - struct page **pages;
>>> > + loff_t offset = *poffset;
>>> > struct iov_iter it;
>>> > - struct inode *inode;
>>> > + struct inode *inode = file->f_path.dentry->d_inode;
>>> > + struct cifsInodeInfo *cifsi = CIFS_I(inode);
>>> > struct cifsFileInfo *open_file;
>>> > struct cifs_tcon *tcon;
>>> > struct cifs_sb_info *cifs_sb;
>>> > - int xid, rc;
>>> > - __u32 pid;
>>> > + struct cifs_writedata *wdata, *tmp;
>>> > + struct list_head wdata_list;
>>> > + int rc;
>>> > + pid_t pid;
>>> >
>>> > len = iov_length(iov, nr_segs);
>>> > if (!len)
>>> > @@ -2083,103 +2115,122 @@ cifs_iovec_write(struct file *file, const struct iovec *iov,
>>> > if (rc)
>>> > return rc;
>>> >
>>> > + INIT_LIST_HEAD(&wdata_list);
>>> > cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
>>> > - num_pages = get_numpages(cifs_sb->wsize, len, &cur_len);
>>> > -
>>> > - pages = kmalloc(sizeof(struct pages *)*num_pages, GFP_KERNEL);
>>> > - if (!pages)
>>> > - return -ENOMEM;
>>> > -
>>> > - to_send = kmalloc(sizeof(struct kvec)*(num_pages + 1), GFP_KERNEL);
>>> > - if (!to_send) {
>>> > - kfree(pages);
>>> > - return -ENOMEM;
>>> > - }
>>> > -
>>> > - rc = cifs_write_allocate_pages(pages, num_pages);
>>> > - if (rc) {
>>> > - kfree(pages);
>>> > - kfree(to_send);
>>> > - return rc;
>>> > - }
>>> > -
>>> > - xid = GetXid();
>>> > open_file = file->private_data;
>>> > + tcon = tlink_tcon(open_file->tlink);
>>> >
>>> > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_RWPIDFORWARD)
>>> > pid = open_file->pid;
>>> > else
>>> > pid = current->tgid;
>>> >
>>> > - tcon = tlink_tcon(open_file->tlink);
>>> > - inode = file->f_path.dentry->d_inode;
>>> > -
>>> > iov_iter_init(&it, iov, nr_segs, len, 0);
>>> > - npages = num_pages;
>>> > -
>>> > do {
>>> > - size_t save_len = cur_len;
>>> > - for (i = 0; i < npages; i++) {
>>> > - copied = min_t(const size_t, cur_len, PAGE_CACHE_SIZE);
>>> > - copied = iov_iter_copy_from_user(pages[i], &it, 0,
>>> > - copied);
>>> > + size_t save_len;
>>> > +
>>> > + nr_pages = get_numpages(cifs_sb->wsize, len, &cur_len);
>>> > + wdata = cifs_writedata_alloc(nr_pages,
>>> > + cifs_uncached_writev_complete);
>>> > + if (!wdata) {
>>> > + rc = -ENOMEM;
>>> > + break;
>>> > + }
>>> > +
>>> > + rc = cifs_write_allocate_pages(wdata->pages, nr_pages);
>>> > + if (rc) {
>>> > + kfree(wdata);
>>> > + break;
>>> > + }
>>> > +
>>> > + save_len = cur_len;
>>> > + for (i = 0; i < nr_pages; i++) {
>>> > + copied = min_t(const size_t, cur_len, PAGE_SIZE);
>>> > + copied = iov_iter_copy_from_user(wdata->pages[i], &it,
>>> > + 0, copied);
>>> > cur_len -= copied;
>>> > iov_iter_advance(&it, copied);
>>> > - to_send[i+1].iov_base = kmap(pages[i]);
>>> > - to_send[i+1].iov_len = copied;
>>> > }
>>> > -
>>> > cur_len = save_len - cur_len;
>>> >
>>> > + wdata->sync_mode = WB_SYNC_ALL;
>>> > + wdata->nr_pages = nr_pages;
>>> > + wdata->offset = (__u64)offset;
>>> > + wdata->cfile = cifsFileInfo_get(open_file);
>>> > + wdata->pid = pid;
>>> > + wdata->bytes = cur_len;
>>> > + wdata->marshal_iov = cifs_uncached_marshal_iov;
>>> > do {
>>> > if (open_file->invalidHandle) {
>>> > rc = cifs_reopen_file(open_file, false);
>>> > if (rc != 0)
>>> > break;
>>> > }
>>>
>>> When we reopen a file, we do it with the current pid, but in
>>> cfile->pid leaves with the old value (and pid variable too).Should we
>>> do reopen with cfile->pid to save the original state of the file (now
>>> we don't restore locks, if we do it - it will be the problem)?
>>>
>>
>> Yes, that does sound like something that should be fixed. That said, we
>> have a lot of problems in the code that handles open files, and that's
>> just one of them. At this point, I'm not going to fix that in this
>> patchset since it's an existing bug and unrelated to what I'm changing
>> here. It certainly wouldn't hurt to fix that at some point though.
>>
>
> I agree - this doesn't related to the patch, it simply pointed me to
> this existing problem.
>
> --
> Best regards,
> Pavel Shilovsky.
--
Thanks,
Steve
--
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
