Skip to contentSkip to conversation

remove ->prepare_write

RFC PATCH New Nick Piggin 2 participants 18 messages Nov 12, 2007 – Nov 15, 2007

Subsystems (10)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 remove ->prepare_write

Hi,

These are a set of patches to convert the last few filesystems to use the new deadlock-free write aops, and remove the core code to handle the legacy write path.

I don't really have setups to sufficiently test these filesystems. So I would really appreciate if filesystem maintainers can pick these patches up, bear with my bugs, and send them upstream when they're ready.

The benefit to you is that you get to use the fast and well tested code paths! Actually, it is interesting: several of the conversions I've done (including these) take a relatively naive aproach of simply prefilling the whole page if it isn't uptodate. It might be the case that some filesystems actually prefer to do something similar to the legacy double-copy path which they're being converted away from! (then again, it would be probably even more ideal to have simple sub-page state tracking structures).

There is still quite a lot of work left to be done. Several filesystems still use prepare_write() helpers, and when they're fixed up, all the old helpers themselves have to be removed. But this step is probably most important to getting rid of complex code.

#1 lore raw

RFC PATCH 1/5 ecryptfs new aops

Convert ecryptfs to new aops.

---

fs/ecryptfs/mmap.c +40 -28
Index: linux-2.6/fs/ecryptfs/mmap.c
===================================================================
@@ -263,31 +263,38 @@ out:
 	return 0;
 }
 
