[linux-cifs] Re: [PATCH v1 8/8] cifs: convert cifs_iovec_write to use async writes


2012/2/21 Jeff Layton :
> On Tue, 21 Feb 2012 12:06:49 +0400
> Pavel Shilovsky  wrote:
>
>> 2012/2/3 Jeff Layton :
>> > Signed-off-by: Jeff Layton 
>> > ---
>> > 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.

This message from: http://www.mailbrowse.com/linux-cifs/5489.html
Previous message: Re: [linux-cifs-client] [PATCH] cifs: hard mount option behaviour implementation
Next message:Re: [PATCH v2] CIFS: Fix mkdir/rmdir bug for the non-POSIX case