Re: [PATCH] cifs: Convert cifs to new aops.

[Steve French]

I have reviewed the patch and merged into cifs-2.6.git tree.

Will do some additional testing (with fsx and other test tools) while
at SDC plugfest this week.

On Wed, Sep 24, 2008 at 12:54 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> 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---
>



-- 
Thanks,

Steve
--
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/