-static int ecryptfs_prepare_write(struct file *file, struct page *page,
-				  unsigned from, unsigned to)
+static int ecryptfs_write_begin(struct file *file,
+			struct address_space *mapping,
+			loff_t pos, unsigned len, unsigned flags,
+			struct page **pagep, void **fsdata)
 {
+	pgoff_t index = pos >> PAGE_CACHE_SHIFT;
+	struct page *page;
 	int rc = 0;
 
-	if (from == 0 && to == PAGE_CACHE_SIZE)
+	page = __grab_cache_page(mapping, index);
+	if (!page)
+		return -ENOMEM;
+
+	if (flags & AOP_FLAG_UNINTERRUPTIBLE && len == PAGE_CACHE_SIZE)
 		goto out;	/* If we are writing a full page, it will be
 				   up to date. */
 	if (!PageUptodate(page)) {
-		rc = ecryptfs_read_lower_page_segment(page, page->index, 0,
+		rc = ecryptfs_read_lower_page_segment(page, index, 0,
 						      PAGE_CACHE_SIZE,
-						      page->mapping->host);
+						      mapping->host);
 		if (rc) {
 			printk(KERN_ERR "%s: Error attemping to read lower "
 			       "page segment; rc = [%d]\n", __FUNCTION__, rc);
-			ClearPageUptodate(page);
 			goto out;
 		} else
 			SetPageUptodate(page);
 	}
-	if (page->index != 0) {
+	if (index != 0) {
 		loff_t end_of_prev_pg_pos =
-			(((loff_t)page->index << PAGE_CACHE_SHIFT) - 1);
+			(((loff_t)index << PAGE_CACHE_SHIFT) - 1);
 
-		if (end_of_prev_pg_pos > i_size_read(page->mapping->host)) {
+		if (end_of_prev_pg_pos > i_size_read(mapping->host)) {
 			rc = ecryptfs_truncate(file->f_path.dentry,
 					       end_of_prev_pg_pos);
 			if (rc) {
@@ -297,7 +304,7 @@ static int ecryptfs_prepare_write(struct
 				goto out;
 			}
 		}
-		if (end_of_prev_pg_pos + 1 > i_size_read(page->mapping->host))
+		if (end_of_prev_pg_pos + 1 > i_size_read(mapping->host))
 			zero_user_page(page, 0, PAGE_CACHE_SIZE, KM_USER0);
 	}
 out:
@@ -391,21 +398,25 @@ int ecryptfs_write_inode_size_to_metadat
 }
 
 /**
- * ecryptfs_commit_write
+ * ecryptfs_write_end
  * @file: The eCryptfs file object
+ * @mapping: The eCryptfs object
+ * @pos, len: Ignored (we rotate the page IV on each write)
  * @page: The eCryptfs page
- * @from: Ignored (we rotate the page IV on each write)
- * @to: Ignored
  *
  * This is where we encrypt the data and pass the encrypted data to
  * the lower filesystem.  In OpenPGP-compatible mode, we operate on
  * entire underlying packets.
  */
-static int ecryptfs_commit_write(struct file *file, struct page *page,
-				 unsigned from, unsigned to)
-{
-	loff_t pos;
-	struct inode *ecryptfs_inode = page->mapping->host;
+static int ecryptfs_write_end(struct file *file,
+			struct address_space *mapping,
+			loff_t pos, unsigned len, unsigned copied,
+			struct page *page, void *fsdata)
+{
+	pgoff_t index = pos >> PAGE_CACHE_SHIFT;
+	unsigned from = pos & (PAGE_CACHE_SIZE - 1);
+	unsigned to = from + copied;
+	struct inode *ecryptfs_inode = mapping->host;
 	struct ecryptfs_crypt_stat *crypt_stat =
 		&ecryptfs_inode_to_private(file->f_path.dentry->d_inode)->crypt_stat;
 	int rc;
@@ -417,25 +428,22 @@ static int ecryptfs_commit_write(struct 
 	} else
 		ecryptfs_printk(KERN_DEBUG, "Not a new file\n");
 	ecryptfs_printk(KERN_DEBUG, "Calling fill_zeros_to_end_of_page"
-			"(page w/ index = [0x%.16x], to = [%d])\n", page->index,
-			to);
+			"(page w/ index = [0x%.16x], to = [%d])\n", index, to);
 	/* Fills in zeros if 'to' goes beyond inode size */
 	rc = fill_zeros_to_end_of_page(page, to);
 	if (rc) {
 		ecryptfs_printk(KERN_WARNING, "Error attempting to fill "
-				"zeros in page with index = [0x%.16x]\n",
-				page->index);
+			"zeros in page with index = [0x%.16x]\n", index);
 		goto out;
 	}
 	rc = ecryptfs_encrypt_page(page);
 	if (rc) {
 		ecryptfs_printk(KERN_WARNING, "Error encrypting page (upper "
-				"index [0x%.16x])\n", page->index);
+				"index [0x%.16x])\n", index);
 		goto out;
 	}
-	pos = (((loff_t)page->index) << PAGE_CACHE_SHIFT) + to;
-	if (pos > i_size_read(ecryptfs_inode)) {
-		i_size_write(ecryptfs_inode, pos);
+	if (pos + copied > i_size_read(ecryptfs_inode)) {
+		i_size_write(ecryptfs_inode, pos + copied);
 		ecryptfs_printk(KERN_DEBUG, "Expanded file size to "
 				"[0x%.16x]\n", i_size_read(ecryptfs_inode));
 	}
@@ -443,7 +451,11 @@ static int ecryptfs_commit_write(struct 
 	if (rc)
 		printk(KERN_ERR "Error writing inode size to metadata; "
 		       "rc = [%d]\n", rc);
+
+	rc = copied;
 out:
+	unlock_page(page);
+	page_cache_release(page);
 	return rc;
 }
 
@@ -464,7 +476,7 @@ static sector_t ecryptfs_bmap(struct add
 struct address_space_operations ecryptfs_aops = {
 	.writepage = ecryptfs_writepage,
 	.readpage = ecryptfs_readpage,
-	.prepare_write = ecryptfs_prepare_write,
-	.commit_write = ecryptfs_commit_write,
+	.write_begin = ecryptfs_write_begin,
+	.write_end = ecryptfs_write_end,
 	.bmap = ecryptfs_bmap,
 };
#2 lore raw

RFC PATCH 2/5 cifs: new aops

In reply to Nick Piggin

Convert cifs to new aops.

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.

---

fs/cifs/file.c +59 -59
Index: linux-2.6/fs/cifs/file.c
===================================================================
@@ -103,7 +103,7 @@ static inline int cifs_open_inode_helper
 
 	/* 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);
@@ -1411,49 +1411,53 @@ static int cifs_writepage(struct page *p
 	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;
+	loff_t position;
+
+	cFYI(1, ("commit write for page %p from position %lld with %d bytes",
+		 page, pos, copied));
+
+	if (!PageUptodate(page) && copied == PAGE_CACHE_SIZE)
+		SetPageUptodate(page);
 
-	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);
-	}
-	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, &position);
+		/* if (rc < 0) should we set writebehind rc? */
 		kunmap(page);
+
+		FreeXid(xid);
 	} else {
+		rc = copied;
 		set_page_dirty(page);
 	}
 
-	FreeXid(xid);
+	if (rc > 0) {
+		position = pos + rc;
+		spin_lock(&inode->i_lock);
+		if (position > inode->i_size)
+			i_size_write(inode, position);
+		spin_unlock(&inode->i_lock);
+	}
+
+	unlock_page(page);
+	page_cache_release(page);
+
 	return rc;
 }
 
@@ -1999,49 +2003,45 @@ int is_size_safe_to_change(struct cifsIn
 		return 1;
 }
 
-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);
+	struct page *page;
+
+	cFYI(1, ("write begin from %lld len %d", (long long)pos, len));
+
+	page = __grab_cache_page(mapping, index);
+	if (!page)
+		return -ENOMEM;
 
-	cFYI(1, ("prepare write for page %p from %d to %d", page, from, to));
 	if (PageUptodate(page))
 		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);
+
+		/* 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;
 }
 
@@ -2050,8 +2050,8 @@ const struct address_space_operations ci
 	.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 = */
@@ -2066,8 +2066,8 @@ const struct address_space_operations ci
 	.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 = */
#3 lore raw

RFC PATCH 3/5 afs: new aops

In reply to Nick Piggin

Convert afs to new aops.

Cannot assume writes will fully complete, so this conversion goes the easy way and always brings the page uptodate before the write.

---

fs/afs/file.c +2 -2
Index: linux-2.6/fs/afs/file.c
===================================================================
@@ -50,8 +50,8 @@ const struct address_space_operations af
 	.launder_page	= afs_launder_page,
 	.releasepage	= afs_releasepage,
 	.invalidatepage	= afs_invalidatepage,
-	.prepare_write	= afs_prepare_write,
-	.commit_write	= afs_commit_write,
+	.write_begin	= afs_write_begin,
+	.write_end	= afs_write_end,
 	.writepage	= afs_writepage,
 	.writepages	= afs_writepages,
 };
fs/afs/internal.h +6 -2
Index: linux-2.6/fs/afs/internal.h
===================================================================
@@ -731,8 +731,12 @@ extern int afs_volume_release_fileserver
  */
 extern int afs_set_page_dirty(struct page *);
 extern void afs_put_writeback(struct afs_writeback *);
-extern int afs_prepare_write(struct file *, struct page *, unsigned, unsigned);
-extern int afs_commit_write(struct file *, struct page *, unsigned, unsigned);
+extern int afs_write_begin(struct file *file, struct address_space *mapping,
+			loff_t pos, unsigned len, unsigned flags,
+			struct page **pagep, void **fsdata);
+extern int afs_write_end(struct file *file, struct address_space *mapping,
+			loff_t pos, unsigned len, unsigned copied,
+			struct page *page, void *fsdata);
 extern int afs_writepage(struct page *, struct writeback_control *);
 extern int afs_writepages(struct address_space *, struct writeback_control *);
 extern int afs_write_inode(struct inode *, int);
fs/afs/write.c +43 -88

204 lines not on this page · raw message

maintainer
#4 lore raw

Re: [rfc][patch 3/5] afs: new aops

