Might be worth Ccing Nick as he's started on finishing the last aops
conversions again at Kernel Summit.
On Wed, Sep 24, 2008 at 01:39:13PM -0400, Jeff Layton wrote:
> This patch is based on the one originally posted by Nick Piggin. His
> patch was very close, but had a couple of small bugs. Nick's original
> comments follow:
>
> ---------------[snip]--------------
>
> This is another relatively naive conversion. Always do the read upfront
> when the page is not uptodate (unless we're in the writethrough path).
>
> Fix an uninitialized data exposure where SetPageUptodate was called
> before the page was uptodate.
>
> SetPageUptodate and switch to writeback mode in the case that the full
> page was dirtied.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Reviewed-by: Badari Pulavarty <pbadari@xxxxxxxxxx>
> Acked-by: Dave Kleikamp <shaggy@xxxxxxxxxxxxxxxxxx>
> Cc: Steve French <smfrench@xxxxxxxxx>
> Cc: Nick Piggin <npiggin@xxxxxxx>
> ---
> fs/cifs/file.c | 120 +++++++++++++++++++++++++++----------------------------
> 1 files changed, 59 insertions(+), 61 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index d39e852..c4a8a06 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -107,7 +107,7 @@ static inline int cifs_open_inode_helper(struct inode *inode, struct file *file,
>
> /* want handles we can use to read with first
> in the list so we do not have to walk the
> - list to search for one in prepare_write */
> + list to search for one in write_begin */
> if ((file->f_flags & O_ACCMODE) == O_WRONLY) {
> list_add_tail(&pCifsFile->flist,
> &pCifsInode->openFileList);
> @@ -915,7 +915,7 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
> }
>
> static ssize_t cifs_write(struct file *file, const char *write_data,
> - size_t write_size, loff_t *poffset)
> + size_t write_size, loff_t *poffset)
> {
> int rc = 0;
> unsigned int bytes_written = 0;
> @@ -1455,49 +1455,52 @@ static int cifs_writepage(struct page *page, struct writeback_control *wbc)
> return rc;
> }
>
> -static int cifs_commit_write(struct file *file, struct page *page,
> - unsigned offset, unsigned to)
> +static int cifs_write_end(struct file *file, struct address_space *mapping,
> + loff_t pos, unsigned len, unsigned copied,
> + struct page *page, void *fsdata)
> {
> - int xid;
> - int rc = 0;
> - struct inode *inode = page->mapping->host;
> - loff_t position = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to;
> - char *page_data;
> + int rc;
> + struct inode *inode = mapping->host;
>
> - xid = GetXid();
> - cFYI(1, ("commit write for page %p up to position %lld for %d",
> - page, position, to));
> - spin_lock(&inode->i_lock);
> - if (position > inode->i_size)
> - i_size_write(inode, position);
> + cFYI(1, ("write_end for page %p from pos %lld with %d bytes",
> + page, pos, copied));
> +
> + if (!PageUptodate(page) && copied == PAGE_CACHE_SIZE)
> + SetPageUptodate(page);
>
> - spin_unlock(&inode->i_lock);
> if (!PageUptodate(page)) {
> - position = ((loff_t)page->index << PAGE_CACHE_SHIFT) + offset;
> - /* can not rely on (or let) writepage write this data */
> - if (to < offset) {
> - cFYI(1, ("Illegal offsets, can not copy from %d to %d",
> - offset, to));
> - FreeXid(xid);
> - return rc;
> - }
> + char *page_data;
> + unsigned offset = pos & (PAGE_CACHE_SIZE - 1);
> + int xid;
> +
> + xid = GetXid();
> /* this is probably better than directly calling
> partialpage_write since in this function the file handle is
> known which we might as well leverage */
> /* BB check if anything else missing out of ppw
> such as updating last write time */
> page_data = kmap(page);
> - rc = cifs_write(file, page_data + offset, to-offset,
> - &position);
> - if (rc > 0)
> - rc = 0;
> - /* else if (rc < 0) should we set writebehind rc? */
> + rc = cifs_write(file, page_data + offset, copied, &pos);
> + /* if (rc < 0) should we set writebehind rc? */
> kunmap(page);
> +
> + FreeXid(xid);
> } else {
> + rc = copied;
> + pos += copied;
> set_page_dirty(page);
> }
>
> - FreeXid(xid);
> + if (rc > 0) {
> + spin_lock(&inode->i_lock);
> + if (pos > inode->i_size)
> + i_size_write(inode, pos);
> + spin_unlock(&inode->i_lock);
> + }
> +
> + unlock_page(page);
> + page_cache_release(page);
> +
> return rc;
> }
>
> @@ -2043,49 +2046,44 @@ bool is_size_safe_to_change(struct cifsInodeInfo *cifsInode, __u64 end_of_file)
> return true;
> }
>
> -static int cifs_prepare_write(struct file *file, struct page *page,
> - unsigned from, unsigned to)
> +static int cifs_write_begin(struct file *file, struct address_space *mapping,
> + loff_t pos, unsigned len, unsigned flags,
> + struct page **pagep, void **fsdata)
> {
> - int rc = 0;
> - loff_t i_size;
> - loff_t offset;
> + pgoff_t index = pos >> PAGE_CACHE_SHIFT;
> + loff_t offset = pos & (PAGE_CACHE_SIZE - 1);
> +
> + cFYI(1, ("write_begin from %lld len %d", (long long)pos, len));
> +
> + *pagep = __grab_cache_page(mapping, index);
> + if (!*pagep)
> + return -ENOMEM;
>
> - cFYI(1, ("prepare write for page %p from %d to %d", page, from, to));
> - if (PageUptodate(page))
> + if (PageUptodate(*pagep))
> return 0;
>
> /* If we are writing a full page it will be up to date,
> no need to read from the server */
> - if ((to == PAGE_CACHE_SIZE) && (from == 0)) {
> - SetPageUptodate(page);
> + if (len == PAGE_CACHE_SIZE && flags & AOP_FLAG_UNINTERRUPTIBLE)
> return 0;
> - }
>
> - offset = (loff_t)page->index << PAGE_CACHE_SHIFT;
> - i_size = i_size_read(page->mapping->host);
> + if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
> + int rc;
>
> - if ((offset >= i_size) ||
> - ((from == 0) && (offset + to) >= i_size)) {
> - /*
> - * We don't need to read data beyond the end of the file.
> - * zero it, and set the page uptodate
> - */
> - simple_prepare_write(file, page, from, to);
> - SetPageUptodate(page);
> - } else if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
> /* might as well read a page, it is fast enough */
> - rc = cifs_readpage_worker(file, page, &offset);
> + rc = cifs_readpage_worker(file, *pagep, &offset);
> +
> + /* we do not need to pass errors back
> + e.g. if we do not have read access to the file
> + because cifs_write_end will attempt synchronous writes
> + -- shaggy */
> } else {
> /* we could try using another file handle if there is one -
> but how would we lock it to prevent close of that handle
> racing with this read? In any case
> - this will be written out by commit_write so is fine */
> + this will be written out by write_end so is fine */
> }
>
> - /* we do not need to pass errors back
> - e.g. if we do not have read access to the file
> - because cifs_commit_write will do the right thing. -- shaggy */
> -
> return 0;
> }
>
> @@ -2094,8 +2092,8 @@ const struct address_space_operations cifs_addr_ops = {
> .readpages = cifs_readpages,
> .writepage = cifs_writepage,
> .writepages = cifs_writepages,
> - .prepare_write = cifs_prepare_write,
> - .commit_write = cifs_commit_write,
> + .write_begin = cifs_write_begin,
> + .write_end = cifs_write_end,
> .set_page_dirty = __set_page_dirty_nobuffers,
> /* .sync_page = cifs_sync_page, */
> /* .direct_IO = */
> @@ -2110,8 +2108,8 @@ const struct address_space_operations cifs_addr_ops_smallbuf = {
> .readpage = cifs_readpage,
> .writepage = cifs_writepage,
> .writepages = cifs_writepages,
> - .prepare_write = cifs_prepare_write,
> - .commit_write = cifs_commit_write,
> + .write_begin = cifs_write_begin,
> + .write_end = cifs_write_end,
> .set_page_dirty = __set_page_dirty_nobuffers,
> /* .sync_page = cifs_sync_page, */
> /* .direct_IO = */
> --
> 1.5.5.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
---end quoted text---
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
