Re: svn_io_file_flush_to_disk fails on AFS filesystems on Darwin

[Nathan Hartman]

On Thu, Nov 12, 2020 at 12:25 PM Branko Čibej  wrote:
>
> On 12.11.2020 17:38, Branko Čibej wrote:
> > On 26.06.2019 17:48, Branko Čibej wrote:
> >> On 26.06.2019 04:52, Quentin Smith wrote:
> >>> Hi,
> >>>
> >>> svn_io_file_flush_to_disk in subversion/libsvn_subr/io.c fails to
> >>> flush files on AFS filesystems on Darwin. The user-visible
> >>> experience is:
> >>>
> >>> $ svn commit
> >>> svn: E25: Commit failed (details follow):
> >>> svn: E25: Can't write '/afs/path/db/txn-current' atomically
> >>> svn: E25: Can't flush file '/afs/path/db/svn-CNFY6N' to disk:
> >>> Inappropriate ioctl for device
> >>>
> >>> This is because Darwin defines F_FULLFSYNC, which on AFS (and maybe
> >>> other filesystems?) returns ENOTTY.
> >>>
> >>> The code ignores EINVAL with a comment about filesystems that don't
> >>> support it; evidently on Darwin this also needs to include ENOTTY.
> >>>
> >>> (Also, I suspect it should fall back from fcntl to fsync, since I
> >>> suspect fsync /does/ work on AFS filesystems.)
> >>>
> >>> I repro'd this with svn 1.10.3 (r1842928), but inspection of trunk
> >>> confirms the code has not changed.
> >> First of all, thanks for the repor.
> >>
> >> Please provide a standalone reproduction script, using our command-line
> >> client.
> >>
> >> Last but not least, I do wish filesystems were consistent in their
> >> implementation ... what on earth is ENOTTY doing here?
> >
> > Sometimes it's good to have ages old browser tabs lying around. This
> > is now fixed in apr_file_datasync on APR trunk and the 1.7.x branch.
> >
> > -- Brane
> >
> > https://svn.apache.org/viewvc?view=revision=1883341
>
> And r1883355 makes Subversion use the right APR functions.

I just realized that this nomination:

[[[

 * r1883355
   Use the APR-1.4+ API for flushing file contents to disk.
   Justification:
 Reduce code duplication between APR and SVN.
   Votes:
 +1: brane

]]]

isn't a mere refactoring, as I thought previously, but rather is a fix
for the above flush-to-disk issue on AFS filesystems--assuming, of
course, that SVN is built with APR >= 1.7. (Perhaps this nomination
deserves a Notes section to point that out.)

(I will probably vote +1 for this, pending to check, since we support
building with APR >= 1.4, if there is a chance of some regression
being introduced when SVN is built with APR < 1.7.)

Nathan

Re: svn_io_file_flush_to_disk fails on AFS filesystems on Darwin

[Branko Čibej]

On 12.11.2020 17:38, Branko Čibej wrote:

On 26.06.2019 17:48, Branko Čibej wrote:

On 26.06.2019 04:52, Quentin Smith wrote:

Hi,

svn_io_file_flush_to_disk in subversion/libsvn_subr/io.c fails to
flush files on AFS filesystems on Darwin. The user-visible 
experience is:


$ svn commit
svn: E25: Commit failed (details follow):
svn: E25: Can't write '/afs/path/db/txn-current' atomically
svn: E25: Can't flush file '/afs/path/db/svn-CNFY6N' to disk:
Inappropriate ioctl for device

This is because Darwin defines F_FULLFSYNC, which on AFS (and maybe
other filesystems?) returns ENOTTY.

The code ignores EINVAL with a comment about filesystems that don't
support it; evidently on Darwin this also needs to include ENOTTY.

(Also, I suspect it should fall back from fcntl to fsync, since I
suspect fsync /does/ work on AFS filesystems.)

I repro'd this with svn 1.10.3 (r1842928), but inspection of trunk
confirms the code has not changed.

First of all, thanks for the repor.

Please provide a standalone reproduction script, using our command-line
client.

Last but not least, I do wish filesystems were consistent in their
implementation ... what on earth is ENOTTY doing here?


Sometimes it's good to have ages old browser tabs lying around. This 
is now fixed in apr_file_datasync on APR trunk and the 1.7.x branch.


-- Brane

https://svn.apache.org/viewvc?view=revision=1883341


And r1883355 makes Subversion use the right APR functions.

Re: svn_io_file_flush_to_disk fails on AFS filesystems on Darwin

[Branko Čibej]

On 26.06.2019 17:48, Branko Čibej wrote:

On 26.06.2019 04:52, Quentin Smith wrote:

Hi,

svn_io_file_flush_to_disk in subversion/libsvn_subr/io.c fails to
flush files on AFS filesystems on Darwin. The user-visible experience is:

$ svn commit
svn: E25: Commit failed (details follow):
svn: E25: Can't write '/afs/path/db/txn-current' atomically
svn: E25: Can't flush file '/afs/path/db/svn-CNFY6N' to disk:
Inappropriate ioctl for device

This is because Darwin defines F_FULLFSYNC, which on AFS (and maybe
other filesystems?) returns ENOTTY.

The code ignores EINVAL with a comment about filesystems that don't
support it; evidently on Darwin this also needs to include ENOTTY.

(Also, I suspect it should fall back from fcntl to fsync, since I
suspect fsync /does/ work on AFS filesystems.)

I repro'd this with svn 1.10.3 (r1842928), but inspection of trunk
confirms the code has not changed.

First of all, thanks for the repor.

Please provide a standalone reproduction script, using our command-line
client.

Last but not least, I do wish filesystems were consistent in their
implementation ... what on earth is ENOTTY doing here?


Sometimes it's good to have ages old browser tabs lying around. This is 
now fixed in apr_file_datasync on APR trunk and the 1.7.x branch.


-- Brane

https://svn.apache.org/viewvc?view=revision=1883341

Re: svn_io_file_flush_to_disk fails on AFS filesystems on Darwin

[Nathan Hartman]

On Thu, Jun 27, 2019 at 5:20 AM Branko Čibej  wrote:

> > On 26.06.2019 20:57, Branko Čibej wrote:
> > Turns out that APR is already doing this _almost_ correctly. The kind of
> > change linked to above would fit in APR quite well. The "problem" is
> > that Subversion currently doesn't use the "new" APR functions for
> > sync/datasync. So my suggestion for the fix would be twofold:
> >
> >   * Enhance the sync functionality in APR (trunk, 1.7 and possibly 1.6)
> >   * Change Subversion to use APR's sync functions
> >
> > I'll look into this.
>
> Interesting ... "man fcntl" on macOS does *not* mention ENOTTY at all.
> So everyone who's currently implementing this fix is relying on an
> undocumented feature of the filesystem. :)


Sounds like a BUG, either in macOS, Darwin, APFS, or the man page.

Sadly it's not obvious where to report it.

Re: svn_io_file_flush_to_disk fails on AFS filesystems on Darwin

[Branko Čibej]

On 27.06.2019 10:56, Branko Čibej wrote:
> On 26.06.2019 20:57, Branko Čibej wrote:
>> On 26.06.2019 18:41, Nathan Hartman wrote:
>>> On Wed, Jun 26, 2019 at 11:49 AM Branko Čibej >> > wrote:
>>>
>>> On 26.06.2019 04:52, Quentin Smith wrote:
>>> > svn_io_file_flush_to_disk in subversion/libsvn_subr/io.c fails to
>>> > flush files on AFS filesystems on Darwin. The user-visible
>>> experience is:
>>>
>>>
>>> (snip)
>>>
>>> > The code ignores EINVAL with a comment about filesystems that don't
>>> > support it; evidently on Darwin this also needs to include ENOTTY.
>>> >
>>> > (Also, I suspect it should fall back from fcntl to fsync, since I
>>> > suspect fsync /does/ work on AFS filesystems.)
>>>
>>>
>>> (snip)
>>>
>>> Last but not least, I do wish filesystems were consistent in their
>>> implementation ... what on earth is ENOTTY doing here?
>>>
>>> -- Brane
>>>
>>>
>>> Looks like libuv encountered similar issues and applied similar
>>> handling to that in SQLite:
>>>
>>> https://github.com/libuv/libuv/commit/5b0e1d75a207bb1c662f4495c0d8ba1e81a5bb7d
>>>
>> Something tell me this should be solved in APR ...
>
> Turns out that APR is already doing this _almost_ correctly. The kind of
> change linked to above would fit in APR quite well. The "problem" is
> that Subversion currently doesn't use the "new" APR functions for
> sync/datasync. So my suggestion for the fix would be twofold:
>
>   * Enhance the sync functionality in APR (trunk, 1.7 and possibly 1.6)
>   * Change Subversion to use APR's sync functions
>
> I'll look into this.

Interesting ... "man fcntl" on macOS does *not* mention ENOTTY at all.
So everyone who's currently implementing this fix is relying on an
undocumented feature of the filesystem. :)

That's fun.

-- Brane

Re: svn_io_file_flush_to_disk fails on AFS filesystems on Darwin

[Branko Čibej]

