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.
--
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
