LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Make BH_Unwritten a first class bufferhead flag
@ 2007-01-08 22:49 David Chinner
  2007-01-08 22:54 ` Christoph Hellwig
  0 siblings, 1 reply; 5+ messages in thread
From: David Chinner @ 2007-01-08 22:49 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: hch, linux-kernel


Currently, XFS uses BH_PrivateStart for flagging unwritten
extent state in a bufferhead. Recently, i found the long standing
mmap/unwritten extent conversion bug, and it was to do with
partial page invalidation not clearing the unwritten flag from
bufferheads attached to the page but beyond EOF. See here
for a full explaination:

http://oss.sgi.com/archives/xfs/2006-12/msg00196.html

The solution I have checked into the XFS dev tree involves
duplicating code from block_invalidatepage to clear the
unwritten flag from the bufferhead(s), and then calling
block_invalidatepage() to do the rest.

Christoph suggested that this would be better solved by
pushing the unwritten flag into the common buffer head flags
and just adding the call to discard_buffer():

http://oss.sgi.com/archives/xfs/2006-12/msg00239.html

The following patch makes BH_Unwritten a first class citizen.
Patch against 2.6.20-rc3.

Signed-Off-By: Dave Chinner <dgc@sgi.com>

---
 fs/buffer.c                  |    1 +
 fs/xfs/linux-2.6/xfs_linux.h |   10 ----------
 include/linux/buffer_head.h  |    2 ++
 3 files changed, 3 insertions(+), 10 deletions(-)

Index: linux/fs/buffer.c
===================================================================
--- linux.orig/fs/buffer.c	2007-01-08 17:19:49.039465038 +1100
+++ linux/fs/buffer.c	2007-01-08 17:20:43.547898480 +1100
@@ -1439,6 +1439,7 @@ static void discard_buffer(struct buffer
 	clear_buffer_req(bh);
 	clear_buffer_new(bh);
 	clear_buffer_delay(bh);
+	clear_buffer_unwritten(bh);
 	unlock_buffer(bh);
 }
 
Index: linux/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
--- linux.orig/fs/xfs/linux-2.6/xfs_linux.h	2007-01-08 17:19:32.703335135 +1100
+++ linux/fs/xfs/linux-2.6/xfs_linux.h	2007-01-08 17:19:36.271363508 +1100
@@ -109,16 +109,6 @@
 #undef  HAVE_PERCPU_SB	/* per cpu superblock counters are a 2.6 feature */
 #endif
 
-/*
- * State flag for unwritten extent buffers.
- *
- * We need to be able to distinguish between these and delayed
- * allocate buffers within XFS.  The generic IO path code does
- * not need to distinguish - we use the BH_Delay flag for both
- * delalloc and these ondisk-uninitialised buffers.
- */
-BUFFER_FNS(PrivateStart, unwritten);
-
 #define restricted_chown	xfs_params.restrict_chown.val
 #define irix_sgid_inherit	xfs_params.sgid_inherit.val
 #define irix_symlink_mode	xfs_params.symlink_mode.val
Index: linux/include/linux/buffer_head.h
===================================================================
--- linux.orig/include/linux/buffer_head.h	2007-01-08 17:17:15.118241081 +1100
+++ linux/include/linux/buffer_head.h	2007-01-08 17:18:25.714802453 +1100
@@ -34,6 +34,7 @@ enum bh_state_bits {
 	BH_Write_EIO,	/* I/O error on write */
 	BH_Ordered,	/* ordered write */
 	BH_Eopnotsupp,	/* operation not supported (barrier) */
+	BH_Unwritten,	/* Buffer is allocated on disk but not written */
 
 	BH_PrivateStart,/* not a state bit, but the first bit available
 			 * for private allocation by other entities
@@ -126,6 +127,7 @@ BUFFER_FNS(Boundary, boundary)
 BUFFER_FNS(Write_EIO, write_io_error)
 BUFFER_FNS(Ordered, ordered)
 BUFFER_FNS(Eopnotsupp, eopnotsupp)
+BUFFER_FNS(Unwritten, unwritten)
 
 #define bh_offset(bh)		((unsigned long)(bh)->b_data & ~PAGE_MASK)
 #define touch_buffer(bh)	mark_page_accessed(bh->b_page)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Make BH_Unwritten a first class bufferhead flag
  2007-01-08 22:49 [PATCH] Make BH_Unwritten a first class bufferhead flag David Chinner
@ 2007-01-08 22:54 ` Christoph Hellwig
  2007-01-08 23:14   ` Nathan Scott
  2007-01-08 23:57   ` David Chinner
  0 siblings, 2 replies; 5+ messages in thread
