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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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