* [pbs-devel] [PATCH 0/3] Fix PBS blockdriver for non-VM settings @ 2020-07-20 15:02 Stefan Reiter 2020-07-20 15:02 ` [pbs-devel] [PATCH backup 1/3] make BufferedFixedReader async Stefan Reiter ` (2 more replies) 0 siblings, 3 replies; 5+ messages in thread From: Stefan Reiter @ 2020-07-20 15:02 UTC (permalink / raw) To: pbs-devel When using the PBS blockdriver with qemu-nbd (for example), it can happen that enough read requests are issued to saturate the tokio thread pool. Not an issue in general, but as me and Wolfgang painstakenly discovered a while back, it does break block_on, which is used in BufferedFixedReader. This means that reading larger amounts of data would hang the QEMU process [0]. Fix this by making the entire BufferedFixedReader API async, thus not requiring a block_on. Additionally I discovered a seperate bug (fixed by patch 3), wherein read requests that we're not aligned to the chunk size would return bogus data. This too only seems to happen in non-VM connections (e.g. nbd, etc...). [0] ...and since the NBD kernel driver appears to be horribly broken, this often also crashes most of the system, but that's a different story. If you ever get in this situation, 'nbd-client -d /dev/nbdX' works (sometimes) to force disconnect the device ('qemu-nbd -d' intelligently issues a read before disconnecting, thus hanging before getting anything done...) backup: Stefan Reiter (1): make BufferedFixedReader async src/backup/fixed_index.rs | 145 ++++++++++---------------------------- 1 file changed, 37 insertions(+), 108 deletions(-) backup-qemu: Stefan Reiter (1): use new async BufferedFixedReader API src/restore.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) qemu: Stefan Reiter (1): PVE: PBS: iterate read_image_at until all data is available block/pbs.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [pbs-devel] [PATCH backup 1/3] make BufferedFixedReader async 2020-07-20 15:02 [pbs-devel] [PATCH 0/3] Fix PBS blockdriver for non-VM settings Stefan Reiter @ 2020-07-20 15:02 ` Stefan Reiter 2020-07-21 9:37 ` Dominik Csapak 2020-07-20 15:02 ` [pbs-devel] [PATCH backup-qemu 2/3] use new async BufferedFixedReader API Stefan Reiter 2020-07-20 15:02 ` [pbs-devel] [PATCH qemu 3/3] PVE: PBS: iterate read_image_at until all data is available Stefan Reiter 2 siblings, 1 reply; 5+ messages in thread From: Stefan Reiter @ 2020-07-20 15:02 UTC (permalink / raw) To: pbs-devel It's currently only used in proxmox-backup-qemu, so no users in this package need to be changed. This also allows simplifying the interface to just what is needed in proxmox-backup-qemu, i.e. by making it async we need to remove the std::io::Read trait, and that allows to get rid of the seeking workaround. Cache locking is done with a reader-writer lock, though multiple requests for one non-cached chunk are still possible (cannot hold a lock during async HTTP request) - though in testing, performance did not regress. The "sequential read" optimization is removed, since it didn't serve any purpose AFAICT. chunk_end was not used anywhere else (gave a warning). Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> --- I'm unsure about the approach here, happy to hear feedback from anyone more familiar with Rust. I couldn't find a way to combine the "read" trait impl with async, so I removed it entirely. Since proxmox-backup-qemu seems to be the only user I don't think this is an issue, but I don't know if this interface was put here deliberately, even if unused elsewhere. src/backup/fixed_index.rs | 145 ++++++++++---------------------------- 1 file changed, 37 insertions(+), 108 deletions(-) diff --git a/src/backup/fixed_index.rs b/src/backup/fixed_index.rs index 73d0dad0..3f5fde2a 100644 --- a/src/backup/fixed_index.rs +++ b/src/backup/fixed_index.rs @@ -1,5 +1,6 @@ use anyhow::{bail, format_err, Error}; use std::io::{Seek, SeekFrom}; +use std::cmp::min; use super::chunk_stat::*; use super::chunk_store::*; @@ -11,7 +12,7 @@ use std::fs::File; use std::io::Write; use std::os::unix::io::AsRawFd; use std::path::{Path, PathBuf}; -use std::sync::Arc; +use std::sync::{Arc,RwLock}; use super::read_chunk::*; use super::ChunkInfo; @@ -146,20 +147,6 @@ impl FixedIndexReader { Ok(()) } - #[inline] - fn chunk_end(&self, pos: usize) -> u64 { - if pos >= self.index_length { - panic!("chunk index out of range"); - } - - let end = ((pos + 1) * self.chunk_size) as u64; - if end > self.size { - self.size - } else { - end - } - } - pub fn print_info(&self) { println!("Size: {}", self.size); println!("ChunkSize: {}", self.chunk_size); @@ -466,27 +453,29 @@ impl FixedIndexWriter { } } +struct ChunkBuffer { + data: Vec<u8>, + chunk_idx: usize, +} + pub struct BufferedFixedReader<S> { store: S, index: FixedIndexReader, archive_size: u64, - read_buffer: Vec<u8>, - buffered_chunk_idx: usize, - buffered_chunk_start: u64, - read_offset: u64, + buffer: RwLock<ChunkBuffer>, } -impl<S: ReadChunk> BufferedFixedReader<S> { +impl<S: AsyncReadChunk> BufferedFixedReader<S> { pub fn new(index: FixedIndexReader, store: S) -> Self { let archive_size = index.size; Self { store, index, archive_size, - read_buffer: Vec::with_capacity(1024 * 1024), - buffered_chunk_idx: 0, - buffered_chunk_start: 0, - read_offset: 0, + buffer: RwLock::new(ChunkBuffer { + data: Vec::with_capacity(1024 * 1024 * 4), + chunk_idx: 0, + }), } } @@ -494,7 +483,7 @@ impl<S: ReadChunk> BufferedFixedReader<S> { self.archive_size } - fn buffer_chunk(&mut self, idx: usize) -> Result<(), Error> { + async fn buffer_chunk(&self, idx: usize) -> Result<Vec<u8>, Error> { let index = &self.index; let info = match index.chunk_info(idx) { Some(info) => info, @@ -503,104 +492,44 @@ impl<S: ReadChunk> BufferedFixedReader<S> { // fixme: avoid copy - let data = self.store.read_chunk(&info.digest)?; + let data = self.store.read_chunk(&info.digest).await?; let size = info.range.end - info.range.start; if size != data.len() as u64 { bail!("read chunk with wrong size ({} != {}", size, data.len()); } - self.read_buffer.clear(); - self.read_buffer.extend_from_slice(&data); + let mut buffer = self.buffer.write().unwrap(); + buffer.data.clear(); + buffer.data.extend_from_slice(&data); + buffer.chunk_idx = idx; - self.buffered_chunk_idx = idx; - - self.buffered_chunk_start = info.range.start as u64; - Ok(()) + Ok(data) } -} -impl<S: ReadChunk> crate::tools::BufferedRead for BufferedFixedReader<S> { - fn buffered_read(&mut self, offset: u64) -> Result<&[u8], Error> { + pub async fn async_buffered_read(&self, buf: &mut [u8], size: usize, offset: u64) -> Result<u64, Error> { if offset == self.archive_size { - return Ok(&self.read_buffer[0..0]); + return Ok(0); } - let buffer_len = self.read_buffer.len(); - let index = &self.index; + let idx = (offset / self.index.chunk_size as u64) as usize; + + let mut copy_to_buf = move |from: &Vec<u8>| -> u64 { + let buffer_offset = (offset % self.index.chunk_size as u64) as usize; + let data_len = min(from.len() - buffer_offset, size); + unsafe { + std::ptr::copy_nonoverlapping(from.as_ptr().add(buffer_offset), buf.as_mut_ptr(), data_len); + } + data_len as _ + }; - // optimization for sequential read - if buffer_len > 0 - && ((self.buffered_chunk_idx + 1) < index.index_length) - && (offset >= (self.buffered_chunk_start + (self.read_buffer.len() as u64))) { - let next_idx = self.buffered_chunk_idx + 1; - let next_end = index.chunk_end(next_idx); - if offset < next_end { - self.buffer_chunk(next_idx)?; - let buffer_offset = (offset - self.buffered_chunk_start) as usize; - return Ok(&self.read_buffer[buffer_offset..]); + let buffer = self.buffer.read().unwrap(); + if buffer.data.len() != 0 && buffer.chunk_idx == idx { + return Ok(copy_to_buf(&buffer.data)); } } - if (buffer_len == 0) - || (offset < self.buffered_chunk_start) - || (offset >= (self.buffered_chunk_start + (self.read_buffer.len() as u64))) - { - let idx = (offset / index.chunk_size as u64) as usize; - self.buffer_chunk(idx)?; - } - - let buffer_offset = (offset - self.buffered_chunk_start) as usize; - Ok(&self.read_buffer[buffer_offset..]) - } -} - -impl<S: ReadChunk> std::io::Read for BufferedFixedReader<S> { - fn read(&mut self, buf: &mut [u8]) -> Result<usize, std::io::Error> { - use crate::tools::BufferedRead; - use std::io::{Error, ErrorKind}; - - let data = match self.buffered_read(self.read_offset) { - Ok(v) => v, - Err(err) => return Err(Error::new(ErrorKind::Other, err.to_string())), - }; - - let n = if data.len() > buf.len() { - buf.len() - } else { - data.len() - }; - - unsafe { - std::ptr::copy_nonoverlapping(data.as_ptr(), buf.as_mut_ptr(), n); - } - - self.read_offset += n as u64; - - Ok(n) - } -} - -impl<S: ReadChunk> Seek for BufferedFixedReader<S> { - fn seek(&mut self, pos: SeekFrom) -> Result<u64, std::io::Error> { - let new_offset = match pos { - SeekFrom::Start(start_offset) => start_offset as i64, - SeekFrom::End(end_offset) => (self.archive_size as i64) + end_offset, - SeekFrom::Current(offset) => (self.read_offset as i64) + offset, - }; - - use std::io::{Error, ErrorKind}; - if (new_offset < 0) || (new_offset > (self.archive_size as i64)) { - return Err(Error::new( - ErrorKind::Other, - format!( - "seek is out of range {} ([0..{}])", - new_offset, self.archive_size - ), - )); - } - self.read_offset = new_offset as u64; - - Ok(self.read_offset) + let new_data = self.buffer_chunk(idx).await?; + Ok(copy_to_buf(&new_data)) } } -- 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pbs-devel] [PATCH backup 1/3] make BufferedFixedReader async 2020-07-20 15:02 ` [pbs-devel] [PATCH backup 1/3] make BufferedFixedReader async Stefan Reiter @ 2020-07-21 9:37 ` Dominik Csapak 0 siblings, 0 replies; 5+ messages in thread From: Dominik Csapak @ 2020-07-21 9:37 UTC (permalink / raw) To: pbs-devel On 7/20/20 5:02 PM, Stefan Reiter wrote: > It's currently only used in proxmox-backup-qemu, so no users in this > package need to be changed. > > This also allows simplifying the interface to just what is needed in > proxmox-backup-qemu, i.e. by making it async we need to remove the > std::io::Read trait, and that allows to get rid of the seeking > workaround. > > Cache locking is done with a reader-writer lock, though multiple > requests for one non-cached chunk are still possible (cannot hold a lock > during async HTTP request) - though in testing, performance did not > regress. > > The "sequential read" optimization is removed, since it didn't serve any > purpose AFAICT. chunk_end was not used anywhere else (gave a warning). > > Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> > --- > > I'm unsure about the approach here, happy to hear feedback from anyone more > familiar with Rust. I couldn't find a way to combine the "read" trait impl with > async, so I removed it entirely. Since proxmox-backup-qemu seems to be the only > user I don't think this is an issue, but I don't know if this interface was put > here deliberately, even if unused elsewhere. high level comment: maybe it would be nicer to implement AsyncSeek[0] for our existing AsyncIndexReader (wich also buffers a single chunk, but works for fixed + dynamic index). then we could simply keep the behaviour we have in proxmox-backup-qemu (just add '.await') for implementing this, we would have to extend the Index trait to provide a function fn get_chunk_from_offset(offset) -> (chunk_idx, offset_in_chunk) since a 'seek' would then be 'instant' (at least there is nothing we could await), we would just have a 'psuedo-async' interface to implement it would then 'magically' work also for a dynamic index alternatively, we could also implement the 'async_buffered_read' simply for our AsyncIndexReader also and still drop this here entirely more comments inline 0: https://docs.rs/tokio/0.2.9/tokio/io/trait.AsyncSeek.html > > src/backup/fixed_index.rs | 145 ++++++++++---------------------------- > 1 file changed, 37 insertions(+), 108 deletions(-) > > diff --git a/src/backup/fixed_index.rs b/src/backup/fixed_index.rs > index 73d0dad0..3f5fde2a 100644 > --- a/src/backup/fixed_index.rs > +++ b/src/backup/fixed_index.rs > @@ -1,5 +1,6 @@ > use anyhow::{bail, format_err, Error}; > use std::io::{Seek, SeekFrom}; > +use std::cmp::min; no need to use this see comment further below > > use super::chunk_stat::*; > use super::chunk_store::*; > @@ -11,7 +12,7 @@ use std::fs::File; > use std::io::Write; > use std::os::unix::io::AsRawFd; > use std::path::{Path, PathBuf}; > -use std::sync::Arc; > +use std::sync::{Arc,RwLock}; > > use super::read_chunk::*; > use super::ChunkInfo; > @@ -146,20 +147,6 @@ impl FixedIndexReader { > Ok(()) > } > > - #[inline] > - fn chunk_end(&self, pos: usize) -> u64 { > - if pos >= self.index_length { > - panic!("chunk index out of range"); > - } > - > - let end = ((pos + 1) * self.chunk_size) as u64; > - if end > self.size { > - self.size > - } else { > - end > - } > - } > - > pub fn print_info(&self) { > println!("Size: {}", self.size); > println!("ChunkSize: {}", self.chunk_size); > @@ -466,27 +453,29 @@ impl FixedIndexWriter { > } > } > > +struct ChunkBuffer { > + data: Vec<u8>, > + chunk_idx: usize, > +} > + > pub struct BufferedFixedReader<S> { > store: S, > index: FixedIndexReader, > archive_size: u64, > - read_buffer: Vec<u8>, > - buffered_chunk_idx: usize, > - buffered_chunk_start: u64, > - read_offset: u64, > + buffer: RwLock<ChunkBuffer>, > } > > -impl<S: ReadChunk> BufferedFixedReader<S> { > +impl<S: AsyncReadChunk> BufferedFixedReader<S> { > pub fn new(index: FixedIndexReader, store: S) -> Self { > let archive_size = index.size; > Self { > store, > index, > archive_size, > - read_buffer: Vec::with_capacity(1024 * 1024), > - buffered_chunk_idx: 0, > - buffered_chunk_start: 0, > - read_offset: 0, > + buffer: RwLock::new(ChunkBuffer { > + data: Vec::with_capacity(1024 * 1024 * 4), > + chunk_idx: 0, > + }), > } > } > > @@ -494,7 +483,7 @@ impl<S: ReadChunk> BufferedFixedReader<S> { > self.archive_size > } > > - fn buffer_chunk(&mut self, idx: usize) -> Result<(), Error> { > + async fn buffer_chunk(&self, idx: usize) -> Result<Vec<u8>, Error> { > let index = &self.index; > let info = match index.chunk_info(idx) { > Some(info) => info, > @@ -503,104 +492,44 @@ impl<S: ReadChunk> BufferedFixedReader<S> { > > // fixme: avoid copy > > - let data = self.store.read_chunk(&info.digest)?; > + let data = self.store.read_chunk(&info.digest).await?; > let size = info.range.end - info.range.start; > if size != data.len() as u64 { > bail!("read chunk with wrong size ({} != {}", size, data.len()); > } > > - self.read_buffer.clear(); > - self.read_buffer.extend_from_slice(&data); > + let mut buffer = self.buffer.write().unwrap(); > + buffer.data.clear(); > + buffer.data.extend_from_slice(&data); > + buffer.chunk_idx = idx; > > - self.buffered_chunk_idx = idx; > - > - self.buffered_chunk_start = info.range.start as u64; > - Ok(()) > + Ok(data) > } > -} > > -impl<S: ReadChunk> crate::tools::BufferedRead for BufferedFixedReader<S> { > - fn buffered_read(&mut self, offset: u64) -> Result<&[u8], Error> { > + pub async fn async_buffered_read(&self, buf: &mut [u8], size: usize, offset: u64) -> Result<u64, Error> { > if offset == self.archive_size { > - return Ok(&self.read_buffer[0..0]); > + return Ok(0); > } > > - let buffer_len = self.read_buffer.len(); > - let index = &self.index; > + let idx = (offset / self.index.chunk_size as u64) as usize; > + > + let mut copy_to_buf = move |from: &Vec<u8>| -> u64 { > + let buffer_offset = (offset % self.index.chunk_size as u64) as usize; > + let data_len = min(from.len() - buffer_offset, size); instead of using std::cmp::min, you can simply use min on a number: let data_len = size.min(from.len() - buffer_offset); > + unsafe { > + std::ptr::copy_nonoverlapping(from.as_ptr().add(buffer_offset), buf.as_mut_ptr(), data_len); > + } here you can avoid unsafe code by writing: buf[0..data_len].copy_from_slice(from[buffer_offset..buffer_offset+data_len]); (does internally exactly the same, but checks the lengths and panics in error case instead of corrupting memory/segfaulting) > + data_len as _ > + }; > > - // optimization for sequential read > - if buffer_len > 0 > - && ((self.buffered_chunk_idx + 1) < index.index_length) > - && (offset >= (self.buffered_chunk_start + (self.read_buffer.len() as u64))) > { > - let next_idx = self.buffered_chunk_idx + 1; > - let next_end = index.chunk_end(next_idx); > - if offset < next_end { > - self.buffer_chunk(next_idx)?; > - let buffer_offset = (offset - self.buffered_chunk_start) as usize; > - return Ok(&self.read_buffer[buffer_offset..]); > + let buffer = self.buffer.read().unwrap(); > + if buffer.data.len() != 0 && buffer.chunk_idx == idx { > + return Ok(copy_to_buf(&buffer.data)); > } > } > > - if (buffer_len == 0) > - || (offset < self.buffered_chunk_start) > - || (offset >= (self.buffered_chunk_start + (self.read_buffer.len() as u64))) > - { > - let idx = (offset / index.chunk_size as u64) as usize; > - self.buffer_chunk(idx)?; > - } > - > - let buffer_offset = (offset - self.buffered_chunk_start) as usize; > - Ok(&self.read_buffer[buffer_offset..]) > - } > -} > - > -impl<S: ReadChunk> std::io::Read for BufferedFixedReader<S> { > - fn read(&mut self, buf: &mut [u8]) -> Result<usize, std::io::Error> { > - use crate::tools::BufferedRead; > - use std::io::{Error, ErrorKind}; > - > - let data = match self.buffered_read(self.read_offset) { > - Ok(v) => v, > - Err(err) => return Err(Error::new(ErrorKind::Other, err.to_string())), > - }; > - > - let n = if data.len() > buf.len() { > - buf.len() > - } else { > - data.len() > - }; > - > - unsafe { > - std::ptr::copy_nonoverlapping(data.as_ptr(), buf.as_mut_ptr(), n); > - } > - > - self.read_offset += n as u64; > - > - Ok(n) > - } > -} > - > -impl<S: ReadChunk> Seek for BufferedFixedReader<S> { > - fn seek(&mut self, pos: SeekFrom) -> Result<u64, std::io::Error> { > - let new_offset = match pos { > - SeekFrom::Start(start_offset) => start_offset as i64, > - SeekFrom::End(end_offset) => (self.archive_size as i64) + end_offset, > - SeekFrom::Current(offset) => (self.read_offset as i64) + offset, > - }; > - > - use std::io::{Error, ErrorKind}; > - if (new_offset < 0) || (new_offset > (self.archive_size as i64)) { > - return Err(Error::new( > - ErrorKind::Other, > - format!( > - "seek is out of range {} ([0..{}])", > - new_offset, self.archive_size > - ), > - )); > - } > - self.read_offset = new_offset as u64; > - > - Ok(self.read_offset) > + let new_data = self.buffer_chunk(idx).await?; > + Ok(copy_to_buf(&new_data)) > } > } > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [pbs-devel] [PATCH backup-qemu 2/3] use new async BufferedFixedReader API 2020-07-20 15:02 [pbs-devel] [PATCH 0/3] Fix PBS blockdriver for non-VM settings Stefan Reiter 2020-07-20 15:02 ` [pbs-devel] [PATCH backup 1/3] make BufferedFixedReader async Stefan Reiter @ 2020-07-20 15:02 ` Stefan Reiter 2020-07-20 15:02 ` [pbs-devel] [PATCH qemu 3/3] PVE: PBS: iterate read_image_at until all data is available Stefan Reiter 2 siblings, 0 replies; 5+ messages in thread From: Stefan Reiter @ 2020-07-20 15:02 UTC (permalink / raw) To: pbs-devel Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> --- Requires dependency bump on proxmox-backup, and while at it maybe the proxmox crate to 0.2 as well? (no issues in my testing, I've been building with 0.2.0 for a while now, since it's the one in the repos). src/restore.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/restore.rs b/src/restore.rs index 3e37066..9995743 100644 --- a/src/restore.rs +++ b/src/restore.rs @@ -1,13 +1,12 @@ use std::sync::{Arc, Mutex}; use std::collections::HashMap; -use std::io::{Read, Seek, SeekFrom}; use std::convert::TryInto; use anyhow::{format_err, bail, Error}; use once_cell::sync::OnceCell; use tokio::runtime::Runtime; -use proxmox_backup::tools::runtime::{get_runtime_with_builder, block_in_place}; +use proxmox_backup::tools::runtime::get_runtime_with_builder; use proxmox_backup::backup::*; use proxmox_backup::client::{HttpClient, HttpClientOptions, BackupReader, RemoteChunkReader}; @@ -16,7 +15,7 @@ use crate::registry::Registry; use crate::capi_types::DataPointer; struct ImageAccessInfo { - reader: Arc<Mutex<BufferedFixedReader<RemoteChunkReader>>>, + reader: Arc<BufferedFixedReader<RemoteChunkReader>>, _archive_name: String, archive_size: u64, } @@ -236,7 +235,7 @@ impl RestoreTask { let info = ImageAccessInfo { archive_size, _archive_name: archive_name, /// useful to debug - reader: Arc::new(Mutex::new(reader)), + reader: Arc::new(reader), }; (*self.image_registry.lock().unwrap()).register(info) @@ -260,12 +259,9 @@ impl RestoreTask { bail!("read index {} out of bounds {}", offset, image_size); } - let bytes = block_in_place(|| { - let mut reader = reader.lock().unwrap(); - reader.seek(SeekFrom::Start(offset))?; - let buf: &mut [u8] = unsafe { std::slice::from_raw_parts_mut(data.0 as *mut u8, size as usize)}; - reader.read(buf) - })?; + let size = size as usize; + let buf: &mut [u8] = unsafe { std::slice::from_raw_parts_mut(data.0 as *mut u8, size)}; + let bytes = reader.async_buffered_read(buf, size, offset).await?; Ok(bytes.try_into()?) } -- 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [pbs-devel] [PATCH qemu 3/3] PVE: PBS: iterate read_image_at until all data is available 2020-07-20 15:02 [pbs-devel] [PATCH 0/3] Fix PBS blockdriver for non-VM settings Stefan Reiter 2020-07-20 15:02 ` [pbs-devel] [PATCH backup 1/3] make BufferedFixedReader async Stefan Reiter 2020-07-20 15:02 ` [pbs-devel] [PATCH backup-qemu 2/3] use new async BufferedFixedReader API Stefan Reiter @ 2020-07-20 15:02 ` Stefan Reiter 2 siblings, 0 replies; 5+ messages in thread From: Stefan Reiter @ 2020-07-20 15:02 UTC (permalink / raw) To: pbs-devel Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> --- Sent as seperate patch for review, but can also be squashed into the PBS bdrv patch easily. block/pbs.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/block/pbs.c b/block/pbs.c index 1481a2bfd1..0083406ad9 100644 --- a/block/pbs.c +++ b/block/pbs.c @@ -198,7 +198,7 @@ static coroutine_fn int pbs_co_preadv(BlockDriverState *bs, QEMUIOVector *qiov, int flags) { BDRVPBSState *s = bs->opaque; - int ret; + int ret, cur = 0; char *pbs_error = NULL; uint8_t *buf = malloc(bytes); @@ -207,18 +207,31 @@ static coroutine_fn int pbs_co_preadv(BlockDriverState *bs, .ctx = qemu_get_current_aio_context(), }; - proxmox_restore_read_image_at_async(s->conn, s->aid, buf, offset, bytes, - read_callback, (void *) &rcb, &ret, &pbs_error); + /* we need to retry until we have either read all requested data or hit the + * end of the file - QEMU does not re-call this function in case the + * returned value is not equal to 'bytes', it just assumes there is no data + * to be read, which breaks unaligned reads */ + while (cur < bytes) { + proxmox_restore_read_image_at_async(s->conn, s->aid, buf + cur, offset + cur, bytes - cur, + read_callback, (void *) &rcb, &ret, &pbs_error); + qemu_coroutine_yield(); - qemu_coroutine_yield(); + if (ret < 0) { + fprintf(stderr, "error during PBS read: %s\n", pbs_error ? pbs_error : "unknown error"); + if (pbs_error) proxmox_backup_free_error(pbs_error); + free(buf); + return -EIO; + } - if (ret < 0) { - fprintf(stderr, "error during PBS read: %s\n", pbs_error ? pbs_error : "unknown error"); - if (pbs_error) proxmox_backup_free_error(pbs_error); - return -EIO; + // EOF + if (ret == 0) { + break; + } + + cur += ret; } - qemu_iovec_from_buf(qiov, 0, buf, bytes); + qemu_iovec_from_buf(qiov, 0, buf, cur); free(buf); return ret; -- 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-21 9:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-20 15:02 [pbs-devel] [PATCH 0/3] Fix PBS blockdriver for non-VM settings Stefan Reiter 2020-07-20 15:02 ` [pbs-devel] [PATCH backup 1/3] make BufferedFixedReader async Stefan Reiter 2020-07-21 9:37 ` Dominik Csapak 2020-07-20 15:02 ` [pbs-devel] [PATCH backup-qemu 2/3] use new async BufferedFixedReader API Stefan Reiter 2020-07-20 15:02 ` [pbs-devel] [PATCH qemu 3/3] PVE: PBS: iterate read_image_at until all data is available Stefan Reiter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox