* [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact
@ 2023-06-29 10:32 Fabian Grünbichler
2023-06-29 11:03 ` Dominik Csapak
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Fabian Grünbichler @ 2023-06-29 10:32 UTC (permalink / raw)
To: pbs-devel
since read_exact does not support short reads, which can easily happen if the
mapped image's EOF is not aligned with the request size.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes:
reported on the forum:
https://forum.proxmox.com/threads/problem-backing-up-using-backup-client.129347
did a quick test reading from a mapped image full of random data, observed
no performance difference..
pbs-fuse-loop/src/fuse_loop.rs | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/pbs-fuse-loop/src/fuse_loop.rs b/pbs-fuse-loop/src/fuse_loop.rs
index 3d0ef123..7e780799 100644
--- a/pbs-fuse-loop/src/fuse_loop.rs
+++ b/pbs-fuse-loop/src/fuse_loop.rs
@@ -188,13 +188,20 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
match self.reader.seek(SeekFrom::Start(req.offset)).await {
Ok(_) => {
let mut buf = vec![0u8; req.size];
- match self.reader.read_exact(&mut buf).await {
- Ok(_) => {
- req.reply(&buf)
- },
- Err(e) => {
- req.io_fail(e)
+ let mut read = 0;
+ let mut res = Ok(());
+ while read < req.size && res.is_ok() {
+ match self.reader.read(&mut buf).await {
+ Ok(0) => { break; },
+ Ok(n) => { read += n; },
+ Err(e) => { res = Err(e); },
}
+ };
+ if let Err(e) = res {
+ req.io_fail(e)
+ } else {
+ buf.truncate(read);
+ req.reply(&buf)
}
},
Err(e) => {
--
2.39.2
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact
2023-06-29 10:32 [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact Fabian Grünbichler
@ 2023-06-29 11:03 ` Dominik Csapak
2023-06-29 11:35 ` Fabian Grünbichler
2023-11-27 9:53 ` Fabian Grünbichler
2023-11-27 13:22 ` Wolfgang Bumiller
2 siblings, 1 reply; 8+ messages in thread
From: Dominik Csapak @ 2023-06-29 11:03 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Fabian Grünbichler
On 6/29/23 12:32, Fabian Grünbichler wrote:
> since read_exact does not support short reads, which can easily happen if the
> mapped image's EOF is not aligned with the request size.
>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>
> Notes:
> reported on the forum:
>
> https://forum.proxmox.com/threads/problem-backing-up-using-backup-client.129347
>
> did a quick test reading from a mapped image full of random data, observed
> no performance difference..
>
> pbs-fuse-loop/src/fuse_loop.rs | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/pbs-fuse-loop/src/fuse_loop.rs b/pbs-fuse-loop/src/fuse_loop.rs
> index 3d0ef123..7e780799 100644
> --- a/pbs-fuse-loop/src/fuse_loop.rs
> +++ b/pbs-fuse-loop/src/fuse_loop.rs
> @@ -188,13 +188,20 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
> match self.reader.seek(SeekFrom::Start(req.offset)).await {
> Ok(_) => {
> let mut buf = vec![0u8; req.size];
> - match self.reader.read_exact(&mut buf).await {
> - Ok(_) => {
> - req.reply(&buf)
> - },
> - Err(e) => {
> - req.io_fail(e)
> + let mut read = 0;
> + let mut res = Ok(());
> + while read < req.size && res.is_ok() {
> + match self.reader.read(&mut buf).await {
> + Ok(0) => { break; },
> + Ok(n) => { read += n; },
> + Err(e) => { res = Err(e); },
> }
according to https://doc.rust-lang.org/std/io/trait.Read.html the error
with errorkind 'Interrupted' should be retried so imho we should do that here?
> + };
> + if let Err(e) = res {
> + req.io_fail(e)
> + } else {
> + buf.truncate(read);
> + req.reply(&buf)
> }
> },
> Err(e) => {
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact
2023-06-29 11:03 ` Dominik Csapak
@ 2023-06-29 11:35 ` Fabian Grünbichler
2023-06-29 11:46 ` Dominik Csapak
0 siblings, 1 reply; 8+ messages in thread
From: Fabian Grünbichler @ 2023-06-29 11:35 UTC (permalink / raw)
To: Dominik Csapak, Proxmox Backup Server development discussion
On June 29, 2023 1:03 pm, Dominik Csapak wrote:
> On 6/29/23 12:32, Fabian Grünbichler wrote:
>> since read_exact does not support short reads, which can easily happen if the
>> mapped image's EOF is not aligned with the request size.
>>
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>
>> Notes:
>> reported on the forum:
>>
>> https://forum.proxmox.com/threads/problem-backing-up-using-backup-client.129347
>>
>> did a quick test reading from a mapped image full of random data, observed
>> no performance difference..
>>
>> pbs-fuse-loop/src/fuse_loop.rs | 19 +++++++++++++------
>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/pbs-fuse-loop/src/fuse_loop.rs b/pbs-fuse-loop/src/fuse_loop.rs
>> index 3d0ef123..7e780799 100644
>> --- a/pbs-fuse-loop/src/fuse_loop.rs
>> +++ b/pbs-fuse-loop/src/fuse_loop.rs
>> @@ -188,13 +188,20 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
>> match self.reader.seek(SeekFrom::Start(req.offset)).await {
>> Ok(_) => {
>> let mut buf = vec![0u8; req.size];
>> - match self.reader.read_exact(&mut buf).await {
>> - Ok(_) => {
>> - req.reply(&buf)
>> - },
>> - Err(e) => {
>> - req.io_fail(e)
>> + let mut read = 0;
>> + let mut res = Ok(());
>> + while read < req.size && res.is_ok() {
>> + match self.reader.read(&mut buf).await {
>> + Ok(0) => { break; },
>> + Ok(n) => { read += n; },
>> + Err(e) => { res = Err(e); },
>> }
>
> according to https://doc.rust-lang.org/std/io/trait.Read.html the error
> with errorkind 'Interrupted' should be retried so imho we should do that here?
this is tokio's AsyncRead(Ext), which doesn't have that remark (and
AFAICT, we don't handle that error when using it anywhere).. if it
were std::io::Read I'd have used our ReadExt's read_exact_or_eof :)
>
>> + };
>> + if let Err(e) = res {
>> + req.io_fail(e)
>> + } else {
>> + buf.truncate(read);
>> + req.reply(&buf)
>> }
>> },
>> Err(e) => {
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact
2023-06-29 11:35 ` Fabian Grünbichler
@ 2023-06-29 11:46 ` Dominik Csapak
0 siblings, 0 replies; 8+ messages in thread
From: Dominik Csapak @ 2023-06-29 11:46 UTC (permalink / raw)
To: Fabian Grünbichler, Proxmox Backup Server development discussion
On 6/29/23 13:35, Fabian Grünbichler wrote:
> On June 29, 2023 1:03 pm, Dominik Csapak wrote:
>> On 6/29/23 12:32, Fabian Grünbichler wrote:
>>> since read_exact does not support short reads, which can easily happen if the
>>> mapped image's EOF is not aligned with the request size.
>>>
>>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>> ---
>>>
>>> Notes:
>>> reported on the forum:
>>>
>>> https://forum.proxmox.com/threads/problem-backing-up-using-backup-client.129347
>>>
>>> did a quick test reading from a mapped image full of random data, observed
>>> no performance difference..
>>>
>>> pbs-fuse-loop/src/fuse_loop.rs | 19 +++++++++++++------
>>> 1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/pbs-fuse-loop/src/fuse_loop.rs b/pbs-fuse-loop/src/fuse_loop.rs
>>> index 3d0ef123..7e780799 100644
>>> --- a/pbs-fuse-loop/src/fuse_loop.rs
>>> +++ b/pbs-fuse-loop/src/fuse_loop.rs
>>> @@ -188,13 +188,20 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
>>> match self.reader.seek(SeekFrom::Start(req.offset)).await {
>>> Ok(_) => {
>>> let mut buf = vec![0u8; req.size];
>>> - match self.reader.read_exact(&mut buf).await {
>>> - Ok(_) => {
>>> - req.reply(&buf)
>>> - },
>>> - Err(e) => {
>>> - req.io_fail(e)
>>> + let mut read = 0;
>>> + let mut res = Ok(());
>>> + while read < req.size && res.is_ok() {
>>> + match self.reader.read(&mut buf).await {
>>> + Ok(0) => { break; },
>>> + Ok(n) => { read += n; },
>>> + Err(e) => { res = Err(e); },
>>> }
>>
>> according to https://doc.rust-lang.org/std/io/trait.Read.html the error
>> with errorkind 'Interrupted' should be retried so imho we should do that here?
>
> this is tokio's AsyncRead(Ext), which doesn't have that remark (and
> AFAICT, we don't handle that error when using it anywhere).. if it
> were std::io::Read I'd have used our ReadExt's read_exact_or_eof :)
yes, you're right ofc. sorry for the noise then
>
>>
>>> + };
>>> + if let Err(e) = res {
>>> + req.io_fail(e)
>>> + } else {
>>> + buf.truncate(read);
>>> + req.reply(&buf)
>>> }
>>> },
>>> Err(e) => {
>>
>>
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact
2023-06-29 10:32 [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact Fabian Grünbichler
2023-06-29 11:03 ` Dominik Csapak
@ 2023-11-27 9:53 ` Fabian Grünbichler
2023-11-27 13:22 ` Wolfgang Bumiller
2 siblings, 0 replies; 8+ messages in thread
From: Fabian Grünbichler @ 2023-11-27 9:53 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
ping, still applies..
On June 29, 2023 12:32 pm, Fabian Grünbichler wrote:
> since read_exact does not support short reads, which can easily happen if the
> mapped image's EOF is not aligned with the request size.
>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>
> Notes:
> reported on the forum:
>
> https://forum.proxmox.com/threads/problem-backing-up-using-backup-client.129347
>
> did a quick test reading from a mapped image full of random data, observed
> no performance difference..
>
> pbs-fuse-loop/src/fuse_loop.rs | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/pbs-fuse-loop/src/fuse_loop.rs b/pbs-fuse-loop/src/fuse_loop.rs
> index 3d0ef123..7e780799 100644
> --- a/pbs-fuse-loop/src/fuse_loop.rs
> +++ b/pbs-fuse-loop/src/fuse_loop.rs
> @@ -188,13 +188,20 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
> match self.reader.seek(SeekFrom::Start(req.offset)).await {
> Ok(_) => {
> let mut buf = vec![0u8; req.size];
> - match self.reader.read_exact(&mut buf).await {
> - Ok(_) => {
> - req.reply(&buf)
> - },
> - Err(e) => {
> - req.io_fail(e)
> + let mut read = 0;
> + let mut res = Ok(());
> + while read < req.size && res.is_ok() {
> + match self.reader.read(&mut buf).await {
> + Ok(0) => { break; },
> + Ok(n) => { read += n; },
> + Err(e) => { res = Err(e); },
> }
> + };
> + if let Err(e) = res {
> + req.io_fail(e)
> + } else {
> + buf.truncate(read);
> + req.reply(&buf)
> }
> },
> Err(e) => {
> --
> 2.39.2
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact
2023-06-29 10:32 [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact Fabian Grünbichler
2023-06-29 11:03 ` Dominik Csapak
2023-11-27 9:53 ` Fabian Grünbichler
@ 2023-11-27 13:22 ` Wolfgang Bumiller
2023-11-27 17:27 ` Fabian Grünbichler
2 siblings, 1 reply; 8+ messages in thread
From: Wolfgang Bumiller @ 2023-11-27 13:22 UTC (permalink / raw)
To: Fabian Grünbichler; +Cc: pbs-devel
On Thu, Jun 29, 2023 at 12:32:13PM +0200, Fabian Grünbichler wrote:
> since read_exact does not support short reads, which can easily happen if the
> mapped image's EOF is not aligned with the request size.
>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>
> Notes:
> reported on the forum:
>
> https://forum.proxmox.com/threads/problem-backing-up-using-backup-client.129347
>
> did a quick test reading from a mapped image full of random data, observed
> no performance difference..
Do you get one if we just drop the loop logic and *actually* just
`read()` once? IMO this is more in line with what a read syscall
*should* be doing.
Further, we use a `CachedChunkReader` under it which actually does a
read loop anyway, so AFAICT this *can't* make a difference.
>
> pbs-fuse-loop/src/fuse_loop.rs | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/pbs-fuse-loop/src/fuse_loop.rs b/pbs-fuse-loop/src/fuse_loop.rs
> index 3d0ef123..7e780799 100644
> --- a/pbs-fuse-loop/src/fuse_loop.rs
> +++ b/pbs-fuse-loop/src/fuse_loop.rs
> @@ -188,13 +188,20 @@ impl<R: AsyncRead + AsyncSeek + Unpin> FuseLoopSession<R> {
> match self.reader.seek(SeekFrom::Start(req.offset)).await {
> Ok(_) => {
> let mut buf = vec![0u8; req.size];
> - match self.reader.read_exact(&mut buf).await {
> - Ok(_) => {
> - req.reply(&buf)
> - },
> - Err(e) => {
> - req.io_fail(e)
> + let mut read = 0;
> + let mut res = Ok(());
> + while read < req.size && res.is_ok() {
> + match self.reader.read(&mut buf).await {
> + Ok(0) => { break; },
> + Ok(n) => { read += n; },
> + Err(e) => { res = Err(e); },
> }
> + };
> + if let Err(e) = res {
> + req.io_fail(e)
> + } else {
> + buf.truncate(read);
> + req.reply(&buf)
> }
> },
> Err(e) => {
> --
> 2.39.2
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact
2023-11-27 13:22 ` Wolfgang Bumiller
@ 2023-11-27 17:27 ` Fabian Grünbichler
2023-11-28 10:07 ` Wolfgang Bumiller
0 siblings, 1 reply; 8+ messages in thread
From: Fabian Grünbichler @ 2023-11-27 17:27 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pbs-devel
> Wolfgang Bumiller <w.bumiller@proxmox.com> hat am 27.11.2023 14:22 CET geschrieben:
>
> On Thu, Jun 29, 2023 at 12:32:13PM +0200, Fabian Grünbichler wrote:
> > since read_exact does not support short reads, which can easily happen if the
> > mapped image's EOF is not aligned with the request size.
> >
> > Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> > ---
> >
> > Notes:
> > reported on the forum:
> >
> > https://forum.proxmox.com/threads/problem-backing-up-using-backup-client.129347
> >
> > did a quick test reading from a mapped image full of random data, observed
> > no performance difference..
>
> Do you get one if we just drop the loop logic and *actually* just
> `read()` once? IMO this is more in line with what a read syscall
> *should* be doing.
> Further, we use a `CachedChunkReader` under it which actually does a
> read loop anyway, so AFAICT this *can't* make a difference.
with a plain read (+ optional truncate of the reply buf) performance is still the same. but (and I am unfortunately not sure if this is a regression in the meantime, or was also broken back when I originally wrote this patch) access via the loop device actually truncates the resulting data:
- my test input image is 1701838801 bytes long (arbitrary misaligned size, straight from /dev/urandom)
- the fuse session correctly gets this passed in as size
- a regular restore restores as many (correct) bytes
- reading via the loop device with bs=1024 or bs=512 or bs=32 only returns 1701838336 bytes (465 are missing)
-- the fuse requests quickly ramp up to 128k request size (no matter the block size used to read from the loop device)
-- the last fuse read request is for 16384 bytes, but the read from PBS (correctly!) only returns 16337
-- 16337 - 31*512 = 465
-- so it seems the short read result is lost somewhere?
-- reading with O_DIRECT doesn't help (in fact, it tanks performance while still reproducing the issue)
anyhow, this requires further analysis and fixing before being applied in whichever fashion..
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact
2023-11-27 17:27 ` Fabian Grünbichler
@ 2023-11-28 10:07 ` Wolfgang Bumiller
0 siblings, 0 replies; 8+ messages in thread
From: Wolfgang Bumiller @ 2023-11-28 10:07 UTC (permalink / raw)
To: Fabian Grünbichler; +Cc: pbs-devel
On Mon, Nov 27, 2023 at 06:27:53PM +0100, Fabian Grünbichler wrote:
> > Wolfgang Bumiller <w.bumiller@proxmox.com> hat am 27.11.2023 14:22 CET geschrieben:
> >
> > On Thu, Jun 29, 2023 at 12:32:13PM +0200, Fabian Grünbichler wrote:
> > > since read_exact does not support short reads, which can easily happen if the
> > > mapped image's EOF is not aligned with the request size.
> > >
> > > Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> > > ---
> > >
> > > Notes:
> > > reported on the forum:
> > >
> > > https://forum.proxmox.com/threads/problem-backing-up-using-backup-client.129347
> > >
> > > did a quick test reading from a mapped image full of random data, observed
> > > no performance difference..
> >
> > Do you get one if we just drop the loop logic and *actually* just
> > `read()` once? IMO this is more in line with what a read syscall
> > *should* be doing.
> > Further, we use a `CachedChunkReader` under it which actually does a
> > read loop anyway, so AFAICT this *can't* make a difference.
>
> with a plain read (+ optional truncate of the reply buf) performance is still the same. but (and I am unfortunately not sure if this is a regression in the meantime, or was also broken back when I originally wrote this patch) access via the loop device actually truncates the resulting data:
>
> - my test input image is 1701838801 bytes long (arbitrary misaligned size, straight from /dev/urandom)
it's a loop device -> these are block devices defaulting to 512 blocks
1701838801 % 512 = 465
if you call `losetup` manually on it you'll get a warning like:
losetup: /your/file: Warning: file does not fit into a 512-byte sector; the end of the file will be ignored.
> - the fuse session correctly gets this passed in as size
> - a regular restore restores as many (correct) bytes
> - reading via the loop device with bs=1024 or bs=512 or bs=32 only returns 1701838336 bytes (465 are missing)
> -- the fuse requests quickly ramp up to 128k request size (no matter the block size used to read from the loop device)
> -- the last fuse read request is for 16384 bytes, but the read from PBS (correctly!) only returns 16337
> -- 16337 - 31*512 = 465
> -- so it seems the short read result is lost somewhere?
> -- reading with O_DIRECT doesn't help (in fact, it tanks performance while still reproducing the issue)
>
> anyhow, this requires further analysis and fixing before being applied in whichever fashion..
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-11-28 10:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-29 10:32 [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact Fabian Grünbichler
2023-06-29 11:03 ` Dominik Csapak
2023-06-29 11:35 ` Fabian Grünbichler
2023-06-29 11:46 ` Dominik Csapak
2023-11-27 9:53 ` Fabian Grünbichler
2023-11-27 13:22 ` Wolfgang Bumiller
2023-11-27 17:27 ` Fabian Grünbichler
2023-11-28 10:07 ` Wolfgang Bumiller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox