Re: [PATCH RFC] btrfs-progs: receive: fallback to buffered copy if clone failed

[Qu Wenruo <wqu@suse.com>]

On 2021/9/29 17:59, Filipe Manana wrote:
[...]
>>> Normally we should only exit if errno is not EINTR, and retry
>>> (continue) on the EINTR case.
>>
>> For this, I'm a little concerned.
>>
>> EINTR means the operation is interrupted (by signal).
>> In that case, doesn't it mean the user want to stop the receive?
> 
> There will be plenty of opportunities to be interrupted and still be responsive.
> But ok, you can leave it as it is.
> 
>>
[...]
>>>
>>> What if we have thousands of clone operations?
>>> Is there any rate limited print() in progs like there is for kernel?
>>
>> Unfortunately we don't yet have.
>>
>> But the good news (that I didn't catch at time of writing) is, we now
>> have global verbose/quite switch, so that we can easily hide those
>> warning by default and only output such warning for verbose run.
> 
> The problem with this is that it will possibly hide future bugs with
> the kernel sending incorrect clone operations.
> 
> Having this fallback-to-read-write behaviour by default would hide
> some bugs we had in the past on the kernel side, and unless users
> start running receive with the verbose switch, we won't know about it.
> Even if they do run with the verbose switch, some might not notice the
> warnings at all, and for those who notice it they might not bother to
> report the warnings since the receive succeeded and they didn't find
> any data corruption/loss.
> 
> Or we might start receiving reports about send/receive doing less
> cloning/extent sharing than before, and we won't easily know that the
> receive side has fallen back to this read-write behaviour due to some
> bug on kernel.
> 
> That's why making the behaviour explicit through a new command line
> flag would cause less surprises and make it harder to miss regressions
> on the kernel. And add some documentation explaining on which cases
> the flag is useful.

This sounds indeed better, handling both cases well.

I'll try to add a new option to receive, maybe something called 
--clone-fallback.

Any advice on the option name would be very helpful.

Thanks,
Qu

> 
> Thanks.
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>> That's one reason why my proposal had in mind an optional flag to
>>> trigger this behaviour.
>>>
>>> Thanks for doing it, I was planning on doing something similar soon.
>>>
>>>> +               ret = buffered_copy(clone_fd, rctx->write_fd, clone_offset,
>>>> +                                   len, offset);
>>>>           }
>>>>
>>>>    out:
>>>> --
>>>> 2.33.0
>>>>
>>>
>>>
> 
> 
>