Re: [pbs-devel] [PATCH proxmox-backup] mapped loop device: use read loop instead of read_exact

[Dominik Csapak <d.csapak@proxmox.com>]

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) => {
>>
>>
>>