fs/afs/write.c void afs_put_writeback(struct afs_writeb · line 95

Quoted 2 lines · Nick Piggin
fs/afs/write.c +1 -1
-	ASSERTCMP(start + len, <=, PAGE_SIZE);
+	ASSERTCMP(len, <=, PAGE_CACHE_SIZE);

Do you guarantee this will work if PAGE_CACHE_SIZE != PAGE_SIZE? If not, you can't make this particular change.

Do we ever intend to have PAGE_CACHE_SIZE != PAGE_SIZE? If not, then surely the former is redundant and should scrapped to avoid confusion?

fs/afs/write.c void afs_put_writeback(struct afs_writeb · line 103

Quoted 7 lines
fs/afs/write.c +7 -0
+	i_size = i_size_read(&vnode->vfs_inode);
+	if (pos + len > i_size)
+		eof = i_size;
+	else
+		eof = PAGE_CACHE_SIZE;
+
+	ret = afs_vnode_fetch_data(vnode, key, 0, eof, page);

That can't be right, surely. Either 'eof' is the size of the file or it's the length of the data to be read. It can't be both. The first case needs eof masking off. Also, 'eof' isn't a good choice of name. 'len' would be better were it not already taken:-/

I notice you removed the stuff that clears holes in the page to be written. Is this is now done by the caller?

I notice also that you use afs_fill_page() in place of afs_prepare_page() to prepare a page. You can't do this if the region to be filled currently lies beyond the server's idea of EOF.

I'll try and get a look at fixing this patch tomorrow.

David

#5 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 7 lines · David Howells

fs/afs/write.c void afs_put_writeback(struct afs_writeb · line 95

Quoted 2 lines · Nick Piggin
fs/afs/write.c +1 -1
-	ASSERTCMP(start + len, <=, PAGE_SIZE);
+	ASSERTCMP(len, <=, PAGE_CACHE_SIZE);

Do you guarantee this will work if PAGE_CACHE_SIZE != PAGE_SIZE? If not, you can't make this particular change.

PAGE_CACHE_SIZE should be used to address the pagecache.

Quoted 2 lines

Do we ever intend to have PAGE_CACHE_SIZE != PAGE_SIZE? If not, then surely the former is redundant and should scrapped to avoid confusion?

Maybe.

Quoted 4 lines
Quoted 4 lines
fs/afs/write.c +4 -0
+	i_size = i_size_read(&vnode->vfs_inode);
+	if (pos + len > i_size)
+		eof = i_size;
+	else
Quoted 8 lines

fs/afs/write.c void afs_put_writeback(struct afs_writeb · line 103

Quoted 3 lines
fs/afs/write.c +3 -0
+		eof = PAGE_CACHE_SIZE;
+
+	ret = afs_vnode_fetch_data(vnode, key, 0, eof, page);

That can't be right, surely. Either 'eof' is the size of the file or it's the length of the data to be read. It can't be both. The first case needs eof masking off. Also, 'eof' isn't a good choice of name. 'len' would be better were it not already taken:-/

Yeah, just missed the mask.

Quoted 2 lines

I notice you removed the stuff that clears holes in the page to be written. Is this is now done by the caller?

It is supposed to bring the page uptodate first. So, no need to clear AFAIKS?

Quoted 5 lines

I notice also that you use afs_fill_page() in place of afs_prepare_page() to prepare a page. You can't do this if the region to be filled currently lies beyond the server's idea of EOF.

I'll try and get a look at fixing this patch tomorrow.

No rush, it won't get into 2.6.24 obviously. But that would be nice, thanks.

maintainer
#6 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 1 line · Nick Piggin

PAGE_CACHE_SIZE should be used to address the pagecache.

Perhaps, but the function being called from there takes pages not page cache slots. If I have to allow for PAGE_CACHE_SIZE > PAGE_SIZE then I need to modify my code, if not then the assertion needs to remain what it is.

Quoted 5 lines
Quoted 2 lines

I notice you removed the stuff that clears holes in the page to be written. Is this is now done by the caller?

It is supposed to bring the page uptodate first. So, no need to clear AFAIKS?

Hmmm... I suppose. However, it is wasteful in the common case as it is then bringing the page up to date by filling/clearing the whole of it and not just the bits that are not going to be written.

David

#7 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 7 lines · David Howells
Quoted 1 line · Nick Piggin

PAGE_CACHE_SIZE should be used to address the pagecache.

Perhaps, but the function being called from there takes pages not page cache slots. If I have to allow for PAGE_CACHE_SIZE > PAGE_SIZE then I need to modify my code, if not then the assertion needs to remain what it is.

It takes a pagecache page, yes. If you follow convention, you use PAGE_CACHE_SIZE for that guy. You don't have to allow PAGE_CACHE_SIZE != PAGE_SIZE, and if all the rest of your code is in units of PAGE_SIZE, then obviously my changing of just the one unit is even more confusing than the current arrangement ;)

Quoted 9 lines
Quoted 5 lines
Quoted 2 lines

I notice you removed the stuff that clears holes in the page to be written. Is this is now done by the caller?

It is supposed to bring the page uptodate first. So, no need to clear AFAIKS?

Hmmm... I suppose. However, it is wasteful in the common case as it is then bringing the page up to date by filling/clearing the whole of it and not just the bits that are not going to be written.

Yes, that's where you come in. You are free (and encouraged) to optimise this.

Let's see, for a network filesystem this is what you could do:
- if the page is not uptodate at write_begin time, do not bring it uptodate (at least, not the region that is going to be written to)
- if the page is not uptodate at write_end time, but the copy was fully completed, just mark it uptodate (provided you brought the regions outside the copy uptodate).
- if the page is not uptodate and you have a short copy, simply do not mark the page uptodate or dirty, and return 0 from write_end, indicating that you have committed 0 bytes. The generic code should DTRT.

Or you could:
Pass back a temporary (not pagecache) page in *pagep, and copy that yourself into the _real_ pagecache page at write_end time, so you know exactly how big the copy will be (this is basically what the 2copy method does now... it is probably not as good as the first method I described, but for a high latency filesystem it may be preferable to always bringing the page uptodate).

Or: keep track of sub-page dirty / uptodate status eg. with a light weight buffer_head like structure, so you can actually have partially dirty pages that are not completely uptodate.

Or... if you can think of another way...

maintainer
#8 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 5 lines · Nick Piggin

It takes a pagecache page, yes. If you follow convention, you use PAGE_CACHE_SIZE for that guy. You don't have to allow PAGE_CACHE_SIZE != PAGE_SIZE, and if all the rest of your code is in units of PAGE_SIZE, then obviously my changing of just the one unit is even more confusing than the current arrangement ;)

The problem is that the code called assumes that the struct page * argument points to a single page, not an array of pages as would presumably be the case if PAGE_CACHE_SIZE > PAGE_SIZE. If I should allow for an array of pages then the lower functions (specifically afs_deliver_fs_fetch_data()) need to change, and until that time occurs, the assertion *must* remain as it is now. It defends the lower functions against being asked to do something they weren't designed to do.

So: you may not change the assertion unless you also fix the lower functions.

David

#9 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 11 lines · David Howells
Quoted 5 lines · Nick Piggin

It takes a pagecache page, yes. If you follow convention, you use PAGE_CACHE_SIZE for that guy. You don't have to allow PAGE_CACHE_SIZE != PAGE_SIZE, and if all the rest of your code is in units of PAGE_SIZE, then obviously my changing of just the one unit is even more confusing than the current arrangement ;)

The problem is that the code called assumes that the struct page * argument points to a single page, not an array of pages as would presumably be the case if PAGE_CACHE_SIZE > PAGE_SIZE.

Incorrect. Christoph's patch for example does this by using compound pages. Now I personally don't like the patch or see the point in PAGE_CACHE_SIZE / PAGE_SIZE distinction, but I'm just telling you what the convention is. There is no point you arguing against it, that's simply how it is.

Quoted 7 lines

If I should allow for an array of pages then
the lower functions (specifically afs_deliver_fs_fetch_data()) need to change, and until that time occurs, the assertion *must* remain as it is now. It defends the lower functions against being asked to do something they weren't designed to do.

So: you may not change the assertion unless you also fix the lower functions.

I won't change the assertion, because you haven't been following said convention, so just changing it in one place is stupider than not changing it at all, but not for the reason cited.

maintainer
#10 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 8 lines · Nick Piggin
Quoted 3 lines

The problem is that the code called assumes that the struct page * argument points to a single page, not an array of pages as would presumably be the case if PAGE_CACHE_SIZE > PAGE_SIZE.

Incorrect. Christoph's patch for example does this by using compound pages. Now I personally don't like the patch or see the point in PAGE_CACHE_SIZE / PAGE_SIZE distinction, but I'm just telling you what the convention is. There is no point you arguing against it, that's simply how it is.

No! You are wrong. I wrote the AFS code. I *know* it can only deal with single pages. It has no knowledge of compound pages and does not handle page arrays. This may be a flaw in my code, but it's there nonetheless. The assertion is a guard against that. *That* was the point of my statement. Perhaps I should've said 'my code' rather than 'the code'.

If Christoph has a patch to deal with that, it's either not upstream yet or it hasn't altered AFS.

Quoted 6 lines
Quoted 2 lines

So: you may not change the assertion unless you also fix the lower functions.

I won't change the assertion, because you haven't been following said convention, so just changing it in one place is stupider than not changing it at all, but not for the reason cited.

The convention is not precisely clear. Just grep for PAGE_CACHE_SIZE in Documentation/. It's only mentioned twice, and in neither case does it give any information about what PAGE_CACHE_SIZE is, what it represents, or where it applies. Therefore it's an ill-defined concept.

If you look in Documentation/filesystems/vfs.txt, you'll see that it almost always talks about 'pages'. It only mentions 'pagecache pages' once - in the description of write_begin(), but it's not clear whether that means anything.

However, I've now noted that I need to fix my code, so just keep the assertion for now and I'll fix my code to handle multipage blocks.

David

#11 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 6 lines · David Howells
Quoted 4 lines · Nick Piggin
Quoted 3 lines

The problem is that the code called assumes that the struct page * argument points to a single page, not an array of pages as would presumably be the case if PAGE_CACHE_SIZE > PAGE_SIZE.

Quoted 6 lines
Quoted 4 lines

Incorrect. Christoph's patch for example does this by using compound pages. Now I personally don't like the patch or see the point in PAGE_CACHE_SIZE / PAGE_SIZE distinction, but I'm just telling you what the convention is. There is no point you arguing against it, that's simply how it is.

No! You are wrong. I wrote the AFS code. I *know* it can only deal with

No I'm talking about core code. In core code, the PAGE_CACHE_SIZE is for page cache struct pages. Single struct pages (not page arrays). Take a look at generic mapping read or something.

There is nothing to deal with page arrays there either, but that's simply the convention.

Quoted 7 lines
Quoted 6 lines
Quoted 2 lines

So: you may not change the assertion unless you also fix the lower functions.

I won't change the assertion, because you haven't been following said convention, so just changing it in one place is stupider than not changing it at all, but not for the reason cited.

Quoted 8 lines

The convention is not precisely clear. Just grep for PAGE_CACHE_SIZE in Documentation/. It's only mentioned twice, and in neither case does it give any information about what PAGE_CACHE_SIZE is, what it represents, or where it applies. Therefore it's an ill-defined concept.

If you look in Documentation/filesystems/vfs.txt, you'll see that it almost always talks about 'pages'. It only mentions 'pagecache pages' once - in the description of write_begin(), but it's not clear whether that means anything.

Documentation is the opposite of convention ;) Look in mm/.

Quoted 2 lines

However, I've now noted that I need to fix my code, so just keep the assertion for now and I'll fix my code to handle multipage blocks.

I'm not saying you need to do that. Leave it at PAGE_SIZE, really it doesn't matter that much at present. This has just blown out of proportion.

maintainer
#12 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 3 lines · Nick Piggin

In core code, the PAGE_CACHE_SIZE is for page cache struct pages. Single struct pages (not page arrays). Take a look at generic mapping read or something.

So you're saying a struct page controls an area of PAGE_CACHE_SIZE, not an area of PAGE_SIZE? Is a struct page then a purely pagecache concept?

Quoted 1 line

Documentation is the opposite of convention ;)

If it's not Documented, then it's irrelevant.

David

#13 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 8 lines · David Howells
Quoted 3 lines · Nick Piggin

In core code, the PAGE_CACHE_SIZE is for page cache struct pages. Single struct pages (not page arrays). Take a look at generic mapping read or something.

So you're saying a struct page controls an area of PAGE_CACHE_SIZE, not an area of PAGE_SIZE?

No, a pagecache page is PAGE_CACHE_SIZE. And not all struct pages control the same amount of data anyway, with compound pages.

Quoted 5 lines

Is a struct page then a purely pagecache concept?

Quoted 1 line

Documentation is the opposite of convention ;)

If it's not Documented, then it's irrelevant.

But you can't just decide yourself that it is irrelevant because you don't grep hard enough ;)

