by kernel test robot

[permalink] [raw]

Subject: Re: [Patch v2 13/15] CIFS: Add support for direct I/O read

Attachments:

(No filename) (4.60 kB)
.config.gz (24.65 kB)
Download all attachments

2018-06-02 07:16:17

> Subject: Re: [Patch v2 06/15] CIFS: Introduce helper function to get page
> offset and length in smb_rqst
>
> On 6/25/2018 5:14 PM, Long Li wrote:
> >> Subject: Re: [Patch v2 06/15] CIFS: Introduce helper function to get
> >> page offset and length in smb_rqst
> >>
> >> On 5/30/2018 3:47 PM, Long Li wrote:
> >>> From: Long Li <[email protected]>
> >>>
> >>> Introduce a function rqst_page_get_length to return the page offset
> >>> and length for a given page in smb_rqst. This function is to be used
> >>> by following patches.
> >>>
> >>> Signed-off-by: Long Li <[email protected]>
> >>> ---
> >>> fs/cifs/cifsproto.h | 3 +++
> >>> fs/cifs/misc.c | 17 +++++++++++++++++
> >>> 2 files changed, 20 insertions(+)
> >>>
> >>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index
> >>> 7933c5f..89dda14 100644
> >>> --- a/fs/cifs/cifsproto.h
> >>> +++ b/fs/cifs/cifsproto.h
> >>> @@ -557,4 +557,7 @@ int cifs_alloc_hash(const char *name, struct
> >> crypto_shash **shash,
> >>> struct sdesc **sdesc);
> >>> void cifs_free_hash(struct crypto_shash **shash, struct sdesc
> >>> **sdesc);
> >>>
> >>> +extern void rqst_page_get_length(struct smb_rqst *rqst, unsigned
> >>> +int
> >> page,
> >>> + unsigned int *len, unsigned int *offset);
> >>> +
> >>> #endif /* _CIFSPROTO_H */
> >>> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c index 96849b5..e951417
> >>> 100644
> >>> --- a/fs/cifs/misc.c
> >>> +++ b/fs/cifs/misc.c
> >>> @@ -905,3 +905,20 @@ cifs_free_hash(struct crypto_shash **shash,
> >> struct sdesc **sdesc)
> >>> crypto_free_shash(*shash);
> >>> *shash = NULL;
> >>> }
> >>> +
> >>> +/**
> >>> + * rqst_page_get_length - obtain the length and offset for a page
> >>> +in smb_rqst
> >>> + * Input: rqst - a smb_rqst, page - a page index for rqst
> >>> + * Output: *len - the length for this page, *offset - the offset
> >>> +for this page */ void rqst_page_get_length(struct smb_rqst *rqst,
> >>> +unsigned int page,
> >>> + unsigned int *len, unsigned int *offset) {
> >>> + *len = rqst->rq_pagesz;
> >>> + *offset = (page == 0) ? rqst->rq_offset : 0;
> >>
> >> Really? Page 0 always has a zero offset??
> >
> > I think you are misreading this line. The offset for page 0 is rq_offset, the
> offset for all subsequent pages are 0.
>
> Ah, yes, sorry I did read it incorrectly.
> >>> +
> >>> + if (rqst->rq_npages == 1 || page == rqst->rq_npages-1)
> >>> + *len = rqst->rq_tailsz;
> >>> + else if (page == 0)
> >>> + *len = rqst->rq_pagesz - rqst->rq_offset; }
> >>>
> >>
> >> This subroutine does what patch 5 does inline. Why not push this
> >> patch up in the sequence and use the helper?
> >
> > This is actually a little different. This function returns the length and offset
> for a given page in the request. There might be multiple pages in a request.
>
> Ok, but I still think there is quite a bit of inline computation of this stuff that
> would more clearly and more robustly be done in a set of common functions.
> If someone ever touches the code to support a new upper layer, or even
> integrate with more complex compounding, things will get ugly fast.
>
> > The other function calculates the total length of all the pages in a request.
>
> Again, a great candidate for a common set of subroutines, IMO.

I will look into this. The reason I didn't make a common function is that this is the only patch that will use it in this way.

2018-06-27 04:58:57

by Long Li

[permalink] [raw]

Subject: RE: [Patch v2 14/15] CIFS: Add support for direct I/O write

