GNU bug report logs - #62404
--reflink=auto not falling back appropriately to standard copy in all cases

Previous Next

Full log

Message #16 received at 62404 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 62404 <at> debbugs.gnu.org
Subject: Re: bug#62404: --reflink=auto not falling back appropriately on older
 kernels
Date: Thu, 23 Mar 2023 15:57:16 -0700

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.