include/linux/mm.h:
* A page may belong to an inode's memory mapping. In this case, page->mapping
* is the pointer to the inode, and page->index is the file offset of the page,
* in units of PAGE_CACHE_SIZE.

include/linux/mm_types.h
unsigned long vm_pgoff; /* Offset (within vm_file) in PAGE_SIZE
units, *not* PAGE_CACHE_SIZE */

Looks like documentation to me.

maintainer
#14 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 4 lines · Nick Piggin
Quoted 2 lines

So you're saying a struct page controls an area of PAGE_CACHE_SIZE, not an area of PAGE_SIZE?

No, a pagecache page is PAGE_CACHE_SIZE.

That doesn't answer my question. I didn't ask about 'pagecache pages' per se.

Are you saying then that a page struct always represents an area of PAGE_SIZE to, say, the page allocator and PAGE_CACHE_SIZE to a filesystem's address operations?

How about I state it this way: Please define what the coverage of a (non-compound) struct page is, and how this relates to PAGE_SIZE and PAGE_CACHE_SIZE. If it's well-defined then this cannot be hard, right?

Quoted 2 lines

And not all struct pages control the same amount of data anyway, with compound pages.

Compound pages are irrelevant to my question. A compound page is actually a regulated by a series of page structs, each of which represents a 'page' of real memory.

