blkdev: flush disk cache on ->fsync

PATCH In review Christoph Hellwig 3 participants 7 messages Aug 20, 2009 – Oct 13, 2009 VFS

Share & tools

Christoph Hellwig

author

2009-08-20 18:24 UTC #0 lore raw

PATCH 1/1 blkdev: flush disk cache on ->fsync

Currently there is no barrier support in the block device code. That means we cannot guarantee any sort of data integerity when using the block device node with dis kwrite caches enabled. Using the raw block device node is a typical use case for virtualization (and I assume databases, too). This patch changes block_fsync to issue a cache flush and thus make fsync on block device nodes actually useful.

Note that in mainline we would also need to add such code to the ->aio_write method for O_SYNC handling, but assuming that Jan's patch series for the O_SYNC rewrite goes in it will also call into ->fsync for 2.6.32.

Signed-off-by Christoph Hellwig <[email protected]>

fs/block_dev.c +11 -1

index 94dfda2..298ad75 100644
@@ -398,7 +398,17 @@ static loff_t block_llseek(struct file *file, loff_t offset, int origin)
  
 static int block_fsync(struct file *filp, struct dentry *dentry, int datasync)
 {
-	return sync_blockdev(I_BDEV(filp->f_mapping->host));
+	struct block_device *bdev = I_BDEV(filp->f_mapping->host);
+	int error;
+
+	error = sync_blockdev(bdev);
+	if (error)
+		return error;
+	
+	error = blkdev_issue_flush(bdev, NULL);
+	if (error == -EOPNOTSUPP)
+		error = 0;
+	return error;
 }
 
 /*

Christoph Hellwig

author

2009-10-10 16:16 UTC #1 lore raw

Is anyone going to pick up this patch?

Quoted 29 lines · Christoph Hellwig

Currently there is no barrier support in the block device code. That means we cannot guarantee any sort of data integerity when using the block device node with dis kwrite caches enabled. Using the raw block device node is a typical use case for virtualization (and I assume databases, too). This patch changes block_fsync to issue a cache flush and thus make fsync on block device nodes actually useful.
Note that in mainline we would also need to add such code to the ->aio_write method for O_SYNC handling, but assuming that Jan's patch series for the O_SYNC rewrite goes in it will also call into ->fsync for 2.6.32.
Signed-off-by Christoph Hellwig <[email protected]>
fs/block_dev.c +4 -1
index 94dfda2..298ad75 100644
@@ -398,7 +398,17 @@ static loff_t block_llseek(struct file *file, loff_t offset, int origin)
 
 static int block_fsync(struct file *filp, struct dentry *dentry, int datasync)
 {
-	return sync_blockdev(I_BDEV(filp->f_mapping->host));
+	struct block_device *bdev = I_BDEV(filp->f_mapping->host);
+	int error;
+
+	error = sync_blockdev(bdev);

fs/block_dev.c static loff_t block_llseek(struct file *file, loff_t offset, int origin) · line 414

Quoted 10 lines

fs/block_dev.c +7 -0

+	if (error)
+		return error;
+	
+	error = blkdev_issue_flush(bdev, NULL);
+	if (error == -EOPNOTSUPP)
+		error = 0;
+	return error;
 }
 
 /*

---end quoted text---

Jeff Moyer

Acked-by

2009-10-12 15:05 UTC #2 lore raw

Quoted 41 lines · Christoph Hellwig

Is anyone going to pick up this patch?
Quoted 38 lines · Christoph Hellwig
Currently there is no barrier support in the block device code. That means we cannot guarantee any sort of data integerity when using the block device node with dis kwrite caches enabled. Using the raw block device node is a typical use case for virtualization (and I assume databases, too). This patch changes block_fsync to issue a cache flush and thus make fsync on block device nodes actually useful.
Note that in mainline we would also need to add such code to the ->aio_write method for O_SYNC handling, but assuming that Jan's patch series for the O_SYNC rewrite goes in it will also call into ->fsync for 2.6.32.
Signed-off-by Christoph Hellwig <[email protected]>
fs/block_dev.c +11 -1
index 94dfda2..298ad75 100644
@@ -398,7 +398,17 @@ static loff_t block_llseek(struct file *file, loff_t offset, int origin)
 
 static int block_fsync(struct file *filp, struct dentry *dentry, int datasync)
 {
-	return sync_blockdev(I_BDEV(filp->f_mapping->host));
+	struct block_device *bdev = I_BDEV(filp->f_mapping->host);
+	int error;
+
+	error = sync_blockdev(bdev);
+	if (error)
+		return error;
+	
+	error = blkdev_issue_flush(bdev, NULL);
+	if (error == -EOPNOTSUPP)
+		error = 0;
+	return error;
 }
 

Quoted 2 lines

fs/block_dev.c static loff_t block_llseek(struct file *file, loff_t offset, int origin) · line 414
Quoted 1 line
fs/block_dev.c +0 -0
 /*
---end quoted text---

Looks ok to me. Jens?

Acked-by Jeff Moyer <[email protected]>

Jens Axboe

2009-10-12 15:07 UTC #3 lore raw

Quoted 43 lines · Jeff Moyer

Quoted 41 lines · Christoph Hellwig
Is anyone going to pick up this patch?
Quoted 38 lines · Christoph Hellwig
Currently there is no barrier support in the block device code. That means we cannot guarantee any sort of data integerity when using the block device node with dis kwrite caches enabled. Using the raw block device node is a typical use case for virtualization (and I assume databases, too). This patch changes block_fsync to issue a cache flush and thus make fsync on block device nodes actually useful.
Note that in mainline we would also need to add such code to the ->aio_write method for O_SYNC handling, but assuming that Jan's patch series for the O_SYNC rewrite goes in it will also call into ->fsync for 2.6.32.
Signed-off-by Christoph Hellwig <[email protected]>
fs/block_dev.c +11 -1
index 94dfda2..298ad75 100644
@@ -398,7 +398,17 @@ static loff_t block_llseek(struct file *file, loff_t offset, int origin)
 
 static int block_fsync(struct file *filp, struct dentry *dentry, int datasync)
 {
-	return sync_blockdev(I_BDEV(filp->f_mapping->host));
+	struct block_device *bdev = I_BDEV(filp->f_mapping->host);
+	int error;
+
+	error = sync_blockdev(bdev);
+	if (error)
+		return error;
+	
+	error = blkdev_issue_flush(bdev, NULL);
+	if (error == -EOPNOTSUPP)
+		error = 0;
+	return error;
 }
 

Quoted 4 lines

Quoted 2 lines
fs/block_dev.c static loff_t block_llseek(struct file *file, loff_t offset, int origin) · line 414
Quoted 1 line
fs/block_dev.c +0 -0
 /*
---end quoted text---
Looks ok to me. Jens?

Yep ditto!

Quoted 2 lines

Acked-by Jeff Moyer <[email protected]>

Signature · 1 line

Jens Axboe

Jens Axboe

2009-10-12 15:07 UTC #4 lore raw

In reply to Christoph Hellwig

Quoted 1 line · Christoph Hellwig

Is anyone going to pick up this patch?

I can pick it up for .33. I think it looks good, but it's probably something we should add up front.

Signature · 1 line

Jens Axboe

Christoph Hellwig

author

2009-10-13 01:42 UTC #5 lore raw

Quoted 5 lines · Jens Axboe

Quoted 1 line · Christoph Hellwig
Is anyone going to pick up this patch?
I can pick it up for .33. I think it looks good, but it's probably something we should add up front.

Given that it's impossible to get data to a raw block device reliably without this I think it's .32 material. Especially given how small the patch is and how long it's been out there..

Jens Axboe

2009-10-13 11:31 UTC #6 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Jens Axboe
Quoted 1 line · Christoph Hellwig
Is anyone going to pick up this patch?
I can pick it up for .33. I think it looks good, but it's probably something we should add up front.
Given that it's impossible to get data to a raw block device reliably without this I think it's .32 material. Especially given how small the patch is and how long it's been out there..

Yes I agree, but it's also not a regression since this has been the situation for eternity. So why not just accept .33 and we can push it to -stable after that?

Signature · 1 line

Jens Axboe