Review: ensure EOF writes into existing extents update filesize

Review: ensure EOF writes into existing extents update filesize

[David Chinner]

As reported a coupl eof times on lkml since the 2.6.22 window opened,
XFS is not updating the file size corectly in all cases. This is a
result of the null files fix not updating the file size when the
write extending the file does not need to allocate a block.

In that case, we use a read mapping of the extent, and this also
happens to use the read I/O completion handler instead of the
write I/O completion handle. Hence the file size was not updated
on I/O completion.

Comments?

Cheers,

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

---
 fs/xfs/linux-2.6/xfs_aops.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_aops.c	2007-05-11 16:03:59.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c	2007-05-12 23:46:54.379994052 +1000
@@ -973,8 +973,9 @@ xfs_page_state_convert(
 
 	bh = head = page_buffers(page);
 	offset = page_offset(page);
-	flags = -1;
-	type = IOMAP_READ;
+	iomap_valid = 0;
+	flags = BMAPI_READ;
+	type = IOMAP_NEW;
 
 	/* TODO: cleanup count and page_dirty */
 
@@ -1004,14 +1005,14 @@ xfs_page_state_convert(
 		 *
 		 * Third case, an unmapped buffer was found, and we are
 		 * in a path where we need to write the whole page out.
- 		 */
+		 */
 		if (buffer_unwritten(bh) || buffer_delay(bh) ||
 		    ((buffer_uptodate(bh) || PageUptodate(page)) &&
 		     !buffer_mapped(bh) && (unmapped || startio))) {
-		     	/*
+			/*
 			 * Make sure we don't use a read-only iomap
 			 */
-		     	if (flags == BMAPI_READ)
+			if (flags == BMAPI_READ)
 				iomap_valid = 0;
 
 			if (buffer_unwritten(bh)) {
@@ -1060,7 +1061,7 @@ xfs_page_state_convert(
 			 * That means it must already have extents allocated
 			 * underneath it. Map the extent by reading it.
 			 */
-			if (!iomap_valid || type != IOMAP_READ) {
+			if (!iomap_valid || flags != BMAPI_READ) {
 				flags = BMAPI_READ;
 				size = xfs_probe_cluster(inode, page, bh,
 								head, 1);
@@ -1071,7 +1072,15 @@ xfs_page_state_convert(
 				iomap_valid = xfs_iomap_valid(&iomap, offset);
 			}
 
-			type = IOMAP_READ;
+			/*
+			 * We set the type to IOMAP_NEW in case we are doing a
+			 * small write at EOF that is extending the file but
+			 * without needing an allocation. We need to update the
+			 * file size on I/O completion in this case so it is
+			 * the same case as having just allocated a new extent
+			 * that we are writing into for the first time.
+			 */
+			type = IOMAP_NEW;
 			if (!test_and_set_bit(BH_Lock, &bh->b_state)) {
 				ASSERT(buffer_mapped(bh));
 				if (iomap_valid)

Re: Review: ensure EOF writes into existing extents update filesize

[Chris Wedgwood]

On Mon, May 21, 2007 at 09:34:17AM +1000, David Chinner wrote:

> In that case, we use a read mapping of the extent, and this also
> happens to use the read I/O completion handler instead of the
> write I/O completion handle. Hence the file size was not updated
> on I/O completion.
>
> Comments?

I've had this (well, the first version you sent to Jeremy) for a few
days on a testing machine without any obvious ill effects.

Once people are happy with this it should be checked to see if it
needs to go into -stable.

Re: Review: ensure EOF writes into existing extents update filesize

[David Chinner]

On Sun, May 20, 2007 at 04:40:48PM -0700, Chris Wedgwood wrote:
> 
> Once people are happy with this it should be checked to see if it
> needs to go into -stable.

It's new code so -stable is irrelevant....

Cheers,

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

Re: Review: ensure EOF writes into existing extents update filesize

[Timothy Shimmin]

Hi,

Trying to understand how the initialisers prior to the loop
are used during the loop.
It looks like the initial "type" isn't used now with this change
as we always assign to it near to where we access it.
Previously, we did access it in the already mapped case (type != IOMAP_READ).
So why do we initialise "type" prior to the loop then?

The inited "flags" var is 1st accessed in the unmapped/allocated
codepath to set iomap_valid to zero on BMAPI_READ
or now also in the other path where it is already mapped.
Previously, flags was init to -1 (not one of the BMAPI macros) and
now it is set to BMAPI_READ.
So, previously if we came straight to an unwritten/delayed/unmapped buffer
we would not set iomap_valid to 0, as flags==-1, whereas now
we will reset it. But what is this doing anyway - is it really needed
to reset iomap_valid.

Nor sure if there are any ramifications of this but just trying to see differences.

So, xfs_add_to_ioend() sets up the io completion handlers.
Previously, we would have set up xfs_end_bio_read (for IOMAP_READ)
and now we set up xfs_end_bio_written (for IOMAP_NEW).
xfs_end_bio_written does an xfs_setfilesize(ioend)
which an xfs_end_bio_read does not.
Which I guess is the crux of this change and
that is apparent.

I'm having trouble following the existing code to understand what it is currently doing.
So you are better off with a reviewer that knows this code (but thought I'd
have a look:)
We seem to be continually testing for iomap_valid which I believe checks whether
the offset is within the mapped range.
If the offset is not within the mapping then we try mapping the <offset, size>
and then retest for validity.
So what happens when it is not valid even after mapping and why wouldn't it be valid.
I need a better understanding of the background code I guess.

Sorry,
Tim.


--On 21 May 2007 9:34:17 AM +1000 David Chinner <dgc@sgi.com> wrote:

>
> As reported a coupl eof times on lkml since the 2.6.22 window opened,
> XFS is not updating the file size corectly in all cases. This is a
> result of the null files fix not updating the file size when the
> write extending the file does not need to allocate a block.
>
> In that case, we use a read mapping of the extent, and this also
> happens to use the read I/O completion handler instead of the
> write I/O completion handle. Hence the file size was not updated
> on I/O completion.
>
> Comments?
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> Principal Engineer
> SGI Australian Software Group
>
> ---
>  fs/xfs/linux-2.6/xfs_aops.c |   23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_aops.c	2007-05-11 16:03:59.000000000 +1000
> +++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c	2007-05-12 23:46:54.379994052 +1000
> @@ -973,8 +973,9 @@ xfs_page_state_convert(
>
>  	bh = head = page_buffers(page);
>  	offset = page_offset(page);
> -	flags = -1;
> -	type = IOMAP_READ;
> +	iomap_valid = 0;
> +	flags = BMAPI_READ;
> +	type = IOMAP_NEW;
>
>  	/* TODO: cleanup count and page_dirty */
>
> @@ -1004,14 +1005,14 @@ xfs_page_state_convert(
>  		 *
>  		 * Third case, an unmapped buffer was found, and we are
>  		 * in a path where we need to write the whole page out.
> - 		 */
> +		 */
>  		if (buffer_unwritten(bh) || buffer_delay(bh) ||
>  		    ((buffer_uptodate(bh) || PageUptodate(page)) &&
>  		     !buffer_mapped(bh) && (unmapped || startio))) {
> -		     	/*
> +			/*
>  			 * Make sure we don't use a read-only iomap
>  			 */
> -		     	if (flags == BMAPI_READ)
> +			if (flags == BMAPI_READ)
>  				iomap_valid = 0;
>
>  			if (buffer_unwritten(bh)) {
> @@ -1060,7 +1061,7 @@ xfs_page_state_convert(
>  			 * That means it must already have extents allocated
>  			 * underneath it. Map the extent by reading it.
>  			 */
> -			if (!iomap_valid || type != IOMAP_READ) {
> +			if (!iomap_valid || flags != BMAPI_READ) {
>  				flags = BMAPI_READ;
>  				size = xfs_probe_cluster(inode, page, bh,
>  								head, 1);
> @@ -1071,7 +1072,15 @@ xfs_page_state_convert(
>  				iomap_valid = xfs_iomap_valid(&iomap, offset);
>  			}
>
> -			type = IOMAP_READ;
> +			/*
> +			 * We set the type to IOMAP_NEW in case we are doing a
> +			 * small write at EOF that is extending the file but
> +			 * without needing an allocation. We need to update the
> +			 * file size on I/O completion in this case so it is
> +			 * the same case as having just allocated a new extent
> +			 * that we are writing into for the first time.
> +			 */
> +			type = IOMAP_NEW;
>  			if (!test_and_set_bit(BH_Lock, &bh->b_state)) {
>  				ASSERT(buffer_mapped(bh));
>  				if (iomap_valid)

Re: Review: ensure EOF writes into existing extents update filesize

[David Chinner]

On Mon, May 21, 2007 at 04:34:26PM +1000, Timothy Shimmin wrote:
> Hi,
> 
> Trying to understand how the initialisers prior to the loop
> are used during the loop.
> It looks like the initial "type" isn't used now with this change
> as we always assign to it near to where we access it.

Yes.

> Previously, we did access it in the already mapped case (type != 
> IOMAP_READ).
> So why do we initialise "type" prior to the loop then?

Because it's good practise - it's not obvious that it gets
initialised in all cases within the loop, so lets make sure
that it is....

> The inited "flags" var is 1st accessed in the unmapped/allocated
> codepath to set iomap_valid to zero on BMAPI_READ
> or now also in the other path where it is already mapped.
.....
> Nor sure if there are any ramifications of this but just trying to see 
> differences.

There is no difference in behaviour....

> So, xfs_add_to_ioend() sets up the io completion handlers.
> Previously, we would have set up xfs_end_bio_read (for IOMAP_READ)
> and now we set up xfs_end_bio_written (for IOMAP_NEW).
> xfs_end_bio_written does an xfs_setfilesize(ioend)
> which an xfs_end_bio_read does not.
> Which I guess is the crux of this change and
> that is apparent.

Yes.

> I'm having trouble following the existing code to understand what
> it is currently doing.  So you are better off with a reviewer that
> knows this code (but thought I'd have a look:)

It's not obvious at all, is it?

> We seem to be continually testing for iomap_valid which I believe
> checks whether the offset is within the mapped range.  If the
> offset is not within the mapping then we try mapping the <offset,
> size> and then retest for validity.  So what happens when it is
> not valid even after mapping and why wouldn't it be valid.  I need
> a better understanding of the background code I guess.

The iomap that we get is a mapping of a range that is the same type
as the current bufferhead we are processing. Hence the mapping may
extend much further than the current buffer (e.g. delalloc or
unwritten mapping will extend to the end of the extent). therefore
as we walk each buffer, we need to check that it falls within the
current mapping and if it doesn't we need to map a new range.

We also need to invalidate the mapping if the buffer changes from
a mapped buffer to an unmapped/unwritten/delalloc buffer as we
need to do extra work there....

I guess I need to ping Christoph and Nathan on this one....

Cheers,

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

Re: Review: ensure EOF writes into existing extents update filesize

[Nathan Scott]

On Tue, 2007-05-22 at 10:44 +1000, David Chinner wrote:
> On Mon, May 21, 2007 at 04:34:26PM +1000, Timothy Shimmin wrote:
> I guess I need to ping Christoph and Nathan on this one....

Could you resend the patch to me please?  I lost the previous copy
while ruthlessly ploughing through my mail backlog.  ;)

cheers.

-- 
Nathan

Re: Review: ensure EOF writes into existing extents update filesize

[David Chinner]

On Tue, May 22, 2007 at 11:02:59AM +1000, Nathan Scott wrote:
> On Tue, 2007-05-22 at 10:44 +1000, David Chinner wrote:
> > On Mon, May 21, 2007 at 04:34:26PM +1000, Timothy Shimmin wrote:
> > I guess I need to ping Christoph and Nathan on this one....
> 
> Could you resend the patch to me please?  I lost the previous copy
> while ruthlessly ploughing through my mail backlog.  ;)

Below.

Cheers,

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

---
 fs/xfs/linux-2.6/xfs_aops.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_aops.c	2007-05-11 16:03:59.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c	2007-05-12 23:46:54.379994052 +1000
@@ -973,8 +973,9 @@ xfs_page_state_convert(
 
 	bh = head = page_buffers(page);
 	offset = page_offset(page);
-	flags = -1;
-	type = IOMAP_READ;
+	iomap_valid = 0;
+	flags = BMAPI_READ;
+	type = IOMAP_NEW;
 
 	/* TODO: cleanup count and page_dirty */
 
@@ -1004,14 +1005,14 @@ xfs_page_state_convert(
 		 *
 		 * Third case, an unmapped buffer was found, and we are
 		 * in a path where we need to write the whole page out.
- 		 */
+		 */
 		if (buffer_unwritten(bh) || buffer_delay(bh) ||
 		    ((buffer_uptodate(bh) || PageUptodate(page)) &&
 		     !buffer_mapped(bh) && (unmapped || startio))) {
-		     	/*
+			/*
 			 * Make sure we don't use a read-only iomap
 			 */
-		     	if (flags == BMAPI_READ)
+			if (flags == BMAPI_READ)
 				iomap_valid = 0;
 
 			if (buffer_unwritten(bh)) {
@@ -1060,7 +1061,7 @@ xfs_page_state_convert(
 			 * That means it must already have extents allocated
 			 * underneath it. Map the extent by reading it.
 			 */
-			if (!iomap_valid || type != IOMAP_READ) {
+			if (!iomap_valid || flags != BMAPI_READ) {
 				flags = BMAPI_READ;
 				size = xfs_probe_cluster(inode, page, bh,
 								head, 1);
@@ -1071,7 +1072,15 @@ xfs_page_state_convert(
 				iomap_valid = xfs_iomap_valid(&iomap, offset);
 			}
 
-			type = IOMAP_READ;
+			/*
+			 * We set the type to IOMAP_NEW in case we are doing a
+			 * small write at EOF that is extending the file but
+			 * without needing an allocation. We need to update the
+			 * file size on I/O completion in this case so it is
+			 * the same case as having just allocated a new extent
+			 * that we are writing into for the first time.
+			 */
+			type = IOMAP_NEW;
 			if (!test_and_set_bit(BH_Lock, &bh->b_state)) {
 				ASSERT(buffer_mapped(bh));
 				if (iomap_valid)

Re: Review: ensure EOF writes into existing extents update filesize

[Nathan Scott]

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

On Tue, 2007-05-22 at 11:03 +1000, David Chinner wrote:
> On Tue, May 22, 2007 at 11:02:59AM +1000, Nathan Scott wrote:
> > On Tue, 2007-05-22 at 10:44 +1000, David Chinner wrote:
> > > On Mon, May 21, 2007 at 04:34:26PM +1000, Timothy Shimmin wrote:
> > > I guess I need to ping Christoph and Nathan on this one....
> > 
> > Could you resend the patch to me please?  I lost the previous copy
> > while ruthlessly ploughing through my mail backlog.  ;)
> 
> Below.

Looks pretty good to me - xfs_convert_page has been overlooked, I
think - attached patch fixes that.  You also initialise iomap_valid
twice inside xfs_page_state_convert now ... I reverted that to just
the once.

cheers.

-- 
Nathan


[-- Attachment #2: nat.patch --]
[-- Type: text/x-patch, Size: 665 bytes --]

Index: linux/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- linux.orig/fs/xfs/linux-2.6/xfs_aops.c	2007-05-22 13:47:40.296829500 +1000
+++ linux/fs/xfs/linux-2.6/xfs_aops.c	2007-05-22 13:48:25.987685000 +1000
@@ -810,7 +810,7 @@ xfs_convert_page(
 			page_dirty--;
 			count++;
 		} else {
-			type = 0;
+			type = IOMAP_NEW;
 			if (buffer_mapped(bh) && all_bh && startio) {
 				lock_buffer(bh);
 				xfs_add_to_ioend(inode, bh, offset,
@@ -968,7 +968,6 @@ xfs_page_state_convert(
 
 	bh = head = page_buffers(page);
 	offset = page_offset(page);
-	iomap_valid = 0;
 	flags = BMAPI_READ;
 	type = IOMAP_NEW;
 

Re: Review: ensure EOF writes into existing extents update filesize

[David Chinner]

On Tue, May 22, 2007 at 02:10:14PM +1000, Nathan Scott wrote:
> On Tue, 2007-05-22 at 11:03 +1000, David Chinner wrote:
> > On Tue, May 22, 2007 at 11:02:59AM +1000, Nathan Scott wrote:
> > > On Tue, 2007-05-22 at 10:44 +1000, David Chinner wrote:
> > > > On Mon, May 21, 2007 at 04:34:26PM +1000, Timothy Shimmin wrote:
> > > > I guess I need to ping Christoph and Nathan on this one....
> > > 
> > > Could you resend the patch to me please?  I lost the previous copy
> > > while ruthlessly ploughing through my mail backlog.  ;)
> > 
> > Below.
> 
> Looks pretty good to me - xfs_convert_page has been overlooked, I
> think - attached patch fixes that.

I thought about that, then tried to trip the problem and was not
successful. AFAICT, if we have multiple pages dirty and in the same
state (i.e. pwrite 0 32769 to dirty 3 pages, then sync, then pwrite
0 33000 to dirty and extend) we should hit the case where we cluster
the dirty pages and call xfs_convert_page() on the last two pages.

In that case, the entire range should be mapped via the one iomap
and so the last buffer (the one we extended) should be added to an
ioend with a type of 0 (IOMAP_READ) and hence we should see the bug. 
With the patch I posted, I can't get this to show the problem. It
should, but it doesn't....

I'll make the change anyway to be safe, but I'm still perplexed
as to why it doesn't seem necessary....

Ah - there it is - xfs_is_delayed_page():

    699                         if (buffer_unwritten(bh))
    700                                 acceptable = (type == IOMAP_UNWRITTEN);
    701                         else if (buffer_delay(bh))
    702                                 acceptable = (type == IOMAP_DELAY);
    703                         else if (buffer_dirty(bh) && buffer_mapped(bh))
    704     >>>>>>>>>>>                 acceptable = (type == 0);
    705                         else
    706                                 break;

The ioend we started with now has type = IOMAP_NEW = 0x40 which 
means xfs_convert_page() aborts clustering this case immediately.
IOWs, we are never getting to this xfs_convert_page() case and
we are only passing through xfs_page_state_convert for mapped
pages.

> You also initialise iomap_valid
> twice inside xfs_page_state_convert now ... I reverted that to just
> the once.

I'll fold these changes in and fixup xfs_is_delayed_page() to look
for type == IOMAP_NEW and send out a new patch. Thanks, Nathan.

Cheers,

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

Re: Review: ensure EOF writes into existing extents update filesize

[David Chinner]

On Tue, May 22, 2007 at 04:05:59PM +1000, David Chinner wrote:
> On Tue, May 22, 2007 at 02:10:14PM +1000, Nathan Scott wrote:
> > On Tue, 2007-05-22 at 11:03 +1000, David Chinner wrote:
> > > On Tue, May 22, 2007 at 11:02:59AM +1000, Nathan Scott wrote:
> > > > On Tue, 2007-05-22 at 10:44 +1000, David Chinner wrote:
> > > > > On Mon, May 21, 2007 at 04:34:26PM +1000, Timothy Shimmin wrote:
> > > > > I guess I need to ping Christoph and Nathan on this one....
> > > > 
> > > > Could you resend the patch to me please?  I lost the previous copy
> > > > while ruthlessly ploughing through my mail backlog.  ;)
> > > 
> > > Below.
> > 
> > Looks pretty good to me - xfs_convert_page has been overlooked, I
> > think - attached patch fixes that.
....
> I'll make the change anyway to be safe, but I'm still perplexed
> as to why it doesn't seem necessary....
> 
> Ah - there it is - xfs_is_delayed_page():
> 
>     699                         if (buffer_unwritten(bh))
>     700                                 acceptable = (type == IOMAP_UNWRITTEN);
>     701                         else if (buffer_delay(bh))
>     702                                 acceptable = (type == IOMAP_DELAY);
>     703                         else if (buffer_dirty(bh) && buffer_mapped(bh))
>     704     >>>>>>>>>>>                 acceptable = (type == 0);
>     705                         else
>     706                                 break;
> 
> The ioend we started with now has type = IOMAP_NEW = 0x40 which 
> means xfs_convert_page() aborts clustering this case immediately.
> IOWs, we are never getting to this xfs_convert_page() case and
> we are only passing through xfs_page_state_convert for mapped
> pages.
> 
> > You also initialise iomap_valid
> > twice inside xfs_page_state_convert now ... I reverted that to just
> > the once.
> 
> I'll fold these changes in and fixup xfs_is_delayed_page() to look
> for type == IOMAP_NEW and send out a new patch. Thanks, Nathan.

Patch below, Nathan. It's passed my point tests and XFSQA...

Cheers,

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

---
 fs/xfs/linux-2.6/xfs_aops.c |   26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_aops.c	2007-05-21 17:49:01.603432320 +1000
+++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_aops.c	2007-05-22 16:07:36.787017461 +1000
@@ -706,7 +706,7 @@ xfs_is_delayed_page(
 			else if (buffer_delay(bh))
 				acceptable = (type == IOMAP_DELAY);
 			else if (buffer_dirty(bh) && buffer_mapped(bh))
-				acceptable = (type == 0);
+				acceptable = (type == IOMAP_NEW);
 			else
 				break;
 		} while ((bh = bh->b_this_page) != head);
@@ -815,7 +815,7 @@ xfs_convert_page(
 			page_dirty--;
 			count++;
 		} else {
-			type = 0;
+			type = IOMAP_NEW;
 			if (buffer_mapped(bh) && all_bh && startio) {
 				lock_buffer(bh);
 				xfs_add_to_ioend(inode, bh, offset,
@@ -973,8 +973,8 @@ xfs_page_state_convert(
 
 	bh = head = page_buffers(page);
 	offset = page_offset(page);
-	flags = -1;
-	type = IOMAP_READ;
+	flags = BMAPI_READ;
+	type = IOMAP_NEW;
 
 	/* TODO: cleanup count and page_dirty */
 
@@ -1004,14 +1004,14 @@ xfs_page_state_convert(
 		 *
 		 * Third case, an unmapped buffer was found, and we are
 		 * in a path where we need to write the whole page out.
- 		 */
+		 */
 		if (buffer_unwritten(bh) || buffer_delay(bh) ||
 		    ((buffer_uptodate(bh) || PageUptodate(page)) &&
 		     !buffer_mapped(bh) && (unmapped || startio))) {
-		     	/*
+			/*
 			 * Make sure we don't use a read-only iomap
 			 */
-		     	if (flags == BMAPI_READ)
+			if (flags == BMAPI_READ)
 				iomap_valid = 0;
 
 			if (buffer_unwritten(bh)) {
@@ -1060,7 +1060,7 @@ xfs_page_state_convert(
 			 * That means it must already have extents allocated
 			 * underneath it. Map the extent by reading it.
 			 */
-			if (!iomap_valid || type != IOMAP_READ) {
+			if (!iomap_valid || flags != BMAPI_READ) {
 				flags = BMAPI_READ;
 				size = xfs_probe_cluster(inode, page, bh,
 								head, 1);
@@ -1071,7 +1071,15 @@ xfs_page_state_convert(
 				iomap_valid = xfs_iomap_valid(&iomap, offset);
 			}
 
-			type = IOMAP_READ;
+			/*
+			 * We set the type to IOMAP_NEW in case we are doing a
+			 * small write at EOF that is extending the file but
+			 * without needing an allocation. We need to update the
+			 * file size on I/O completion in this case so it is
+			 * the same case as having just allocated a new extent
+			 * that we are writing into for the first time.
+			 */
+			type = IOMAP_NEW;
 			if (!test_and_set_bit(BH_Lock, &bh->b_state)) {
 				ASSERT(buffer_mapped(bh));
 				if (iomap_valid)

Re: Review: ensure EOF writes into existing extents update filesize

[Nathan Scott]

On Wed, 2007-05-23 at 10:02 +1000, David Chinner wrote:
> ...
> Patch below, Nathan. It's passed my point tests and XFSQA...

*nod* - looks good Dave, nice detective work there.

cheers.

-- 
Nathan