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

[Filipe Manana <fdmanana@gmail.com>]

On Wed, Sep 29, 2021 at 11:25 AM Qu Wenruo <wqu@suse.com> wrote:
>
>
>
> 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.

--clone-fallback

Sounds fine to me. It's short enough and gives some clue what it's about.
It's not completely self-explanatory, but I don't have a better
suggestion, and I think any name will always require an explanation of
what it does and when it's useful in the documentation (--help, man
page).

Thanks.


>
> 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
> >>>>
> >>>
> >>>
> >
> >
> >
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”