[PATCH] blkdev: flush disk cache on ->fsync

[PATCH] blkdev: flush disk cache on ->fsync

[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 <hch@lst.de>


diff --git a/fs/block_dev.c b/fs/block_dev.c
index 94dfda2..298ad75 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -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;
 }
 
 /*

Re: [PATCH] blkdev: flush disk cache on ->fsync

[Christoph Hellwig]

Is anyone going to pick up this patch?

On Thu, Aug 20, 2009 at 08:24:32PM +0200, Christoph Hellwig wrote:
> 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 <hch@lst.de>
> 
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 94dfda2..298ad75 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -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;
>  }
>  
>  /*
---end quoted text---

Re: [PATCH] blkdev: flush disk cache on ->fsync

[Jeff Moyer]

Christoph Hellwig <hch@lst.de> writes:

> Is anyone going to pick up this patch?
>
> On Thu, Aug 20, 2009 at 08:24:32PM +0200, Christoph Hellwig wrote:
>> 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 <hch@lst.de>
>> 
>> 
>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>> index 94dfda2..298ad75 100644
>> --- a/fs/block_dev.c
>> +++ b/fs/block_dev.c
>> @@ -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;
>>  }
>>  
>>  /*
> ---end quoted text---

Looks ok to me.  Jens?

Acked-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH] blkdev: flush disk cache on ->fsync

[Jens Axboe]

On Mon, Oct 12 2009, Jeff Moyer wrote:
> Christoph Hellwig <hch@lst.de> writes:
> 
> > Is anyone going to pick up this patch?
> >
> > On Thu, Aug 20, 2009 at 08:24:32PM +0200, Christoph Hellwig wrote:
> >> 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 <hch@lst.de>
> >> 
> >> 
> >> diff --git a/fs/block_dev.c b/fs/block_dev.c
> >> index 94dfda2..298ad75 100644
> >> --- a/fs/block_dev.c
> >> +++ b/fs/block_dev.c
> >> @@ -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;
> >>  }
> >>  
> >>  /*
> > ---end quoted text---
> 
> Looks ok to me.  Jens?

Yep ditto!

> 
> Acked-by: Jeff Moyer <jmoyer@redhat.com>

-- 
Jens Axboe

Re: [PATCH] blkdev: flush disk cache on ->fsync

[Jens Axboe]

On Sat, Oct 10 2009, Christoph Hellwig wrote:
> 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.

-- 
Jens Axboe

Re: [PATCH] blkdev: flush disk cache on ->fsync

[Christoph Hellwig]

On Mon, Oct 12, 2009 at 05:07:13PM +0200, Jens Axboe wrote:
> On Sat, Oct 10 2009, Christoph Hellwig wrote:
> > 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..

Re: [PATCH] blkdev: flush disk cache on ->fsync

[Jens Axboe]

On Tue, Oct 13 2009, Christoph Hellwig wrote:
> On Mon, Oct 12, 2009 at 05:07:13PM +0200, Jens Axboe wrote:
> > On Sat, Oct 10 2009, Christoph Hellwig wrote:
> > > 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?

-- 
Jens Axboe