> Subject: Re: [Patch v2 14/15] CIFS: Add support for direct I/O write
>
> On 6/26/2018 12:39 AM, Long Li wrote:
> >> Subject: Re: [Patch v2 14/15] CIFS: Add support for direct I/O write
> >>
> >> On 5/30/2018 3:48 PM, Long Li wrote:
> >>> From: Long Li <[email protected]>
> >>>
> >>> Implement the function for direct I/O write. It doesn't support AIO,
> >>> which will be implemented in a follow up patch.
> >>>
> >>> Signed-off-by: Long Li <[email protected]>
> >>> ---
> >>> fs/cifs/cifsfs.h | 1 +
> >>> fs/cifs/file.c | 165
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> 2 files changed, 166 insertions(+)
> >>>
> >>> diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h index
> >>> 7fba9aa..e9c5103 100644
> >>> --- a/fs/cifs/cifsfs.h
> >>> +++ b/fs/cifs/cifsfs.h
> >>> @@ -105,6 +105,7 @@ extern ssize_t cifs_user_readv(struct kiocb
> >>> *iocb,
> >> struct iov_iter *to);
> >>> extern ssize_t cifs_direct_readv(struct kiocb *iocb, struct iov_iter *to);
> >>> extern ssize_t cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to);
> >>> extern ssize_t cifs_user_writev(struct kiocb *iocb, struct
> >>> iov_iter *from);
> >>> +extern ssize_t cifs_direct_writev(struct kiocb *iocb, struct
> >>> +iov_iter *from);
> >>> extern ssize_t cifs_strict_writev(struct kiocb *iocb, struct iov_iter
> *from);
> >>> extern int cifs_lock(struct file *, int, struct file_lock *);
> >>> extern int cifs_fsync(struct file *, loff_t, loff_t, int); diff
> >>> --git a/fs/cifs/file.c b/fs/cifs/file.c index e6e6f24..8c385b1
> >>> 100644
> >>> --- a/fs/cifs/file.c
> >>> +++ b/fs/cifs/file.c
> >>> @@ -2461,6 +2461,35 @@ cifs_uncached_writedata_release(struct kref
> >>> *refcount)
> >>>
> >>> static void collect_uncached_write_data(struct cifs_aio_ctx
> >>> *ctx);
> >>>
> >>> +static void cifs_direct_writedata_release(struct kref *refcount) {
> >>> + int i;
> >>> + struct cifs_writedata *wdata = container_of(refcount,
> >>> + struct cifs_writedata, refcount);
> >>> +
> >>> + for (i = 0; i < wdata->nr_pages; i++)
> >>> + put_page(wdata->pages[i]);
> >>> +
> >>> + cifs_writedata_release(refcount);
> >>> +}
> >>> +
> >>> +static void cifs_direct_writev_complete(struct work_struct *work) {
> >>> + struct cifs_writedata *wdata = container_of(work,
> >>> + struct cifs_writedata, work);
> >>> + struct inode *inode = d_inode(wdata->cfile->dentry);
> >>> + struct cifsInodeInfo *cifsi = CIFS_I(inode);
> >>> +
> >>> + spin_lock(&inode->i_lock);
> >>> + cifs_update_eof(cifsi, wdata->offset, wdata->bytes);
> >>> + if (cifsi->server_eof > inode->i_size)
> >>> + i_size_write(inode, cifsi->server_eof);
> >>> + spin_unlock(&inode->i_lock);
> >>> +
> >>> + complete(&wdata->done);
> >>> + kref_put(&wdata->refcount, cifs_direct_writedata_release); }
> >>> +
> >>> static void
> >>> cifs_uncached_writev_complete(struct work_struct *work)
> >>> {
> >>> @@ -2703,6 +2732,142 @@ static void
> >>> collect_uncached_write_data(struct
> >> cifs_aio_ctx *ctx)
> >>> complete(&ctx->done);
> >>> }
> >>>
> >>> +ssize_t cifs_direct_writev(struct kiocb *iocb, struct iov_iter
> >>> +*from) {
> >>> + struct file *file = iocb->ki_filp;
> >>> + ssize_t total_written = 0;
> >>> + struct cifsFileInfo *cfile;
> >>> + struct cifs_tcon *tcon;
> >>> + struct cifs_sb_info *cifs_sb;
> >>> + struct TCP_Server_Info *server;
> >>> + pid_t pid;
> >>> + unsigned long nr_pages;
> >>> + loff_t offset = iocb->ki_pos;
> >>> + size_t len = iov_iter_count(from);
> >>> + int rc;
> >>> + struct cifs_writedata *wdata;
> >>> +
> >>> + /*
> >>> + * iov_iter_get_pages_alloc doesn't work with ITER_KVEC.
> >>> + * In this case, fall back to non-direct write function.
> >>> + */
> >>> + if (from->type & ITER_KVEC) {
> >>> + cifs_dbg(FYI, "use non-direct cifs_user_writev for kvec
> >> I/O\n");
> >>> + return cifs_user_writev(iocb, from);
> >>> + }
> >>> +
> >>> + rc = generic_write_checks(iocb, from);
> >>> + if (rc <= 0)
> >>> + return rc;
> >>> +
> >>> + cifs_sb = CIFS_FILE_SB(file);
> >>> + cfile = file->private_data;
> >>> + tcon = tlink_tcon(cfile->tlink);
> >>> + server = tcon->ses->server;
> >>> +
> >>> + if (!server->ops->async_writev)
> >>> + return -ENOSYS;
> >>> +
> >>> + if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_RWPIDFORWARD)
> >>> + pid = cfile->pid;
> >>> + else
> >>> + pid = current->tgid;
> >>> +
> >>> + do {
> >>> + unsigned int wsize, credits;
> >>> + struct page **pagevec;
> >>> + size_t start;
> >>> + ssize_t cur_len;
> >>> +
> >>> + rc = server->ops->wait_mtu_credits(server, cifs_sb->wsize,
> >>> + &wsize, &credits);
> >>> + if (rc)
> >>> + break;
> >>> +
> >>> + cur_len = iov_iter_get_pages_alloc(
> >>> + from, &pagevec, wsize, &start);
> >>> + if (cur_len < 0) {
> >>> + cifs_dbg(VFS,
> >>> + "direct_writev couldn't get user pages "
> >>> + "(rc=%zd) iter type %d iov_offset %lu count"
> >>> + " %lu\n",
> >>> + cur_len, from->type,
> >>> + from->iov_offset, from->count);
> >>> + dump_stack();
> >>> + break;
> >>> + }
> >>> + if (cur_len < 0)
> >>> + break;
> >>
> >> This cur_len < 0 test is redundant with the prior if(), delete.
> >>> +
> >>> + nr_pages = (cur_len + start + PAGE_SIZE - 1) / PAGE_SIZE;
> >>
> >> Am I misreading, or will this return be one more page than needed? If
> >> start (the first byte offset) is > 0, nr_pages will already be one.
> >> And if cur_len is 4KB, even if start is 0, nr_pages will be two.
> >
> > I think the calculation is correct, assuming cur_len > 0. (which
> > should be the case if we reach here)
>
> Err, cur_len could possibly be zero, the prior line only breaks the loop if
> cur_len < 0.

I will look into this. It's a good idea to return error on 0 since we have no way to proceed.

>
> > If cur_len is 4kb and start is 0, nr_pages will be 1.
>
> Hmm, I guess. But again, it seems as if this page handling is all being coded
> inline, in subtly different ways. I'm just concerned about clarity and
> robustness. To me, if it's hard to review, it's even harder to maintain.

I will address this in an updated patch.

>
> Tom.
>
> >
> >>
> >>> +
> >>> + wdata = cifs_writedata_direct_alloc(pagevec,
> >>> + cifs_direct_writev_complete);
> >>> + if (!wdata) {
> >>> + rc = -ENOMEM;
> >>> + add_credits_and_wake_if(server, credits, 0);
> >>> + break;
> >>> + }
> >>> +
> >>> + wdata->nr_pages = nr_pages;
> >>> + wdata->page_offset = start;
> >>> + wdata->pagesz = PAGE_SIZE;
> >>> + wdata->tailsz =
> >>> + nr_pages > 1 ?
> >>> + cur_len - (PAGE_SIZE - start) -
> >>> + (nr_pages - 2) * PAGE_SIZE :
> >>> + cur_len;
> >>> +
> >>> + wdata->sync_mode = WB_SYNC_ALL;
> >>> + wdata->offset = (__u64)offset;
> >>> + wdata->cfile = cifsFileInfo_get(cfile);
> >>> + wdata->pid = pid;
> >>> + wdata->bytes = cur_len;
> >>> + wdata->credits = credits;
> >>> +
> >>> + rc = 0;
> >>> + if (wdata->cfile->invalidHandle)
> >>> + rc = cifs_reopen_file(wdata->cfile, false);
> >>> +
> >>> + if (!rc)
> >>> + rc = server->ops->async_writev(wdata,
> >>> + cifs_direct_writedata_release);
> >>> +
> >>> + if (rc) {
> >>> + add_credits_and_wake_if(server, wdata->credits, 0);
> >>> + kref_put(&wdata->refcount,
> >>> + cifs_direct_writedata_release);
> >>> + if (rc == -EAGAIN)
> >>> + continue;
> >>> + break;
> >>> + }
> >>
> >> Same comments as for previous patch re the if (rc) ladder, and the
> >> break/continues both being better expressed as careful goto's.
> >>
> >>> +
> >>> + wait_for_completion(&wdata->done);
> >>> + if (wdata->result) {
> >>> + rc = wdata->result;
> >>> + kref_put(&wdata->refcount,
> >>> + cifs_direct_writedata_release);
> >>> + if (rc == -EAGAIN)
> >>> + continue;
> >>> + break;
> >>> + }
> >>> +
> >>> + kref_put(&wdata->refcount, cifs_direct_writedata_release);
> >>> +
> >>> + iov_iter_advance(from, cur_len);
> >>> + total_written += cur_len;
> >>> + offset += cur_len;
> >>> + len -= cur_len;
> >>> + } while (len);
> >>> +
> >>> + if (unlikely(!total_written))
> >>> + return rc;
> >>> +
> >>> + iocb->ki_pos += total_written;
> >>> + return total_written;
> >>> +
> >>> +}
> >>> +
> >>> ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from)
> >>> {
> >>> struct file *file = iocb->ki_filp;
> >>>