On 26.06.2019 20:57, Branko Čibej wrote:
> On 26.06.2019 18:41, Nathan Hartman wrote:
>> On Wed, Jun 26, 2019 at 11:49 AM Branko Čibej > > wrote:
>>
>> On 26.06.2019 04:52, Quentin Smith wrote:
>> > svn_io_file_flush_to_disk in subversion/libsvn_subr/io.c fails to
>> > flush files on AFS filesystems on Darwin. The user-visible
>> experience is:
>>
>>
>> (snip)
>>
>> > The code ignores EINVAL with a comment about filesystems that don't
>> > support it; evidently on Darwin this also needs to include ENOTTY.
>> >
>> > (Also, I suspect it should fall back from fcntl to fsync, since I
>> > suspect fsync /does/ work on AFS filesystems.)
>>
>>
>> (snip)
>>
>> Last but not least, I do wish filesystems were consistent in their
>> implementation ... what on earth is ENOTTY doing here?
>>
>> -- Brane
>>
>>
>> Looks like libuv encountered similar issues and applied similar
>> handling to that in SQLite:
>>
>> https://github.com/libuv/libuv/commit/5b0e1d75a207bb1c662f4495c0d8ba1e81a5bb7d
>>
> Something tell me this should be solved in APR ...


Turns out that APR is already doing this _almost_ correctly. The kind of
change linked to above would fit in APR quite well. The "problem" is
that Subversion currently doesn't use the "new" APR functions for
sync/datasync. So my suggestion for the fix would be twofold:

  * Enhance the sync functionality in APR (trunk, 1.7 and possibly 1.6)
  * Change Subversion to use APR's sync functions

I'll look into this.

-- Brane

Re: svn_io_file_flush_to_disk fails on AFS filesystems on Darwin

[Branko Čibej]

On 26.06.2019 18:41, Nathan Hartman wrote:
> On Wed, Jun 26, 2019 at 11:49 AM Branko Čibej  > wrote:
>
> On 26.06.2019 04:52, Quentin Smith wrote:
> > svn_io_file_flush_to_disk in subversion/libsvn_subr/io.c fails to
> > flush files on AFS filesystems on Darwin. The user-visible
> experience is:
>
>
> (snip)
>
> > The code ignores EINVAL with a comment about filesystems that don't
> > support it; evidently on Darwin this also needs to include ENOTTY.
> >
> > (Also, I suspect it should fall back from fcntl to fsync, since I
> > suspect fsync /does/ work on AFS filesystems.)
>
>
> (snip)
>
> Last but not least, I do wish filesystems were consistent in their
> implementation ... what on earth is ENOTTY doing here?
>
> -- Brane
>
>
> Looks like libuv encountered similar issues and applied similar
> handling to that in SQLite:
>
> https://github.com/libuv/libuv/commit/5b0e1d75a207bb1c662f4495c0d8ba1e81a5bb7d
>

Something tell me this should be solved in APR ...

Re: svn_io_file_flush_to_disk fails on AFS filesystems on Darwin

[Nathan Hartman]

On Wed, Jun 26, 2019 at 11:49 AM Branko Čibej  wrote:

> On 26.06.2019 04:52, Quentin Smith wrote:
> > svn_io_file_flush_to_disk in subversion/libsvn_subr/io.c fails to
> > flush files on AFS filesystems on Darwin. The user-visible experience is:


(snip)

> The code ignores EINVAL with a comment about filesystems that don't
> > support it; evidently on Darwin this also needs to include ENOTTY.
> >
> > (Also, I suspect it should fall back from fcntl to fsync, since I
> > suspect fsync /does/ work on AFS filesystems.)


(snip)

Last but not least, I do wish filesystems were consistent in their
> implementation ... what on earth is ENOTTY doing here?
>
> -- Brane


Looks like libuv encountered similar issues and applied similar handling to
that in SQLite:

https://github.com/libuv/libuv/commit/5b0e1d75a207bb1c662f4495c0d8ba1e81a5bb7d

Re: svn_io_file_flush_to_disk fails on AFS filesystems on Darwin

[Branko Čibej]

On 26.06.2019 04:52, Quentin Smith wrote:
> Hi,
>
> svn_io_file_flush_to_disk in subversion/libsvn_subr/io.c fails to
> flush files on AFS filesystems on Darwin. The user-visible experience is:
>
> $ svn commit
> svn: E25: Commit failed (details follow):
> svn: E25: Can't write '/afs/path/db/txn-current' atomically
> svn: E25: Can't flush file '/afs/path/db/svn-CNFY6N' to disk:
> Inappropriate ioctl for device
>
> This is because Darwin defines F_FULLFSYNC, which on AFS (and maybe
> other filesystems?) returns ENOTTY.
>
> The code ignores EINVAL with a comment about filesystems that don't
> support it; evidently on Darwin this also needs to include ENOTTY.
>
> (Also, I suspect it should fall back from fcntl to fsync, since I
> suspect fsync /does/ work on AFS filesystems.)
>
> I repro'd this with svn 1.10.3 (r1842928), but inspection of trunk
> confirms the code has not changed.

First of all, thanks for the repor.

Please provide a standalone reproduction script, using our command-line
client.

Last but not least, I do wish filesystems were consistent in their
implementation ... what on earth is ENOTTY doing here?

-- Brane