Re: [patch] fs: restore nobh
On Fri 26-10-07 00:49:41, Nick Piggin wrote:
On Thu, Oct 25, 2007 at 09:07:36PM +0200, Jan Kara wrote:
Hi,
This is overdue, sorry. Got a little complicated, and I've been away from
my filesystem test setup so I didn't want ot send it (lucky, coz I found
a bug after more substantial testing).
Anyway, RFC?
Hmm, maybe one comment/question:
Index: linux-2.6/fs/buffer.c
===
--- linux-2.6.orig/fs/buffer.c2007-10-08 14:09:35.0 +1000
+++ linux-2.6/fs/buffer.c 2007-10-08 16:45:28.0 +1000
...
-/*
- * Make sure any changes to nobh_commit_write() are reflected in
- * nobh_truncate_page(), since it doesn't call commit_write().
- */
-int nobh_commit_write(struct file *file, struct page *page,
- unsigned from, unsigned to)
+int nobh_write_end(struct file *file, struct address_space *mapping,
+ loff_t pos, unsigned len, unsigned copied,
+ struct page *page, void *fsdata)
{
struct inode *inode = page-mapping-host;
- loff_t pos = ((loff_t)page-index PAGE_CACHE_SHIFT) + to;
+ struct buffer_head *head = NULL;
+ struct buffer_head *bh;
- if (page_has_buffers(page))
- return generic_commit_write(file, page, from, to);
+ if (!PageMappedToDisk(page)) {
+ if (unlikely(copied len) !page_has_buffers(page))
+ attach_nobh_buffers(page, head);
+ if (page_has_buffers(page))
+ return generic_write_end(file, mapping, pos, len,
copied, page, fsdata);
+ }
So we can have a page with attached buffers but we decide to not call
generic_write_end()...
SetPageUptodate(page);
set_page_dirty(page);
And we just set the page dirty but we leave buffers clean. So cannot
subsequent try_to_free_buffers() come, mark page as clean (as buffers
are clean) and discard the data?
Hmm, we might just be OK here because set_page_dirty should dirty the
buffers. However, I think you have spotted a mistake here and it would be
better if the generic_write_end test was outside the PageMapedToDisk
check.
But set_page_dirty() does not dirty buffers. That is only done by
set_page_dirty_buffers(). Or am I missing something?
Honza
--
Jan Kara [EMAIL PROTECTED]
SUSE Labs, CR
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] fs: restore nobh
On Thu, Oct 25, 2007 at 09:07:36PM +0200, Jan Kara wrote:
Hi,
This is overdue, sorry. Got a little complicated, and I've been away from
my filesystem test setup so I didn't want ot send it (lucky, coz I found
a bug after more substantial testing).
Anyway, RFC?
Hmm, maybe one comment/question:
Index: linux-2.6/fs/buffer.c
===
--- linux-2.6.orig/fs/buffer.c 2007-10-08 14:09:35.0 +1000
+++ linux-2.6/fs/buffer.c 2007-10-08 16:45:28.0 +1000
...
-/*
- * Make sure any changes to nobh_commit_write() are reflected in
- * nobh_truncate_page(), since it doesn't call commit_write().
- */
-int nobh_commit_write(struct file *file, struct page *page,
- unsigned from, unsigned to)
+int nobh_write_end(struct file *file, struct address_space *mapping,
+ loff_t pos, unsigned len, unsigned copied,
+ struct page *page, void *fsdata)
{
struct inode *inode = page-mapping-host;
- loff_t pos = ((loff_t)page-index PAGE_CACHE_SHIFT) + to;
+ struct buffer_head *head = NULL;
+ struct buffer_head *bh;
- if (page_has_buffers(page))
- return generic_commit_write(file, page, from, to);
+ if (!PageMappedToDisk(page)) {
+ if (unlikely(copied len) !page_has_buffers(page))
+ attach_nobh_buffers(page, head);
+ if (page_has_buffers(page))
+ return generic_write_end(file, mapping, pos, len,
copied, page, fsdata);
+ }
So we can have a page with attached buffers but we decide to not call
generic_write_end()...
SetPageUptodate(page);
set_page_dirty(page);
And we just set the page dirty but we leave buffers clean. So cannot
subsequent try_to_free_buffers() come, mark page as clean (as buffers
are clean) and discard the data?
Hmm, we might just be OK here because set_page_dirty should dirty the
buffers. However, I think you have spotted a mistake here and it would be
better if the generic_write_end test was outside the PageMapedToDisk
check.
Thanks!
-
To unsubscribe from this list: send the line unsubscribe linux-fsdevel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch] fs: restore nobh
Hi,
This is overdue, sorry. Got a little complicated, and I've been away from
my filesystem test setup so I didn't want ot send it (lucky, coz I found
a bug after more substantial testing).
Anyway, RFC?
---
Implement nobh in new aops. This is a bit tricky.
FWIW, nobh_truncate is now implemented in a way that does not
create blocks in sparse regions, which is a silly thing for it
to have been doing (isn't it?)
ext2 survives fsx and fsstress. jfs is converted as well... ext3
should be easy to do (but not done yet).
Index: linux-2.6/fs/buffer.c
===
--- linux-2.6.orig/fs/buffer.c 2007-10-08 14:09:35.0 +1000
+++ linux-2.6/fs/buffer.c 2007-10-08 16:45:28.0 +1000
@@ -2378,7 +2378,7 @@
}
/*
- * nobh_prepare_write()'s prereads are special: the buffer_heads are freed
+ * nobh_write_begin()'s prereads are special: the buffer_heads are freed
* immediately, while under the page lock. So it needs a special end_io
* handler which does not touch the bh after unlocking it.
*/
@@ -2388,16 +2388,45 @@
}
/*
+ * Attach the singly-linked list of buffers created by nobh_write_begin, to
+ * the page (converting it to circular linked list and taking care of page
+ * dirty races).
+ */
+static void attach_nobh_buffers(struct page *page, struct buffer_head *head)
+{
+ struct buffer_head *bh;
+
+ BUG_ON(!PageLocked(page));
+
+ spin_lock(page-mapping-private_lock);
+ bh = head;
+ do {
+ if (PageDirty(page))
+ set_buffer_dirty(bh);
+ if (!bh-b_this_page)
+ bh-b_this_page = head;
+ bh = bh-b_this_page;
+ } while (bh != head);
+ attach_page_buffers(page, head);
+ spin_unlock(page-mapping-private_lock);
+}
+
+/*
* On entry, the page is fully not uptodate.
* On exit the page is fully uptodate in the areas outside (from,to)
*/
-int nobh_prepare_write(struct page *page, unsigned from, unsigned to,
+int nobh_write_begin(struct file *file, struct address_space *mapping,
+ loff_t pos, unsigned len, unsigned flags,
+ struct page **pagep, void **fsdata,
get_block_t *get_block)
{
- struct inode *inode = page-mapping-host;
+ struct inode *inode = mapping-host;
const unsigned blkbits = inode-i_blkbits;
const unsigned blocksize = 1 blkbits;
struct buffer_head *head, *bh;
+ struct page *page;
+ pgoff_t index;
+ unsigned from, to;
unsigned block_in_page;
unsigned block_start, block_end;
sector_t block_in_file;
@@ -2406,8 +2435,22 @@
int ret = 0;
int is_mapped_to_disk = 1;
- if (page_has_buffers(page))
- return block_prepare_write(page, from, to, get_block);
+ index = pos PAGE_CACHE_SHIFT;
+ from = pos (PAGE_CACHE_SIZE - 1);
+ to = from + len;
+
+ page = __grab_cache_page(mapping, index);
+ if (!page)
+ return -ENOMEM;
+ *pagep = page;
+ *fsdata = NULL;
+
+ if (page_has_buffers(page)) {
+ unlock_page(page);
+ page_cache_release(page);
+ *pagep = NULL;
+ return block_write_begin(file, mapping, pos, len, flags, pagep,
fsdata, get_block);
+ }
if (PageMappedToDisk(page))
return 0;
@@ -2422,8 +2465,10 @@
* than the circular one we're used to.
*/
head = alloc_page_buffers(page, blocksize, 0);
- if (!head)
- return -ENOMEM;
+ if (!head) {
+ ret = -ENOMEM;
+ goto out_release;
+ }
block_in_file = (sector_t)page-index (PAGE_CACHE_SHIFT - blkbits);
@@ -2492,15 +2537,12 @@
if (is_mapped_to_disk)
SetPageMappedToDisk(page);
- do {
- bh = head;
- head = head-b_this_page;
- free_buffer_head(bh);
- } while (head);
+ *fsdata = head; /* to be released by nobh_write_end */
return 0;
failed:
+ BUG_ON(!ret);
/*
* Error recovery is a bit difficult. We need to zero out blocks that
* were newly allocated, and dirty them to ensure they get written out.
@@ -2508,64 +2550,56 @@
* the handling of potential IO errors during writeout would be hard
* (could try doing synchronous writeout, but what if that fails too?)
*/
- spin_lock(page-mapping-private_lock);
- bh = head;
- block_start = 0;
- do {
- if (PageUptodate(page))
- set_buffer_uptodate(bh);
- if (PageDirty(page))
- set_buffer_dirty(bh);
+ attach_nobh_buffers(page, head);
+ page_zero_new_buffers(page, from, to);
- block_end = block_start+blocksize;
- if (block_end
