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

Previous Next

Full log

View this message in rfc822 format

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

On 23/03/2023 22:57, Paul Eggert wrote:
> 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".

Thanks for the review.
I'll adjust as suggested.
The is_CLONENOTSUP() change _was_ a little contrived
since it wasn't actually needed in the new implementation.
It would only be needed if we ever wanted to change back
to the more aggressive errno checks after FICLONE,
in which case the is_CLONENOTSUP() would then be correct.
I'll leave it as is and comment for now.

thanks,
Pádraig

Previous Next

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