From: Christoph Hellwig @ 2007-01-08 22:54 UTC (permalink / raw)
  To: David Chinner; +Cc: linux-fsdevel, hch, linux-kernel

this doesn't look like a full first class flag to me yet.  Don't
we need to check for buffer_unwritten in the places we're checking
for buffer_delay so we can stop setting buffer_delay for unwritten
buffers?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Make BH_Unwritten a first class bufferhead flag
  2007-01-08 22:54 ` Christoph Hellwig
@ 2007-01-08 23:14   ` Nathan Scott
  2007-01-08 23:57   ` David Chinner
  1 sibling, 0 replies; 5+ messages in thread
From: Nathan Scott @ 2007-01-08 23:14 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: David Chinner, linux-fsdevel, linux-kernel

On Mon, 2007-01-08 at 22:54 +0000, Christoph Hellwig wrote:
> this doesn't look like a full first class flag to me yet.  Don't
> we need to check for buffer_unwritten in the places we're checking
> for buffer_delay so we can stop setting buffer_delay for unwritten
> buffers?

Yep, that does need to be done.  The first of the two calls
to set_buffer_delay can be removed from __xfs_get_blocks also
(currently there is an implied association between Delay and
Unwritten, which should be removed now).

I have a vague memory of some magic sysrq code (from 2.4 days)
which counted BH state on a page - if that still exists it'd
need to be updated too, but I can't seem to find it in current
2.6 kernels (used to live in buffer.c in ye olde 2.4 days).  It
probably left us around the time of PG_private's introduction.

cheers.

-- 
Nathan


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Make BH_Unwritten a first class bufferhead flag
  2007-01-08 22:54 ` Christoph Hellwig
  2007-01-08 23:14   ` Nathan Scott
@ 2007-01-08 23:57   ` David Chinner
  2007-01-09 10:50     ` Christoph Hellwig
  1 sibling, 1 reply; 5+ messages in thread
From: David Chinner @ 2007-01-08 23:57 UTC (permalink / raw)
  To: Christoph Hellwig, David Chinner, linux-fsdevel, linux-kernel

On Mon, Jan 08, 2007 at 10:54:02PM +0000, Christoph Hellwig wrote:
> this doesn't look like a full first class flag to me yet.  Don't
> we need to check for buffer_unwritten in the places we're checking
> for buffer_delay so we can stop setting buffer_delay for unwritten
> buffers?

That would be __block_prepare_write() and block_truncate_page()?
I can't see anywhere else in the code where buffer_delay is used
outside XFS....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Make BH_Unwritten a first class bufferhead flag
  2007-01-08 23:57   ` David Chinner
@ 2007-01-09 10:50     ` Christoph Hellwig
  0 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2007-01-09 10:50 UTC (permalink / raw)
  To: David Chinner; +Cc: Christoph Hellwig, linux-fsdevel, linux-kernel

On Tue, Jan 09, 2007 at 10:57:45AM +1100, David Chinner wrote:
> On Mon, Jan 08, 2007 at 10:54:02PM +0000, Christoph Hellwig wrote:
> > this doesn't look like a full first class flag to me yet.  Don't
> > we need to check for buffer_unwritten in the places we're checking
> > for buffer_delay so we can stop setting buffer_delay for unwritten
> > buffers?
> 
> That would be __block_prepare_write() and block_truncate_page()?
> I can't see anywhere else in the code where buffer_delay is used
> outside XFS....

Yes, I think it's just those two.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2007-01-09 10:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-08 22:49 [PATCH] Make BH_Unwritten a first class bufferhead flag David Chinner
2007-01-08 22:54 ` Christoph Hellwig
2007-01-08 23:14   ` Nathan Scott
2007-01-08 23:57   ` David Chinner
2007-01-09 10:50     ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).