ext4: fix blkdev_issue_flush() failure handling

PATCH In review Bartlomiej Zolnierkiewicz 6 participants 13 messages Mar 29, 2009 – Mar 31, 2009 ext4 file system

Share & tools

Bartlomiej Zolnierkiewicz

author

2009-03-29 17:30 UTC #0 lore raw

PATCH 1/1 ext4: fix blkdev_issue_flush() failure handling

blkdev_issue_flush() may fail (i.e. due to media error on FLUSH CACHE command execution) so its users should check for the return value.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 fs/ext4/fsync.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

fs/ext4/fsync.c +6 -3

Index: b/fs/ext4/fsync.c
===================================================================
@@ -48,7 +48,7 @@ int ext4_sync_file(struct file *file, st
 {
 	struct inode *inode = dentry->d_inode;
 	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
-	int ret = 0;
+	int ret = 0, tmp_ret;
 
 	J_ASSERT(ext4_journal_current_handle() == NULL);

@@ -92,8 +92,11 @@ int ext4_sync_file(struct file *file, st
 			.nr_to_write = 0, /* sys_fsync did this */
 		};
 		ret = sync_inode(inode, &wbc);
-		if (journal && (journal->j_flags & JBD2_BARRIER))
-			blkdev_issue_flush(inode->i_sb->s_bdev, NULL);
+		if (journal && (journal->j_flags & JBD2_BARRIER)) {
+			tmp_ret = blkdev_issue_flush(inode->i_sb->s_bdev, NULL);
+			if (ret == 0 && tmp_ret < 0 && tmp_ret != -EOPNOTSUPP)
+				ret = tmp_ret;
+		}
 	}
 out:
 	return ret;

Eric Sandeen

2009-03-29 17:43 UTC #1 lore raw

Quoted 25 lines · Bartlomiej Zolnierkiewicz

blkdev_issue_flush() may fail (i.e. due to media error on FLUSH CACHE command execution) so its users should check for the return value.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 fs/ext4/fsync.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Index: b/fs/ext4/fsync.c
===================================================================
fs/ext4/fsync.c +1 -1
@@ -48,7 +48,7 @@ int ext4_sync_file(struct file *file, st
 {
 	struct inode *inode = dentry->d_inode;
 	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
-	int ret = 0;
+	int ret = 0, tmp_ret;
 
 	J_ASSERT(ext4_journal_current_handle() == NULL);
 
@@ -92,8 +92,11 @@ int ext4_sync_file(struct file *file, st
 			.nr_to_write = 0, /* sys_fsync did this */
 		};
 		ret = sync_inode(inode, &wbc);

fs/ext4/fsync.c int ext4_sync_file(struct file *file, st · line 102

Quoted 10 lines

fs/ext4/fsync.c +5 -2

-		if (journal && (journal->j_flags & JBD2_BARRIER))
-			blkdev_issue_flush(inode->i_sb->s_bdev, NULL);
+		if (journal && (journal->j_flags & JBD2_BARRIER)) {
+			tmp_ret = blkdev_issue_flush(inode->i_sb->s_bdev, NULL);
+			if (ret == 0 && tmp_ret < 0 && tmp_ret != -EOPNOTSUPP)
+				ret = tmp_ret;
+		}
 	}
 out:
 	return ret;

As long as we keep the call there this is probably good, but after talking w/ Chris Mason, I think the call is extraneous anyway and should probably just be removed...

-Eric

Theodore Tso

maintainer

2009-03-30 02:25 UTC #2 lore raw

Quoted 5 lines · Eric Sandeen

As long as we keep the call there this is probably good, but after talking w/ Chris Mason, I think the call is extraneous anyway and should probably just be removed...

Yes, I agree, but it takes a lot of digging to be completely sure of that it's safe to remove it. Interestingly, it was you who added the patch which added the call to blkdev_issue_flush():

commit d755fb384250d6bd7fd18a0930e71965acc8e72e

Author: Eric Sandeen <[email protected]>
Date:   Fri Jul 11 19:27:31 2008 -0400

ext4: call blkdev_issue_flush on fsync

To ensure that bits are truly on-disk after an fsync, we should call blkdev_issue_flush if barriers are supported.

Inspired by an old thread on barriers, by reiserfs & xfs which do the same, and by a patch SuSE ships with their kernel

Signed-off-by Eric Sandeen <[email protected]>

Signed-off-by Mingming Cao <[email protected]>

Signed-off-by "Theodore Ts'o" <[email protected]>

When we remove it we should add a comment noting why it's not necessary.

- Ted

Eric Sandeen

2009-03-30 03:22 UTC #3 lore raw

Quoted 6 lines · Theodore Tso

Quoted 4 lines · Eric Sandeen
As long as we keep the call there this is probably good, but after talking w/ Chris Mason, I think the call is extraneous anyway and should probably just be removed...

Quoted 7 lines

Yes, I agree, but it takes a lot of digging to be completely sure of that it's safe to remove it. Interestingly, it was you who added the patch which added the call to blkdev_issue_flush():
commit d755fb384250d6bd7fd18a0930e71965acc8e72e
Author: Eric Sandeen <[email protected]>
Date:   Fri Jul 11 19:27:31 2008 -0400

Yes, it was. Although I got the idea when hch pointed out that SuSE did this... thanks to Chris. It's come full circle. :)

-Eric

Chris Mason

2009-03-30 11:48 UTC #4 lore raw

Quoted 11 lines · Eric Sandeen

Quoted 9 lines · Theodore Tso
Quoted 4 lines · Eric Sandeen
As long as we keep the call there this is probably good, but after talking w/ Chris Mason, I think the call is extraneous anyway and should probably just be removed...
Yes, I agree, but it takes a lot of digging to be completely sure of that it's safe to remove it. Interestingly, it was you who added the patch which added the call to blkdev_issue_flush():

Quoted 7 lines

Quoted 3 lines
commit d755fb384250d6bd7fd18a0930e71965acc8e72e
Author: Eric Sandeen <[email protected]>
Date:   Fri Jul 11 19:27:31 2008 -0400
Yes, it was. Although I got the idea when hch pointed out that SuSE did this... thanks to Chris. It's come full circle. :)

Grin. I'm not sure the I_DIRTY checks alone are enough to decide that a commit is required though. I think the inode could be clean but still have metadata that needs commit.

-chris

Fernando Luis Vázquez Cao

2009-03-30 13:01 UTC #5 lore raw

Quoted 14 lines · Chris Mason

Quoted 13 lines · Eric Sandeen
Quoted 11 lines · Theodore Tso
Quoted 4 lines · Eric Sandeen
As long as we keep the call there this is probably good, but after talking w/ Chris Mason, I think the call is extraneous anyway and should probably just be removed...
Yes, I agree, but it takes a lot of digging to be completely sure of that it's safe to remove it. Interestingly, it was you who added the patch which added the call to blkdev_issue_flush():
commit d755fb384250d6bd7fd18a0930e71965acc8e72e
Author: Eric Sandeen <[email protected]>
Date:   Fri Jul 11 19:27:31 2008 -0400

Quoted 6 lines

Quoted 2 lines
Yes, it was. Although I got the idea when hch pointed out that SuSE did this... thanks to Chris. It's come full circle. :)
Grin. I'm not sure the I_DIRTY checks alone are enough to decide that a commit is required though. I think the inode could be clean but still have metadata that needs commit.

Chris, I have just sent patches that attempt to fix both ext3 and ext4 while also adding a per-device sysfs knob tu disable write-flushes. A previous version of this patch set added a new generic mount option but comments from Christoph and others convinced me to turn it into a per-device tunable. Could you take a look at the patches?

Bartlomiej, I have just noticed that I happened to be working on patches for reiserfs and xfs similar to the ones you sent earlier this week. I picked some bits from your submission so I took the liberty to add your signed-off to my patches. Could you take a look at them and let me know if you are confortable with that?

Latest patches: http://lkml.org/lkml/2009/3/30/100
Beginning of the sub-thread: http://lkml.org/lkml/2009/3/29/28

Best regards,

Fernando

Chris Mason

2009-03-30 13:24 UTC #6 lore raw

Quoted 22 lines · Fernando Luis Vázquez Cao

Quoted 14 lines · Chris Mason
Quoted 13 lines · Eric Sandeen
Quoted 11 lines · Theodore Tso
Quoted 4 lines · Eric Sandeen
As long as we keep the call there this is probably good, but after talking w/ Chris Mason, I think the call is extraneous anyway and should probably just be removed...
Yes, I agree, but it takes a lot of digging to be completely sure of that it's safe to remove it. Interestingly, it was you who added the patch which added the call to blkdev_issue_flush():
commit d755fb384250d6bd7fd18a0930e71965acc8e72e
Author: Eric Sandeen <[email protected]>
Date:   Fri Jul 11 19:27:31 2008 -0400
Quoted 6 lines
Quoted 2 lines
Yes, it was. Although I got the idea when hch pointed out that SuSE did this... thanks to Chris. It's come full circle. :)
Grin. I'm not sure the I_DIRTY checks alone are enough to decide that a commit is required though. I think the inode could be clean but still have metadata that needs commit.

Quoted 7 lines

Chris, I have just sent patches that attempt to fix both ext3 and ext4 while also adding a per-device sysfs knob tu disable write-flushes. A previous version of this patch set added a new generic mount option but comments from Christoph and others convinced me to turn it into a per-device tunable. Could you take a look at the patches?

Jens' comment are right on I think. If we get that fixed up we can get rid of all the filesystem mount -o barrier=flush,0,1,xyz confusion and set it via the block devices directly.

That would be nice ;)

-chris

Fernando Luis Vázquez Cao

2009-03-31 04:28 UTC #7 lore raw

Quoted 28 lines · Chris Mason

Quoted 19 lines · Fernando Luis Vázquez Cao
Quoted 13 lines · Chris Mason
Quoted 12 lines · Eric Sandeen
Quoted 11 lines · Theodore Tso
Quoted 4 lines · Eric Sandeen
As long as we keep the call there this is probably good, but after talking w/ Chris Mason, I think the call is extraneous anyway and should probably just be removed...
Yes, I agree, but it takes a lot of digging to be completely sure of that it's safe to remove it. Interestingly, it was you who added the patch which added the call to blkdev_issue_flush():
commit d755fb384250d6bd7fd18a0930e71965acc8e72e
Author: Eric Sandeen <[email protected]>
Date:   Fri Jul 11 19:27:31 2008 -0400
Quoted 5 lines
Quoted 2 lines
Yes, it was. Although I got the idea when hch pointed out that SuSE did this... thanks to Chris. It's come full circle. :)
Grin. I'm not sure the I_DIRTY checks alone are enough to decide that a commit is required though. I think the inode could be clean but still have metadata that needs commit.
Quoted 7 lines
Chris, I have just sent patches that attempt to fix both ext3 and ext4 while also adding a per-device sysfs knob tu disable write-flushes. A previous version of this patch set added a new generic mount option but comments from Christoph and others convinced me to turn it into a per-device tunable. Could you take a look at the patches?

Quoted 5 lines

Jens' comment are right on I think. If we get that fixed up we can get rid of all the filesystem mount -o barrier=flush,0,1,xyz confusion and set it via the block devices directly.
That would be nice ;)

Thank you for your feedback, Chris! I will address some of the issues spotted in the mailing list and resend the whole patch-set.

Regards,

Fernando
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Theodore Tso

maintainer

2009-03-30 14:25 UTC #8 lore raw

In reply to Fernando Luis Vázquez Cao

Quoted 7 lines · Fernando Luis Vázquez Cao

Chris, I have just sent patches that attempt to fix both ext3 and ext4 while also adding a per-device sysfs knob tu disable write-flushes. A previous version of this patch set added a new generic mount option but comments from Christoph and others convinced me to turn it into a per-device tunable. Could you take a look at the patches?

Fernando, see my comments on those patches. We don't need to issue a barrier after a call to sync_inode() or ext[34]_force_commit(), since those functions will issue a barrier for us. It would probably be a good idea to use blktrace to test and make sure that we have one and exactly one barrier op issued for each fsync().

- Ted

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Fernando Luis Vázquez Cao

2009-03-31 04:33 UTC #9 lore raw

Quoted 8 lines · Theodore Tso

Quoted 6 lines · Fernando Luis Vázquez Cao
Chris, I have just sent patches that attempt to fix both ext3 and ext4 while also adding a per-device sysfs knob tu disable write-flushes. A previous version of this patch set added a new generic mount option but comments from Christoph and others convinced me to turn it into a per-device tunable. Could you take a look at the patches?

Quoted 5 lines

Fernando, see my comments on those patches. We don't need to issue a barrier after a call to sync_inode() or ext[34]_force_commit(), since those functions will issue a barrier for us. It would probably be a good idea to use blktrace to test and make sure that we have one and exactly one barrier op issued for each fsync().

I'll give blktrace a spin and check if things are working as expected.

Thanks!

- Fernando
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Bartlomiej Zolnierkiewicz

author

2009-03-30 15:22 UTC #10 lore raw

In reply to Fernando Luis Vázquez Cao

Hi,

Quoted 29 lines · Fernando Luis Vázquez Cao

Quoted 14 lines · Chris Mason
Quoted 13 lines · Eric Sandeen
Quoted 11 lines · Theodore Tso
Quoted 4 lines · Eric Sandeen
As long as we keep the call there this is probably good, but after talking w/ Chris Mason, I think the call is extraneous anyway and should probably just be removed...
Yes, I agree, but it takes a lot of digging to be completely sure of that it's safe to remove it. Interestingly, it was you who added the patch which added the call to blkdev_issue_flush():
commit d755fb384250d6bd7fd18a0930e71965acc8e72e
Author: Eric Sandeen <[email protected]>
Date:   Fri Jul 11 19:27:31 2008 -0400
Quoted 6 lines
Quoted 2 lines
Yes, it was. Although I got the idea when hch pointed out that SuSE did this... thanks to Chris. It's come full circle. :)
Grin. I'm not sure the I_DIRTY checks alone are enough to decide that a commit is required though. I think the inode could be clean but still have metadata that needs commit.
Chris, I have just sent patches that attempt to fix both ext3 and ext4 while also adding a per-device sysfs knob tu disable write-flushes. A previous version of this patch set added a new generic mount option but comments from Christoph and others convinced me to turn it into a per-device tunable. Could you take a look at the patches?

Quoted 5 lines

Bartlomiej, I have just noticed that I happened to be working on patches for reiserfs and xfs similar to the ones you sent earlier this week. I picked some bits from your submission so I took the liberty to add your signed-off to my patches. Could you take a look at them and let me know if you are confortable with that?

I'm fine with people building bigger changes on top of my patches but if you do so you please clearly denote in the patch description what changes you have applied to the original patch...

Thanks,
Bart
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Fernando Luis Vázquez Cao

2009-03-31 04:23 UTC #11 lore raw

Quoted 35 lines · Bartlomiej Zolnierkiewicz

Hi,
Quoted 26 lines · Fernando Luis Vázquez Cao
Quoted 13 lines · Chris Mason
Quoted 12 lines · Eric Sandeen
Quoted 11 lines · Theodore Tso
Quoted 4 lines · Eric Sandeen
As long as we keep the call there this is probably good, but after talking w/ Chris Mason, I think the call is extraneous anyway and should probably just be removed...
Yes, I agree, but it takes a lot of digging to be completely sure of that it's safe to remove it. Interestingly, it was you who added the patch which added the call to blkdev_issue_flush():
commit d755fb384250d6bd7fd18a0930e71965acc8e72e
Author: Eric Sandeen <[email protected]>
Date:   Fri Jul 11 19:27:31 2008 -0400
Quoted 5 lines
Quoted 2 lines
Yes, it was. Although I got the idea when hch pointed out that SuSE did this... thanks to Chris. It's come full circle. :)
Grin. I'm not sure the I_DIRTY checks alone are enough to decide that a commit is required though. I think the inode could be clean but still have metadata that needs commit.
Chris, I have just sent patches that attempt to fix both ext3 and ext4 while also adding a per-device sysfs knob tu disable write-flushes. A previous version of this patch set added a new generic mount option but comments from Christoph and others convinced me to turn it into a per-device tunable. Could you take a look at the patches?
Quoted 5 lines
Bartlomiej, I have just noticed that I happened to be working on patches for reiserfs and xfs similar to the ones you sent earlier this week. I picked some bits from your submission so I took the liberty to add your signed-off to my patches. Could you take a look at them and let me know if you are confortable with that?

Quoted 3 lines

I'm fine with people building bigger changes on top of my patches but if you do so you please clearly denote in the patch description what changes you have applied to the original patch...

You are right, sorry about that. I will add a short changelog when I resubmit the patches.

Thanks!

- Fernando
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Theodore Tso

maintainer

2009-03-30 17:46 UTC #12 lore raw

In reply to Chris Mason

Quoted 6 lines · Chris Mason

Quoted 2 lines
Yes, it was. Although I got the idea when hch pointed out that SuSE did this... thanks to Chris. It's come full circle. :)
Grin. I'm not sure the I_DIRTY checks alone are enough to decide that a commit is required though. I think the inode could be clean but still have metadata that needs commit.

So to close this hole, I think what we can do is to track the last transaction id where ext4_do_update_inode() was called, and if that transaction id == the currently running transaction id, then we need to call ext4_force_commit() even though the inode is clean. I think that should fix up the race that you're concerned about.

- Ted