Do you say, then, that all, say, readpage() and readpages() methods must handle a compound page if that is given to them?

David

#15 lore raw

Re: [rfc][patch 3/5] afs: new aops

Quoted 5 lines · David Howells
Quoted 3 lines · Nick Piggin
Quoted 2 lines

So you're saying a struct page controls an area of PAGE_CACHE_SIZE, not an area of PAGE_SIZE?

Quoted 7 lines
Quoted 1 line

No, a pagecache page is PAGE_CACHE_SIZE.

That doesn't answer my question. I didn't ask about 'pagecache pages' per se.

Are you saying then that a page struct always represents an area of PAGE_SIZE to, say, the page allocator and PAGE_CACHE_SIZE to a filesystem's address operations?

Yes.

Quoted 3 lines

How about I state it this way: Please define what the coverage of a (non-compound) struct page is, and how this relates to PAGE_SIZE and PAGE_CACHE_SIZE. If it's well-defined then this cannot be hard, right?

No it's easy. It's PAGE_SIZE (which also happens to be PAGE_CACHE_SIZE). An implementation that would (not trivially, but without changing the basic concepts) allow a larger pagecache size is with compound pages. And actually hugetlbfs already does this.

Quoted 6 lines
Quoted 2 lines

And not all struct pages control the same amount of data anyway, with compound pages.

Compound pages are irrelevant to my question. A compound page is actually a regulated by a series of page structs, each of which represents a 'page' of real memory.

And it can also represent a PAGE_CACHE_SIZE page of pagecache.

Quoted 2 lines

Do you say, then, that all, say, readpage() and readpages() methods must handle a compound page if that is given to them?

I'm not talking about a specific implementation that allows PAGE_CACHE_SIZE > PAGE_SIZE. So no, I don't say anything about that. I say that pagecache pages are PAGE_CACHE_SIZE! Yes it is easy now because that is the same as PAGE_SIZE. Yes it will be harder if you wanted to change that. What you would not have to change is the assumption that pagecache pages are in PAGE_SIZE units.

#16 lore raw

RFC PATCH 4/5 rd: rewrite rd

In reply to Nick Piggin

This is a rewrite of the ramdisk block device driver.

The old one is really difficult because it effectively implements a block device which serves data out of its own buffer cache. It relies on the dirty bit being set, to pin its backing store in cache, however there are non trivial paths which can clear the dirty bit (eg. try_to_free_buffers()), which had recently lead to data corruption. And in general it is completely wrong for a block device driver to do this.

The new one is more like a regular block device driver. It has no idea about vm/vfs stuff. It's backing store is similar to the buffer cache (a simple radix-tree of pages), but it doesn't know anything about page cache (the pages in the radix tree are not pagecache pages).

