[PATCH] cifs: Convert cifs to new aops.

[PATCH] cifs: Convert cifs to new aops.

[Jeff Layton]

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@redhat.com>
Reviewed-by: Badari Pulavarty <pbadari@us.ibm.com>
Acked-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
Cc: Steve French <smfrench@gmail.com>
Cc: Nick Piggin <npiggin@suse.de>
---
 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

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

[Christoph Hellwig]

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@redhat.com>
> Reviewed-by: Badari Pulavarty <pbadari@us.ibm.com>
> Acked-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> Cc: Steve French <smfrench@gmail.com>
> Cc: Nick Piggin <npiggin@suse.de>
> ---
>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
---end quoted text---

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@infradead.org> 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@redhat.com>
>> Reviewed-by: Badari Pulavarty <pbadari@us.ibm.com>
>> Acked-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: Nick Piggin <npiggin@suse.de>
>> ---
>>  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@vger.kernel.org
>> 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

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

[Badari Pulavarty]

On Wed, 2008-09-24 at 13:54 -0400, Christoph Hellwig wrote:
> Might be worth Ccing Nick as he's started on finishing the last aops
> conversions again at Kernel Summit.

Yes. Its me, who started this again (after talking to Nick at KS) -
trying to make use ecryptfs and cifs gets converted to new aops. Nick
has been on the CCed on all our (private) mails.

BTW, I posted ecryptfs conversion also. Only thing remaining is
afs. Once thats done, we can get rid of prepare/commit write.

Thanks,
Badari

> 
> 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@redhat.com>
> > Reviewed-by: Badari Pulavarty <pbadari@us.ibm.com>
> > Acked-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
> > Cc: Steve French <smfrench@gmail.com>
> > Cc: Nick Piggin <npiggin@suse.de>
> > ---
> >  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@vger.kernel.org
> > 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-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/