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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux