bug#62404: --reflink=auto not falling back appropriately on older kernels

[Paul Eggert]

Thanks for looking into this.

On 3/23/23 07:55, Pádraig Brady wrote:

> Perhaps we should use the above __ANDROID__ logic in all cases,
> so that we fallback unless there is a specific reason not to.

Yes, that sounds better.

> +/* Whether the errno indicates operation is a transient failure.
> +   I.e., a failure that would indicate the operation _is_ supported,
> +   but has failed in a terminal way.  */

I got confused by the terminology here, as "transient failure" has the 
opposite connotation from terminal failure. I suggest that we remove the 
function (since it's used only once and its name is confusing) and 
replace its calling code as suggested below.


> -            if (errno == EPERM && *total_n_read == 0)
> +            if (is_CLONENOTSUP (errno, *total_n_read))

The *total_n_read should be *total_n_read != 0 partly for style and 
partly to be portable to ancient compilers that don't properly support 
bool. More important, I suggest leaving is_CLONENOTSUP's API as-is, and 
changing the 'if' to:

   if (*total_n_read == 0 && is_CLONENOTSUP (errno))

as I don't see why we should treat EPERM differently from ENOSYS etc.


>  /* Whether the errno from FICLONE, or copy_file_range
>     indicates operation is not supported for this file or file system.  */
>  
>  static bool
> -is_CLONENOTSUP (int err)
> +is_CLONENOTSUP (int err, bool partial)

Since the code no longer calls is_CLONENOTSUP when FICLONE fails, the 
comment should be changed to not mention FICLONE.


> +  /* If the clone operation is not creating the destination (i.e., FICLONE)
> +     then we could possibly be more restrictive with errors deemed non 
> terminal.
> +     However doing that like in the following
> +     would be more coupled to disparate errno handling on various systems.
> +     if (0 <= dest_desc)
> +       transient_failure = ! is_CLONENOTSUP (errno, false);  */
> +  bool transient_failure = is_transient_failure (errno);

I had trouble with those two "transient_failure"s lining up, plus the 
terminology thing mentioned above.  I suggest changing this to:

  /* When FICLONE fails, report failure only with errno values known
     to mean trouble when FICLONE is supported and called properly.
     Do do not report failure merely because !is_CLONENOTSUP (errno),
     as too many systems yield oddball errno values here.  */
  bool report_failure = (errno == EIO || errno == ENOMEM
                         || errno == ENOSPC || errno == EDQUOT);

and then replace all occurrences of "transient_failure" with 
"report_failure".