There is one slight downside -- direct block device access and filesystem metadata access goes through an extra copy and gets stored in RAM twice. However, this downside is only slight, because the real buffercache of the device is now reclaimable (because we're not playing crazy games with it), so under memory intensive situations, footprint should effectively be the same -- maybe even a slight advantage to the new driver because it can also reclaim buffer heads.

The fact that it now goes through all the regular vm/fs paths makes it much more useful for testing, too.

---

drivers/block/Kconfig +1 -11
Index: linux-2.6/drivers/block/Kconfig
===================================================================
@@ -311,7 +311,7 @@ config BLK_DEV_UB
 	  If unsure, say N.
 
 config BLK_DEV_RAM
-	tristate "RAM disk support"
+	tristate "RAM block device support"
 	---help---
 	  Saying Y here will allow you to use a portion of your RAM memory as
 	  a block device, so that you can make file systems on it, read and
@@ -346,16 +346,6 @@ config BLK_DEV_RAM_SIZE
 	  The default value is 4096 kilobytes. Only change this if you know
 	  what you are doing.
 
-config BLK_DEV_RAM_BLOCKSIZE
-	int "Default RAM disk block size (bytes)"
-	depends on BLK_DEV_RAM
-	default "1024"
-	help
-	  The default value is 1024 bytes.  PAGE_SIZE is a much more
-	  efficient choice however.  The default is kept to ensure initrd
-	  setups function - apparently needed by the rd_load_image routine
-	  that supposes the filesystem in the image uses a 1024 blocksize.
-
 config CDROM_PKTCDVD
 	tristate "Packet writing on CD/DVD media"
 	depends on !UML
drivers/block/Makefile +1 -1
Index: linux-2.6/drivers/block/Makefile
===================================================================
@@ -11,7 +11,7 @@ obj-$(CONFIG_AMIGA_FLOPPY)	+= amiflop.o
 obj-$(CONFIG_PS3_DISK)		+= ps3disk.o
 obj-$(CONFIG_ATARI_FLOPPY)	+= ataflop.o
 obj-$(CONFIG_AMIGA_Z2RAM)	+= z2ram.o
-obj-$(CONFIG_BLK_DEV_RAM)	+= rd.o
+obj-$(CONFIG_BLK_DEV_RD)	+= brd.o
 obj-$(CONFIG_BLK_DEV_LOOP)	+= loop.o
 obj-$(CONFIG_BLK_DEV_PS2)	+= ps2esdi.o
 obj-$(CONFIG_BLK_DEV_XD)	+= xd.o
drivers/block/brd.c new file +477 -0

477 lines not on this page · raw message

drivers/block/rd.c deleted +0 -523

523 lines not on this page · raw message

MAINTAINERS +5 -0
Index: linux-2.6/MAINTAINERS
===================================================================
@@ -3158,6 +3158,11 @@ W:	http://rt2x00.serialmonkey.com/
 S:	Maintained
 F:	drivers/net/wireless/rt2x00/
 
+RAMDISK RAM BLOCK DEVICE DRIVER
+P:	Nick Piggin
+M:	[email protected]
+S:	Maintained
+
 RANDOM NUMBER DRIVER
 P:	Matt Mackall
 M:	[email protected]
#17 lore raw

RFC PATCH 5/5 remove prepare_write

In reply to Nick Piggin
drivers/block/loop.c +2 -3
Index: linux-2.6/drivers/block/loop.c
===================================================================
@@ -40,8 +40,7 @@
  * Heinz Mauelshagen <[email protected]>, Feb 2002
  *
  * Support for falling back on the write file operation when the address space
- * operations prepare_write and/or commit_write are not available on the
- * backing filesystem.
+ * operations write_begin is not available on the backing filesystem.
  * Anton Altaparmakov, 16 Feb 2005
  *
  * Still To Fix:
@@ -761,7 +760,7 @@ static int loop_set_fd(struct loop_devic
 		 */
 		if (!file->f_op->splice_read)
 			goto out_putf;
-		if (aops->prepare_write || aops->write_begin)
+		if (aops->write_begin)
 			lo_flags |= LO_FLAGS_USE_AOPS;
 		if (!(lo_flags & LO_FLAGS_USE_AOPS) && !file->f_op->write)
 			lo_flags |= LO_FLAGS_READ_ONLY;
fs/fat/inode.c +1 -1
Index: linux-2.6/fs/fat/inode.c
===================================================================
@@ -175,7 +175,7 @@ static ssize_t fat_direct_IO(int rw, str
 
 	if (rw == WRITE) {
 		/*
-		 * FIXME: blockdev_direct_IO() doesn't use ->prepare_write(),
+		 * FIXME: blockdev_direct_IO() doesn't use ->write_begin(),
 		 * so we need to update the ->mmu_private to block boundary.
 		 *
 		 * But we must fill the remaining area or hole by nul for
fs/ocfs2/file.c +1 -2
Index: linux-2.6/fs/ocfs2/file.c
===================================================================
@@ -790,8 +790,7 @@ leave:
 
 /* Some parts of this taken from generic_cont_expand, which turned out
  * to be too fragile to do exactly what we need without us having to
- * worry about recursive locking in ->prepare_write() and
- * ->commit_write(). */
+ * worry about recursive locking in ->write_begin() and ->write_end(). */
 static int ocfs2_write_zero_page(struct inode *inode,
 				 u64 size)
 {
fs/splice.c +2 -2
Index: linux-2.6/fs/splice.c
===================================================================
@@ -734,8 +734,8 @@ ssize_t splice_from_pipe(struct pipe_ino
 	};
 
 	/*
-	 * The actor worker might be calling ->prepare_write and
-	 * ->commit_write. Most of the time, these expect i_mutex to
+	 * The actor worker might be calling ->write_begin and
+	 * ->write_end. Most of the time, these expect i_mutex to
 	 * be held. Since this may result in an ABBA deadlock with
 	 * pipe->inode, we have to order lock acquiry here.
 	 */
include/linux/fs.h +0 -7
Index: linux-2.6/include/linux/fs.h
===================================================================
@@ -453,13 +453,6 @@ struct address_space_operations {
 	int (*readpages)(struct file *filp, struct address_space *mapping,
 			struct list_head *pages, unsigned nr_pages);
 
-	/*
-	 * ext3 requires that a successful prepare_write() call be followed
-	 * by a commit_write() call - they must be balanced
-	 */
-	int (*prepare_write)(struct file *, struct page *, unsigned, unsigned);
-	int (*commit_write)(struct file *, struct page *, unsigned, unsigned);
-
 	int (*write_begin)(struct file *, struct address_space *mapping,
 				loff_t pos, unsigned len, unsigned flags,
 				struct page **pagep, void **fsdata);
mm/filemap.c +4 -235

264 lines not on this page · raw message

Documentation/filesystems/Locking +7 -5
Index: linux-2.6/Documentation/filesystems/Locking
===================================================================
@@ -166,8 +166,12 @@ prototypes:
 	int (*set_page_dirty)(struct page *page);
 	int (*readpages)(struct file *filp, struct address_space *mapping,
 			struct list_head *pages, unsigned nr_pages);
-	int (*prepare_write)(struct file *, struct page *, unsigned, unsigned);
-	int (*commit_write)(struct file *, struct page *, unsigned, unsigned);
+	int (*write_begin)(struct file *, struct address_space *mapping,
+				loff_t pos, unsigned len, unsigned flags,
+				struct page **pagep, void **fsdata);
+	int (*write_end)(struct file *, struct address_space *mapping,
+				loff_t pos, unsigned len, unsigned copied,
+				struct page *page, void *fsdata);
 	sector_t (*bmap)(struct address_space *, sector_t);
 	int (*invalidatepage) (struct page *, unsigned long);
 	int (*releasepage) (struct page *, int);
@@ -185,8 +189,6 @@ sync_page:		no	maybe
 writepages:		no
 set_page_dirty		no	no
 readpages:		no
-prepare_write:		no	yes			yes
-commit_write:		no	yes			yes
 write_begin:		no	locks the page		yes
 write_end:		no	yes, unlocks		yes
 perform_write:		no	n/a			yes
@@ -196,7 +198,7 @@ releasepage:		no	yes
 direct_IO:		no
 launder_page:		no	yes
 
-	->prepare_write(), ->commit_write(), ->sync_page() and ->readpage()
+	->write_begin(), ->write_end(), ->sync_page() and ->readpage()
 may be called from the request handler (/dev/loop).
 
 	->readpage() unlocks the page, either synchronously or via I/O
Documentation/filesystems/vfs.txt +5 -34
Index: linux-2.6/Documentation/filesystems/vfs.txt
===================================================================
@@ -506,7 +506,7 @@ written-back to storage typically in who
 address_space has finer control of write sizes.
 
 The read process essentially only requires 'readpage'.  The write
-process is more complicated and uses prepare_write/commit_write or
+process is more complicated and uses write_begin/write_end or
 set_page_dirty to write data into the address_space, and writepage,
 sync_page, and writepages to writeback data to storage.
 
@@ -535,8 +535,6 @@ struct address_space_operations {
 	int (*set_page_dirty)(struct page *page);
 	int (*readpages)(struct file *filp, struct address_space *mapping,
 			struct list_head *pages, unsigned nr_pages);
-	int (*prepare_write)(struct file *, struct page *, unsigned, unsigned);
-	int (*commit_write)(struct file *, struct page *, unsigned, unsigned);
 	int (*write_begin)(struct file *, struct address_space *mapping,
 				loff_t pos, unsigned len, unsigned flags,
 				struct page **pagep, void **fsdata);
@@ -612,37 +610,7 @@ struct address_space_operations {
 	readpages is only used for read-ahead, so read errors are
   	ignored.  If anything goes wrong, feel free to give up.
 
-  prepare_write: called by the generic write path in VM to set up a write
-  	request for a page.  This indicates to the address space that
-  	the given range of bytes is about to be written.  The
-  	address_space should check that the write will be able to
-  	complete, by allocating space if necessary and doing any other
-  	internal housekeeping.  If the write will update parts of
-  	any basic-blocks on storage, then those blocks should be
-  	pre-read (if they haven't been read already) so that the
-  	updated blocks can be written out properly.
-	The page will be locked.
-
-	Note: the page _must not_ be marked uptodate in this function
-	(or anywhere else) unless it actually is uptodate right now. As
-	soon as a page is marked uptodate, it is possible for a concurrent
-	read(2) to copy it to userspace.
-
-  commit_write: If prepare_write succeeds, new data will be copied
-        into the page and then commit_write will be called.  It will
-        typically update the size of the file (if appropriate) and
-        mark the inode as dirty, and do any other related housekeeping
-        operations.  It should avoid returning an error if possible -
-        errors should have been handled by prepare_write.
-
-  write_begin: This is intended as a replacement for prepare_write. The
-	key differences being that:
-		- it returns a locked page (in *pagep) rather than being
-		  given a pre locked page;
-		- it must be able to cope with short writes (where the
-		  length passed to write_begin is greater than the number
-		  of bytes copied into the page).
-
+  write_begin:
 	Called by the generic buffered write code to ask the filesystem to
 	prepare to write len bytes at the given offset in the file. The
 	address_space should check that the write will be able to complete,
@@ -654,6 +622,9 @@ struct address_space_operations {
         The filesystem must return the locked pagecache page for the specified
 	offset, in *pagep, for the caller to write into.
 
+	It must be able to cope with short writes (where the length passed to
+	write_begin is greater than the number of bytes copied into the page).
+
 	flags is a field for AOP_FLAG_xxx flags, described in
 	include/linux/fs.h.