block: Add flush after metadata writes
cover block: Add flush after metadata writes
This addresses the data integrity problems described at
http://wiki.qemu.org/Features/Qcow2DataIntegrity#Metadata_update_ordering.2C_Part_2
These problems are the same for all image formats (except raw, which doesn't have any metadata), so I'm going to add more patches for the other formats for the real patch submission.
Kevin Wolf (2):
block: Add bdrv_(p)write_sync
qcow2: Use bdrv_(p)write_sync for metadata writes
block.c | 37 +++++++++++++++++++++++++++++++++++++ block.h | 4 ++++ block/qcow2-cluster.c | 16 ++++++++-------- block/qcow2-refcount.c | 18 +++++++++--------- block/qcow2-snapshot.c | 14 +++++++------- block/qcow2.c | 10 +++++----- 6 files changed, 70 insertions(+), 29 deletions(-)
RFC PATCH 1/2 block: Add bdrv_(p)write_sync
Add new functions that write and flush the written data to disk immediately. This is what needs to be used for image format metadata to maintain integrity for cache=... modes that don't use O_DSYNC. (Actually, we only need barriers, and therefore the functions are defined as such, but flushes is what is implemented in this patch - we can try to change that later)
--- block.c | 37 +++++++++++++++++++++++++++++++++++++ block.h | 4 ++++ 2 files changed, 41 insertions(+), 0 deletions(-)
block.c +37 -0
@@ -1010,6 +1010,43 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,return count1; } +/* +* Writes to the file and ensures that no writes are reordered across this +* request (acts as a barrier) +* +* Returns 0 on success, -errno in error cases. +*/ +int bdrv_pwrite_sync (BlockDriverState *bs,int64_t offset, +const void *buf,int count) +{ +int ret; + + ret =bdrv_pwrite (bs, offset, buf, count); +if (ret <0 ) { +return ret; + } + +/* No flush needed for cache=writethrough, it uses O_DSYNC */ +if ((bs->open_flags &BDRV_O_CACHE_MASK ) !=0 ) { +bdrv_flush (bs); + } + +return 0 ; +} + +/* +* Writes to the file and ensures that no writes are reordered across this +* request (acts as a barrier) +* +* Returns 0 on success, -errno in error cases. +*/ +int bdrv_write_sync (BlockDriverState *bs,int64_t sector_num, +const uint8_t *buf,int nb_sectors) +{ +return bdrv_pwrite_sync (bs,BDRV_SECTOR_SIZE * sector_num, + buf,BDRV_SECTOR_SIZE * nb_sectors); +} +/** * Truncate file to 'offset' bytes (needed only for file protocols) */
block.h +4 -0
@@ -80,6 +80,10 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset, void *buf, int count);int bdrv_pwrite (BlockDriverState *bs,int64_t offset,const void *buf,int count); +int bdrv_pwrite_sync (BlockDriverState *bs,int64_t offset, +const void *buf,int count); +int bdrv_write_sync (BlockDriverState *bs,int64_t sector_num, +const uint8_t *buf,int nb_sectors);int bdrv_truncate (BlockDriverState *bs,int64_t offset);int64_t bdrv_getlength (BlockDriverState *bs);void bdrv_get_geometry (BlockDriverState *bs,uint64_t *nb_sectors_ptr);
Signature · 1 line
1.6.6.1
RFC PATCH 2/2 qcow2: Use bdrv_(p)write_sync for metadata writes
In reply to Kevin WolfUse bdrv_(p)write_sync to ensure metadata integrity in case of a crash.
--- block/qcow2-cluster.c | 16 ++++++++-------- block/qcow2-refcount.c | 18 +++++++++--------- block/qcow2-snapshot.c | 14 +++++++------- block/qcow2.c | 10 +++++----- 4 files changed, 29 insertions(+), 29 deletions(-)
block/qcow2-cluster.c +8 -8
@@ -64,7 +64,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)BLKDBG_EVENT (bs->file,BLKDBG_L1_GROW_WRITE_TABLE );for (i =0 ; i < s->l1_size; i++) new_l1_table[i] =cpu_to_be64 (new_l1_table[i]); - ret =bdrv_pwrite (bs->file, new_l1_table_offset, new_l1_table, new_l1_size2); + ret =bdrv_pwrite_sync (bs->file, new_l1_table_offset, new_l1_table, new_l1_size2);if (ret != new_l1_size2) goto fail;for (i =0 ; i < s->l1_size; i++)
@@ -74,7 +74,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)BLKDBG_EVENT (bs->file,BLKDBG_L1_GROW_ACTIVATE_TABLE );cpu_to_be32w ((uint32_t *)data, new_l1_size);cpu_to_be64w ((uint64_t *)(data +4 ), new_l1_table_offset); - ret =bdrv_pwrite (bs->file, offsetof(QCowHeader , l1_size), data,sizeof (data)); + ret =bdrv_pwrite_sync (bs->file, offsetof(QCowHeader , l1_size), data,sizeof (data));if (ret !=sizeof (data)) { goto fail; }
@@ -207,7 +207,7 @@ static int write_l1_entry(BlockDriverState *bs, int l1_index) }BLKDBG_EVENT (bs->file,BLKDBG_L1_UPDATE ); - ret =bdrv_pwrite (bs->file, s->l1_table_offset +8 * l1_start_index, + ret =bdrv_pwrite_sync (bs->file, s->l1_table_offset +8 * l1_start_index, buf,sizeof (buf));if (ret <0 ) {return ret;
@@ -263,7 +263,7 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table) }/* write the l2 table to the file */ BLKDBG_EVENT (bs->file,BLKDBG_L2_ALLOC_WRITE ); - ret =bdrv_pwrite (bs->file, l2_offset, l2_table, + ret =bdrv_pwrite_sync (bs->file, l2_offset, l2_table, s->l2_size *sizeof (uint64_t ));if (ret <0 ) { goto fail;
@@ -413,8 +413,8 @@ static int copy_sectors(BlockDriverState *bs, uint64_t start_sect, &s->aes_encrypt_key); }BLKDBG_EVENT (bs->file,BLKDBG_COW_WRITE ); - ret =bdrv_write (bs->file, (cluster_offset >>9 ) + n_start, - s->cluster_data, n); + ret =bdrv_write_sync (bs->file, (cluster_offset >>9 ) + n_start, + s->cluster_data, n);if (ret <0 )return ret;return 0 ;
@@ -631,7 +631,7 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,BLKDBG_EVENT (bs->file,BLKDBG_L2_UPDATE_COMPRESSED ); l2_table[l2_index] =cpu_to_be64 (cluster_offset); -if (bdrv_pwrite (bs->file, +if (bdrv_pwrite_sync (bs->file, l2_offset + l2_index *sizeof (uint64_t ), l2_table + l2_index,sizeof (uint64_t )) !=sizeof (uint64_t ))
@@ -655,7 +655,7 @@ static int write_l2_entries(BlockDriverState *bs, uint64_t *l2_table,int ret;BLKDBG_EVENT (bs->file,BLKDBG_L2_UPDATE ); - ret =bdrv_pwrite (bs->file, l2_offset + start_offset, + ret =bdrv_pwrite_sync (bs->file, l2_offset + start_offset, &l2_table[l2_start_index], len);if (ret <0 ) {return ret;
block/qcow2-refcount.c +9 -9
@@ -44,7 +44,7 @@ static int write_refcount_block(BlockDriverState *bs) }BLKDBG_EVENT (bs->file,BLKDBG_REFBLOCK_UPDATE ); -if (bdrv_pwrite (bs->file, s->refcount_block_cache_offset, +if (bdrv_pwrite_sync (bs->file, s->refcount_block_cache_offset, s->refcount_block_cache, size) != size) {return -EIO ;
@@ -269,7 +269,7 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)/* Now the new refcount block needs to be written to disk */ BLKDBG_EVENT (bs->file,BLKDBG_REFBLOCK_ALLOC_WRITE ); - ret =bdrv_pwrite (bs->file, new_block, s->refcount_block_cache, + ret =bdrv_pwrite_sync (bs->file, new_block, s->refcount_block_cache, s->cluster_size);if (ret <0 ) { goto fail_block;
@@ -279,7 +279,7 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)if (refcount_table_index < s->refcount_table_size) {uint64_t data64 =cpu_to_be64 (new_block);BLKDBG_EVENT (bs->file,BLKDBG_REFBLOCK_ALLOC_HOOKUP ); - ret =bdrv_pwrite (bs->file, + ret =bdrv_pwrite_sync (bs->file, s->refcount_table_offset + refcount_table_index *sizeof (uint64_t ), &data64,sizeof (data64));if (ret <0 ) {
@@ -359,7 +359,7 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)/* Write refcount blocks to disk */ BLKDBG_EVENT (bs->file,BLKDBG_REFBLOCK_ALLOC_WRITE_BLOCKS ); - ret =bdrv_pwrite (bs->file, meta_offset, new_blocks, + ret =bdrv_pwrite_sync (bs->file, meta_offset, new_blocks, blocks_clusters * s->cluster_size);qemu_free (new_blocks);if (ret <0 ) {
@@ -372,7 +372,7 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index) }BLKDBG_EVENT (bs->file,BLKDBG_REFBLOCK_ALLOC_WRITE_TABLE ); - ret =bdrv_pwrite (bs->file, table_offset, new_table, + ret =bdrv_pwrite_sync (bs->file, table_offset, new_table, table_size *sizeof (uint64_t ));if (ret <0 ) { goto fail_table;
@@ -387,7 +387,7 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index)cpu_to_be64w ((uint64_t *)data, table_offset);cpu_to_be32w ((uint32_t *)(data +8 ), table_clusters);BLKDBG_EVENT (bs->file,BLKDBG_REFBLOCK_ALLOC_SWITCH_TABLE ); - ret =bdrv_pwrite (bs->file, offsetof(QCowHeader , refcount_table_offset), + ret =bdrv_pwrite_sync (bs->file, offsetof(QCowHeader , refcount_table_offset), data,sizeof (data));if (ret <0 ) { goto fail_table;
@@ -444,7 +444,7 @@ static int write_refcount_block_entries(BlockDriverState *bs, size = (last_index - first_index) <<REFCOUNT_SHIFT ;BLKDBG_EVENT (bs->file,BLKDBG_REFBLOCK_UPDATE_PART ); - ret =bdrv_pwrite (bs->file, + ret =bdrv_pwrite_sync (bs->file, refcount_block_offset + (first_index <<REFCOUNT_SHIFT ), &s->refcount_block_cache[first_index], size);if (ret <0 ) {
@@ -826,7 +826,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, } }if (l2_modified) { -if (bdrv_pwrite (bs->file, +if (bdrv_pwrite_sync (bs->file, l2_offset, l2_table, l2_size) != l2_size) goto fail; }
@@ -850,7 +850,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,if (l1_modified) {for (i =0 ; i < l1_size; i++)cpu_to_be64s (&l1_table[i]); -if (bdrv_pwrite (bs->file, l1_table_offset, l1_table, +if (bdrv_pwrite_sync (bs->file, l1_table_offset, l1_table, l1_size2) != l1_size2) goto fail;for (i =0 ; i < l1_size; i++)
block/qcow2-snapshot.c +7 -7
@@ -158,24 +158,24 @@ static int qcow_write_snapshots(BlockDriverState *bs) h.id_str_size =cpu_to_be16 (id_str_size); h.name_size =cpu_to_be16 (name_size); offset =align_offset (offset,8 ); -if (bdrv_pwrite (bs->file, offset, &h,sizeof (h)) !=sizeof (h)) +if (bdrv_pwrite_sync (bs->file, offset, &h,sizeof (h)) !=sizeof (h)) goto fail; offset +=sizeof (h); -if (bdrv_pwrite (bs->file, offset, sn->id_str, id_str_size) != id_str_size) +if (bdrv_pwrite_sync (bs->file, offset, sn->id_str, id_str_size) != id_str_size) goto fail; offset += id_str_size; -if (bdrv_pwrite (bs->file, offset, sn->name, name_size) != name_size) +if (bdrv_pwrite_sync (bs->file, offset, sn->name, name_size) != name_size) goto fail; offset += name_size; }/* update the various header fields */ data64 =cpu_to_be64 (snapshots_offset); -if (bdrv_pwrite (bs->file, offsetof(QCowHeader , snapshots_offset), +if (bdrv_pwrite_sync (bs->file, offsetof(QCowHeader , snapshots_offset), &data64,sizeof (data64)) !=sizeof (data64)) goto fail; data32 =cpu_to_be32 (s->nb_snapshots); -if (bdrv_pwrite (bs->file, offsetof(QCowHeader , nb_snapshots), +if (bdrv_pwrite_sync (bs->file, offsetof(QCowHeader , nb_snapshots), &data32,sizeof (data32)) !=sizeof (data32)) goto fail;
@@ -284,7 +284,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)for (i =0 ; i < s->l1_size; i++) { l1_table[i] =cpu_to_be64 (s->l1_table[i]); } -if (bdrv_pwrite (bs->file, sn->l1_table_offset, +if (bdrv_pwrite_sync (bs->file, sn->l1_table_offset, l1_table, s->l1_size *sizeof (uint64_t )) != (s->l1_size *sizeof (uint64_t ))) goto fail;
@@ -335,7 +335,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)if (bdrv_pread (bs->file, sn->l1_table_offset, s->l1_table, l1_size2) != l1_size2) goto fail; -if (bdrv_pwrite (bs->file, s->l1_table_offset, +if (bdrv_pwrite_sync (bs->file, s->l1_table_offset, s->l1_table, l1_size2) != l1_size2) goto fail;for (i =0 ;i < s->l1_size; i++) {
block/qcow2.c +5 -5
@@ -736,7 +736,7 @@ static int qcow2_update_ext_header(BlockDriverState *bs, backing_file_offset =sizeof (QCowHeader) + offset; } - ret =bdrv_pwrite (bs->file,sizeof (QCowHeader), buf, ext_size); + ret =bdrv_pwrite_sync (bs->file,sizeof (QCowHeader), buf, ext_size);if (ret <0 ) { goto fail; }
@@ -745,13 +745,13 @@ static int qcow2_update_ext_header(BlockDriverState *bs,uint64_t be_backing_file_offset =cpu_to_be64 (backing_file_offset);uint32_t be_backing_file_size =cpu_to_be32 (backing_file_len); - ret =bdrv_pwrite (bs->file, offsetof(QCowHeader , backing_file_offset), + ret =bdrv_pwrite_sync (bs->file, offsetof(QCowHeader , backing_file_offset), &be_backing_file_offset,sizeof (uint64_t ));if (ret <0 ) { goto fail; } - ret =bdrv_pwrite (bs->file, offsetof(QCowHeader , backing_file_size), + ret =bdrv_pwrite_sync (bs->file, offsetof(QCowHeader , backing_file_size), &be_backing_file_size,sizeof (uint32_t ));if (ret <0 ) { goto fail;
@@ -1035,8 +1035,8 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)/* write updated header.size */ offset =cpu_to_be64 (offset); - ret =bdrv_pwrite (bs->file, offsetof(QCowHeader , size), - &offset,sizeof (uint64_t )); + ret =bdrv_pwrite_sync (bs->file, offsetof(QCowHeader , size), + &offset,sizeof (uint64_t ));if (ret <0 ) {return ret; }
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [RFC][PATCH 2/2] qcow2: Use bdrv_(p)write_sync for metadata writes
Quoted 1 line · Kevin Wolf
Use bdrv_(p)write_sync to ensure metadata integrity in case of a crash.
Any performance numbers? This change is necessary for correctness but I wonder what the performance impact is for users.
Stefan
[Qemu-devel] Re: [RFC][PATCH 2/2] qcow2: Use bdrv_(p)write_sync for metadata writes
Am 17.06.2010 16:19, schrieb Stefan Hajnoczi:
Quoted 5 lines · Kevin Wolf
Quoted 1 line
Use bdrv_(p)write_sync to ensure metadata integrity in case of a crash.
Any performance numbers? This change is necessary for correctness but I wonder what the performance impact is for users.
No numbers yet, but as you say we need to do it anyway. It should definitely be better than any other option that I can think of (cache=writethrough or some O_DIRECT|O_DSYNC mode) in that it only hurts performance when metadata is actually changed. As long as we only write guest data, there is no difference.
Making it a barrier instead of a flush would probably be better, have you already had a look at this since we talked about it?
Kevin
[Qemu-devel] Re: [RFC][PATCH 2/2] qcow2: Use bdrv_(p)write_sync for metadata writes
Quoted 7 lines · Kevin Wolf
Am 17.06.2010 16:19, schrieb Stefan Hajnoczi:
Quoted 5 lines · Kevin Wolf
Quoted 1 line
Use bdrv_(p)write_sync to ensure metadata integrity in case of a crash.
Any performance numbers? This change is necessary for correctness but I wonder what the performance impact is for users.
Quoted 8 lines
No numbers yet, but as you say we need to do it anyway. It should definitely be better than any other option that I can think of (cache=writethrough or some O_DIRECT|O_DSYNC mode) in that it only hurts performance when metadata is actually changed. As long as we only write guest data, there is no difference.
Making it a barrier instead of a flush would probably be better, have you already had a look at this since we talked about it?
No I haven't seen a userspace barrier solution. I'm not actively working on that at the moment but am keeping an eye out for solutions.
I'm not sure what to make of sync_file_range(2), the man page
suggests it cannot be relied on since only already allocated blocks will be flushed appropriately. Filesystem metadata changes are not flushed, which is especially problematic for copy-on-write filesystems (according to the man page).
Thinking about the performance more, I predict that guest OS installations will slow down but day-to-day usage (especially modifying already allocated blocks) will be alright for the reasons you mentioned.
Stefan
[Qemu-devel] Re: [RFC][PATCH 2/2] qcow2: Use bdrv_(p)write_sync for metadata writes
Am 17.06.2010 21:47, schrieb Stefan Hajnoczi:
Quoted 17 lines · Kevin Wolf
Quoted 7 lines
Am 17.06.2010 16:19, schrieb Stefan Hajnoczi:
Quoted 5 lines · Kevin Wolf
Quoted 1 line
Use bdrv_(p)write_sync to ensure metadata integrity in case of a crash.
Any performance numbers? This change is necessary for correctness but I wonder what the performance impact is for users.
Quoted 8 lines
No numbers yet, but as you say we need to do it anyway. It should definitely be better than any other option that I can think of (cache=writethrough or some O_DIRECT|O_DSYNC mode) in that it only hurts performance when metadata is actually changed. As long as we only write guest data, there is no difference.
Making it a barrier instead of a flush would probably be better, have you already had a look at this since we talked about it?
Quoted 7 lines
No I haven't seen a userspace barrier solution. I'm not actively working on that at the moment but am keeping an eye out for solutions.
I'm not sure what to make ofsync_file_range(2), the man page
suggests it cannot be relied on since only already allocated blocks will be flushed appropriately. Filesystem metadata changes are not flushed, which is especially problematic for copy-on-write filesystems (according to the man page).
qcow2 files are expected to grow, so this probably doesn't help us. At best we could try to detect if the file grows and decide if to use sync_file_range or fdatasync.
Of course, it would be better to have a flag or something to include the necessary metadata, but I have no idea how it's implemented and if this is easily possible.
Quoted 4 lines
Thinking about the performance more, I predict that guest OS installations will slow down but day-to-day usage (especially modifying already allocated blocks) will be alright for the reasons you mentioned.
Right, it's only growth that will get an impact. The pooint is that it's not only installation (in most cases you could even use cache=unsafe for that) but that you start to grow again after taking a snapshot - be it internal (savevm) or external (backing file). This is where it hurts.
Kevin
[Qemu-devel] Re: [RFC][PATCH 2/2] qcow2: Use bdrv_(p)write_sync for metadata writes
Quoted 3 lines · Kevin Wolf
qcow2 files are expected to grow, so this probably doesn't help us. At best we could try to detect if the file grows and decide if to use
sync_file_rangeor fdatasync.
No. sync_file_range is always unsafe for use with a real filesystem. It never even calls into the filesystem and never flushes the disk write cache. For our use case it's exactly a no-op.
Quoted 3 lines
Of course, it would be better to have a flag or something to include the necessary metadata, but I have no idea how it's implemented and if this is easily possible.
We could add a flag to it. But rather than adding more crap to this bastard that should never have been added I'd rather add a real fsync_range system call.
Note that for our use case it doesn't matter anyway. For an image file accessed with cache=none there will be no data in the pagecache, and the writeback of the inode and associated metadata (btree blocks or indirect blocks) will be same wether we give it a range or not, and same for the disk cache flush.