On Tue, 12 Apr 2011 13:43:07 +0400
Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> 2011/4/2 Jeff Layton <jlayton@xxxxxxxxxx>:
> > Add the ability for CIFS to do an asynchronous write. The kernel will
> > set the frame up as it would for a "normal" SMBWrite2 request, and use
> > cifs_call_async to send it. The mid callback will then be configured to
> > handle the result.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > Âfs/cifs/cifsproto.h | Â 19 +++++
> > Âfs/cifs/cifssmb.c  | Â219 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > Â2 files changed, 238 insertions(+), 0 deletions(-)
>
> Looks good too, except two small notes.
>
Thanks for looking. I respun this yesterday actually and the new patch
is a bit different. With the new patchset, we'll no longer be
constrained by the current wsize limit of 14 pages. It should be able
to handle an arbitrary wsize, which should also help performance.
> >
> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > index e255a2b..7b964df 100644
> > --- a/fs/cifs/cifsproto.h
> > +++ b/fs/cifs/cifsproto.h
> > @@ -21,6 +21,7 @@
> > Â#ifndef _CIFSPROTO_H
> > Â#define _CIFSPROTO_H
> > Â#include <linux/nls.h>
> > +#include <linux/pagevec.h>
> >
> > Âstruct statfs;
> > Âstruct smb_vol;
> > @@ -433,4 +434,22 @@ extern int mdfour(unsigned char *, unsigned char *, int);
> > Âextern int E_md4hash(const unsigned char *passwd, unsigned char *p16);
> > Âextern int SMBencrypt(unsigned char *passwd, const unsigned char *c8,
> > Â Â Â Â Â Â Â Â Â Â Â Âunsigned char *p24);
> > +
> > +/* asynchronous write support */
> > +struct cifs_writedata {
> > +    struct list_head    Âpending;
> > +    struct kref       refcount;
> > +    struct completion    completion;
> > +    struct work_struct   Âwork;
> > +    struct cifsFileInfo   *cfile;
> > + Â Â Â __u64 Â Â Â Â Â Â Â Â Â offset;
> > +    unsigned int      Âbytes;
> > +    int           result;
> > +    struct page       *pages[PAGEVEC_SIZE];
> > +};
> > +
> > +int cifs_async_writev(struct cifs_writedata *wdata);
> > +struct cifs_writedata *cifs_writedata_alloc(void);
> > +void cifs_writedata_release(struct kref *refcount);
> > +
> > Â#endif         /* _CIFSPROTO_H */
> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > index 8de7e79..b308605 100644
> > --- a/fs/cifs/cifssmb.c
> > +++ b/fs/cifs/cifssmb.c
> > @@ -32,6 +32,7 @@
> > Â#include <linux/vfs.h>
> > Â#include <linux/slab.h>
> > Â#include <linux/posix_acl_xattr.h>
> > +#include <linux/pagemap.h>
> > Â#include <asm/uaccess.h>
> > Â#include "cifspdu.h"
> > Â#include "cifsglob.h"
> > @@ -1604,6 +1605,224 @@ CIFSSMBWrite(const int xid, struct cifs_tcon *tcon,
> > Â Â Â Âreturn rc;
> > Â}
> >
> > +void
> > +cifs_writedata_release(struct kref *refcount)
> > +{
> > + Â Â Â struct cifs_writedata *wdata = container_of(refcount,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct cifs_writedata, refcount);
> > + Â Â Â struct address_space *mapping;
> > +
> > + Â Â Â if (wdata->cfile) {
> > + Â Â Â Â Â Â Â mapping = wdata->cfile->dentry->d_inode->i_mapping;
> > +
> > + Â Â Â Â Â Â Â spin_lock(&mapping->private_lock);
> > + Â Â Â Â Â Â Â list_del(&wdata->pending);
> > + Â Â Â Â Â Â Â spin_unlock(&mapping->private_lock);
> > +
> > + Â Â Â Â Â Â Â cifsFileInfo_put(wdata->cfile);
> > + Â Â Â }
> > +
> > + Â Â Â kfree(wdata);
> > +}
> > +
> > +static void
> > +cifs_writev_complete(struct work_struct *work)
> > +{
> > + Â Â Â struct cifs_writedata *wdata = container_of(work,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct cifs_writedata, work);
> > + Â Â Â struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
> ^^^ this variable is unused.
>
Not exactly.
> > + Â Â Â struct inode *inode = wdata->cfile->dentry->d_inode;
> > + Â Â Â struct page *page;
> > + Â Â Â int i = 0;
> > +
> > + Â Â Â if (wdata->result == 0) {
> > + Â Â Â Â Â Â Â cifs_update_eof(CIFS_I(inode), wdata->offset, wdata->bytes);
> > + Â Â Â Â Â Â Â cifs_stats_bytes_written(tcon, wdata->bytes);
^^^^^
The tcon is used here
> > + Â Â Â }
> > +
> > + Â Â Â for (i = 0; i < ARRAY_SIZE(wdata->pages) && (page = wdata->pages[i]);
> > + Â Â Â Â Â Â Â Â Â Â Â i++) {
> > + Â Â Â Â Â Â Â if (wdata->result == -EAGAIN)
> > + Â Â Â Â Â Â Â Â Â Â Â __set_page_dirty_nobuffers(page);
> > + Â Â Â Â Â Â Â else if (wdata->result < 0)
> > + Â Â Â Â Â Â Â Â Â Â Â SetPageError(page);
> > + Â Â Â Â Â Â Â end_page_writeback(page);
> > + Â Â Â Â Â Â Â page_cache_release(page);
> > + Â Â Â }
> > + Â Â Â if (wdata->result != -EAGAIN)
> > + Â Â Â Â Â Â Â mapping_set_error(inode->i_mapping, wdata->result);
> > + Â Â Â complete(&wdata->completion);
> > + Â Â Â kref_put(&wdata->refcount, cifs_writedata_release);
> > +}
> > +
> > +struct cifs_writedata *
> > +cifs_writedata_alloc(void)
> > +{
> > + Â Â Â struct cifs_writedata *wdata;
> > +
> > + Â Â Â wdata = kzalloc(sizeof(*wdata), GFP_NOFS);
> > + Â Â Â if (wdata != NULL) {
> > + Â Â Â Â Â Â Â INIT_LIST_HEAD(&wdata->pending);
> > + Â Â Â Â Â Â Â INIT_WORK(&wdata->work, cifs_writev_complete);
> > + Â Â Â Â Â Â Â kref_init(&wdata->refcount);
> > + Â Â Â Â Â Â Â init_completion(&wdata->completion);
> > + Â Â Â }
> > + Â Â Â return wdata;
> > +}
> > +
> > +/*
> > + * Check the midState and signature on received buffer (if any), and queue the
> > + * workqueue completion task.
> > + */
> > +static void
> > +cifs_writev_callback(struct mid_q_entry *mid)
> > +{
> > + Â Â Â struct cifs_writedata *wdata = mid->callback_data;
> > + Â Â Â struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
> > + Â Â Â unsigned int written;
> > + Â Â Â WRITE_RSP *smb = (WRITE_RSP *)mid->resp_buf;
> > +
> > + Â Â Â switch(mid->midState) {
> > + Â Â Â case MID_RESPONSE_RECEIVED:
> > + Â Â Â Â Â Â Â wdata->result = cifs_check_receive(mid, tcon->ses->server, 0);
> > + Â Â Â Â Â Â Â if (wdata->result != 0)
> > + Â Â Â Â Â Â Â Â Â Â Â break;
> > +
> > + Â Â Â Â Â Â Â written = le16_to_cpu(smb->CountHigh);
> > + Â Â Â Â Â Â Â written <<= 16;
> > + Â Â Â Â Â Â Â written += le16_to_cpu(smb->Count);
> > + Â Â Â Â Â Â Â /*
> > + Â Â Â Â Â Â Â Â* Mask off high 16 bits when bytes written as returned
> > + Â Â Â Â Â Â Â Â* by the server is greater than bytes requested by the
> > + Â Â Â Â Â Â Â Â* client. OS/2 servers are known to set incorrect
> > + Â Â Â Â Â Â Â Â* CountHigh values.
> > + Â Â Â Â Â Â Â Â*/
> > + Â Â Â Â Â Â Â if (written > wdata->bytes)
> > + Â Â Â Â Â Â Â Â Â Â Â written &= 0xFFFF;
> > +
> > + Â Â Â Â Â Â Â if (written < wdata->bytes)
> > + Â Â Â Â Â Â Â Â Â Â Â wdata->result = -ENOSPC;
> > + Â Â Â Â Â Â Â else
> > + Â Â Â Â Â Â Â Â Â Â Â wdata->bytes = written;
> > + Â Â Â Â Â Â Â break;
> > + Â Â Â case MID_REQUEST_SUBMITTED:
> > + Â Â Â case MID_RETRY_NEEDED:
> > + Â Â Â Â Â Â Â wdata->result = -EAGAIN;
> > + Â Â Â Â Â Â Â break;
> > + Â Â Â default:
> > + Â Â Â Â Â Â Â wdata->result = -EIO;
> > + Â Â Â Â Â Â Â break;
> > + Â Â Â }
> > +
> > + Â Â Â queue_work(system_nrt_wq, &wdata->work);
> > + Â Â Â DeleteMidQEntry(mid);
> > + Â Â Â atomic_dec(&tcon->ses->server->inFlight);
> > + Â Â Â wake_up(&tcon->ses->server->request_q);
> > +}
> > +
> > +/* cifs_async_writev - send an async write, and set up mid to handle result */
> > +int
> > +cifs_async_writev(struct cifs_writedata *wdata)
> > +{
> > + Â Â Â int i, rc = -EACCES;
> > + Â Â Â WRITE_REQ *smb = NULL;
> > + Â Â Â int wct;
> > + Â Â Â struct cifs_tcon *tcon = tlink_tcon(wdata->cfile->tlink);
> > + Â Â Â struct inode *inode = wdata->cfile->dentry->d_inode;
> > + Â Â Â unsigned int npages = ARRAY_SIZE(wdata->pages);
> > + Â Â Â struct kvec *iov = NULL;
> > +
> > + Â Â Â cFYI(1, "async write at %llu %u bytes", wdata->offset, wdata->bytes);
> ^^^^ bytes is
> zero here. You calculate it next - I suggest to move this info message
> after you get right bytes value.
>
Good catch. Will fix.
> > +
> > + Â Â Â if (tcon->ses->capabilities & CAP_LARGE_FILES) {
> > + Â Â Â Â Â Â Â wct = 14;
> > + Â Â Â } else {
> > + Â Â Â Â Â Â Â wct = 12;
> > + Â Â Â Â Â Â Â if (wdata->offset >> 32 > 0) {
> > + Â Â Â Â Â Â Â Â Â Â Â /* can not handle big offset for old srv */
> > + Â Â Â Â Â Â Â Â Â Â Â return -EIO;
> > + Â Â Â Â Â Â Â }
> > + Â Â Â }
> > +
> > + Â Â Â rc = small_smb_init(SMB_COM_WRITE_ANDX, wct, tcon, (void **)&smb);
> > + Â Â Â if (rc)
> > + Â Â Â Â Â Â Â goto async_writev_out;
> > +
> > + Â Â Â /* FIXME: only allocate number of kvecs we need */
> > + Â Â Â iov = kzalloc((npages + 1) * sizeof(*iov), GFP_NOFS);
> > + Â Â Â if (iov == NULL) {
> > + Â Â Â Â Â Â Â rc = -ENOMEM;
> > + Â Â Â Â Â Â Â goto async_writev_out;
> > + Â Â Â }
> > +
> > + Â Â Â smb->AndXCommand = 0xFF; Â Â Â Â/* none */
> > + Â Â Â smb->Fid = wdata->cfile->netfid;
> > + Â Â Â smb->OffsetLow = cpu_to_le32(wdata->offset & 0xFFFFFFFF);
> > + Â Â Â if (wct == 14)
> > + Â Â Â Â Â Â Â smb->OffsetHigh = cpu_to_le32(wdata->offset >> 32);
> > + Â Â Â smb->Reserved = 0xFFFFFFFF;
> > + Â Â Â smb->WriteMode = 0;
> > + Â Â Â smb->Remaining = 0;
> > +
> > + Â Â Â smb->DataOffset =
> > + Â Â Â Â Â cpu_to_le16(offsetof(struct smb_com_write_req, Data) - 4);
> > +
> > + Â Â Â /* 4 for RFC1001 length + 1 for BCC */
> > + Â Â Â iov[0].iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4 + 1;
> > + Â Â Â iov[0].iov_base = smb;
> > +
> > + Â Â Â /* marshal up the pages into iov array */
> > + Â Â Â npages = 0;
> > + Â Â Â wdata->bytes = 0;
> > + Â Â Â for (i = 0; i < ARRAY_SIZE(wdata->pages) && wdata->pages[i]; i++) {
> > + Â Â Â Â Â Â Â if (wdata->bytes + PAGE_CACHE_SIZE <=
> > + Â Â Â Â Â Â Â Â Â CIFS_SB(inode->i_sb)->wsize) {
> > + Â Â Â Â Â Â Â Â Â Â Â iov[i + 1].iov_len = min(inode->i_size -
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â page_offset(wdata->pages[i]),
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (loff_t)PAGE_CACHE_SIZE);
> > + Â Â Â Â Â Â Â Â Â Â Â iov[i + 1].iov_base = kmap(wdata->pages[i]);
> > + Â Â Â Â Â Â Â Â Â Â Â wdata->bytes += iov[i + 1].iov_len;
> > + Â Â Â Â Â Â Â Â Â Â Â npages++;
> > + Â Â Â Â Â Â Â } else {
> > + Â Â Â Â Â Â Â Â Â Â Â /* if we're beyond wsize, then re-mark page dirty */
> > + Â Â Â Â Â Â Â Â Â Â Â __set_page_dirty_nobuffers(wdata->pages[i]);
> > + Â Â Â Â Â Â Â }
> > + Â Â Â }
> > +
> > + Â Â Â smb->DataLengthLow = cpu_to_le16(wdata->bytes & 0xFFFF);
> > + Â Â Â smb->DataLengthHigh = cpu_to_le16(wdata->bytes >> 16);
> > +
> > + Â Â Â if (wct == 14) {
> > + Â Â Â Â Â Â Â inc_rfc1001_len(&smb->hdr, wdata->bytes + 1);
> > + Â Â Â Â Â Â Â put_bcc(wdata->bytes + 1, &smb->hdr);
> > + Â Â Â } else {
> > + Â Â Â Â Â Â Â /* wct == 12 */
> > + Â Â Â Â Â Â Â struct smb_com_writex_req *smbw =
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (struct smb_com_writex_req *)smb;
> > + Â Â Â Â Â Â Â inc_rfc1001_len(&smbw->hdr, wdata->bytes + 5);
> > + Â Â Â Â Â Â Â put_bcc(wdata->bytes + 5, &smbw->hdr);
> > + Â Â Â Â Â Â Â iov[0].iov_len += 4; /* pad bigger by four bytes */
> > + Â Â Â }
> > +
> > + Â Â Â kref_get(&wdata->refcount);
> > + Â Â Â rc = cifs_call_async(tcon->ses->server, iov, npages + 1,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Âcifs_writev_callback, wdata);
> > +
> > + Â Â Â if (rc == 0)
> > + Â Â Â Â Â Â Â cifs_stats_inc(&tcon->stats.cifs_stats.num_writes);
> > + Â Â Â else
> > + Â Â Â Â Â Â Â kref_put(&wdata->refcount, cifs_writedata_release);
> > +
> > + Â Â Â /* send is done, unmap pages */
> > + Â Â Â for (i = 0; i < npages; i++)
> > + Â Â Â Â Â Â Â kunmap(wdata->pages[i]);
> > +
> > +async_writev_out:
> > + Â Â Â cifs_small_buf_release(smb);
> > + Â Â Â kfree(iov);
> > + Â Â Â return rc;
> > +}
> > +
> > Âint
> > ÂCIFSSMBWrite2(const int xid, struct cifs_tcon *tcon,
> > Â Â Â Â Â Â const int netfid, const unsigned int count,
> > --
> > 1.7.4
> >
> > --
> > 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
> >
>
> Anyway, it's good. You can my "Reviewed-by: Pavel Shilovsky
> <piastry@xxxxxxxxxxx>" tag when you fix the notes above.
>
--
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
