diff mbox series

block: flush the disk cache on BLKFLSBUF

Message ID 1a33ace-57f9-9ef9-b967-d6617ca33089@redhat.com (mailing list archive)
State New, archived
Headers show
Series block: flush the disk cache on BLKFLSBUF | expand

Commit Message

Mikulas Patocka June 26, 2023, 8:25 p.m. UTC
The BLKFLSBUF ioctl doesn't send the flush bio to the block device, thus
flushed data may be lurking in the disk cache and they may not be really
flushed to the stable storage.

This patch adds the call to blkdev_issue_flush to blkdev_flushbuf.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 block/ioctl.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Christoph Hellwig June 27, 2023, 4:52 a.m. UTC | #1
On Mon, Jun 26, 2023 at 10:25:28PM +0200, Mikulas Patocka wrote:
> The BLKFLSBUF ioctl doesn't send the flush bio to the block device, thus
> flushed data may be lurking in the disk cache and they may not be really
> flushed to the stable storage.
> 
> This patch adds the call to blkdev_issue_flush to blkdev_flushbuf.

Umm, why?  This is an ioctl no one should be using, and we certainly
should not add new functionality to it.  Can you explain what you're
trying to do here?
Mikulas Patocka June 27, 2023, 3:31 p.m. UTC | #2
> On Mon, 26 Jun 2023, Mikulas Patocka wrote:
> 
> > The BLKFLSBUF ioctl doesn't send the flush bio to the block device, thus
> > flushed data may be lurking in the disk cache and they may not be really
> > flushed to the stable storage.
> > 
> > This patch adds the call to blkdev_issue_flush to blkdev_flushbuf.
> 
> Umm, why?  This is an ioctl no one should be using, and we certainly
> should not add new functionality to it.  Can you explain what you're
> trying to do here?

Marc Smith reported a bug where he wrote to the dm-writecache target using 
O_DIRECT, then reset the machine without proper shutdown and the freshly 
written data were lost. It turned out that he didn't use the fsync or 
fdatasync syscall (and dm-writecache makes its metadata persistent on a 
FLUSH bio).

When I was analyzing this issue, it turned out that there is no easy way 
how to send the FLUSH bio to a block device from a command line.

The sync command synchronizes only filesystems, it doesn't flush cache for 
unmounted block devices (do you think that it should flush block devices 
too?).

The "blockdev --flushbufs" command also doesn't send the FLUSH bio, but I 
would expect it to send it. Without sending the FLUSH bio, "blockdev 
--flushbufs" doesn't really guarantee anything.

Mikulas
Christoph Hellwig June 27, 2023, 3:49 p.m. UTC | #3
On Tue, Jun 27, 2023 at 05:31:12PM +0200, Mikulas Patocka wrote:
> Marc Smith reported a bug where he wrote to the dm-writecache target using 
> O_DIRECT, then reset the machine without proper shutdown and the freshly 
> written data were lost. It turned out that he didn't use the fsync or 
> fdatasync syscall (and dm-writecache makes its metadata persistent on a 
> FLUSH bio).

Which so far is expected.  Even with O_DIRECT you need O_(D)SYNC or
fsync/fdatasync to persist data.

> When I was analyzing this issue, it turned out that there is no easy way 
> how to send the FLUSH bio to a block device from a command line.

xfs_io -c fsync /dev/foo

> The "blockdev --flushbufs" command also doesn't send the FLUSH bio, but I 
> would expect it to send it. Without sending the FLUSH bio, "blockdev 
> --flushbufs" doesn't really guarantee anything.

I wouldn't expect it.  It's a really weird legacy thing that calls
back up into the file system, but only if it sets s_bdev to this
device.  I don't think we should add new users of it that overload the
semantics.
diff mbox series

Patch

Index: linux-2.6/block/ioctl.c
===================================================================
--- linux-2.6.orig/block/ioctl.c
+++ linux-2.6/block/ioctl.c
@@ -351,6 +351,7 @@  static int blkdev_flushbuf(struct block_
 		return -EACCES;
 	fsync_bdev(bdev);
 	invalidate_bdev(bdev);
+	blkdev_issue_flush(bdev);
 	return 0;
 }