Review: ensure EOF writes into existing extents update filesize

DISCUSSION David Chinner 4 participants 11 messages May 20, 2007 – May 23, 2007 XFS filesystem

Share & tools

David Chinner

author

2007-05-20 23:34 UTC #0 lore raw

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.

Signature · 68 lines

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)

Chris Wedgwood

2007-05-20 23:40 UTC #1 lore raw

Quoted 6 lines · David Chinner

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.

David Chinner

author

2007-05-20 23:44 UTC #2 lore raw

Quoted 3 lines · Chris Wedgwood

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.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

Timothy Shimmin

2007-05-21 06:30 UTC #3 lore raw

In reply to David Chinner

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.

Quoted 85 lines · --On 21 May 2007 9:34:17 AM +1000 David Chinner <[email protected]> wrote:

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

fs/xfs/linux-2.6/xfs_aops.c +16 -7

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

David Chinner

author

2007-05-22 00:44 UTC #4 lore raw

Quoted 6 lines · 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.

Yes.

Quoted 3 lines

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

Quoted 3 lines

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.

.....

Quoted 2 lines

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

There is no difference in behaviour....

Quoted 7 lines

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.

Quoted 3 lines

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?

Quoted 6 lines

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.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

Nathan Scott

2007-05-22 00:57 UTC #5 lore raw

Quoted 2 lines · David Chinner

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.

Signature · 1 line

Nathan

David Chinner

author

2007-05-22 01:03 UTC #6 lore raw

Quoted 6 lines · Nathan Scott

Quoted 2 lines · David Chinner
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.

Signature · 68 lines

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)

Nathan Scott

2007-05-22 04:04 UTC #7 lore raw

Quoted 9 lines · David Chinner

Quoted 6 lines · Nathan Scott
Quoted 2 lines · David Chinner
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.

Signature · 1 line

Nathan

fs/xfs/linux-2.6/xfs_aops.c +1 -2

Index: linux/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
@@ -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;

David Chinner

author

2007-05-22 06:06 UTC #8 lore raw

Quoted 6 lines · Nathan Scott

Quoted 5 lines · David Chinner
Quoted 4 lines · Nathan Scott
Quoted 2 lines · David Chinner
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....

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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.

Quoted 3 lines

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.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

David Chinner

author

2007-05-23 00:02 UTC #9 lore raw

Quoted 14 lines · David Chinner

Quoted 13 lines · Nathan Scott
Quoted 9 lines · David Chinner
Quoted 6 lines · Nathan Scott
Quoted 2 lines · David Chinner
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.

....

Quoted 20 lines

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.

Quoted 6 lines

Quoted 3 lines
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.

Signature · 85 lines

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)

Nathan Scott

2007-05-23 00:09 UTC #10 lore raw

Quoted 2 lines · David Chinner

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

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

cheers.

Signature · 1 line

Nathan