darcs

Issue 904 record fails on sshfs (sshfs has no atomic_create)

Title record fails on sshfs (sshfs has no atomic_create)
Priority feature Status resolved
Milestone Resolved in 2.10.0
Superseder Nosy List Serware, darcs-devel, davidsarah, dmitry.kurochkin, fuse-sshfs, gour, jaredj, jch, kowey, nwf, thorkilnaur, tommy, twb
Assigned To jch
Topics ProbablyEasy, SSH

Created on 2008-06-05.03:20:03 by twb, last changed 2013-02-15.14:00:38 by noreply.

Files
File name Uploaded Type Edit Remove
darcs-sshfs-sloppy-locks.dpatch nwf, 2008-07-11.05:21:50 text/x-darcs-patch
typescript twb, 2008-06-05.03:20:00 text/plain
Messages
msg4953 (view) Author: twb Date: 2008-06-05.03:20:00
darcs record fails on sshfs filesystems because sshfs does not (and
cannot, due to limitations of the underlying protocol, SFTP) implement
the atomic_create operation.  The attached transcript demonstrates the
failure.  For reference, I have also included analogous transcripts of
mercurial (hg) and git; the former works, the latter fails without
much explanation.

If it can be done easily and simply, I would like darcs to fall back
to whatever method hg uses (assuming it's safe) when atomic_create
isn't available.
Attachments
msg4961 (view) Author: kowey Date: 2008-06-05.07:33:07
I'm in no position to comment on this bug-report.  I do note that on issue554,
David says:

> You could try that, but sshfs is a pretty cruddy piece of work, and may not
> work very well.  It doesn't implement POSIX filesystem semantics.

Which makes think that this is wont-fix.

Juliusz: if you are around, perhaps you might have something to say about it? 
Otherwise, just ignore and I'll un-assign in you in a week
msg5209 (view) Author: nwf Date: 2008-07-11.05:21:49
Please note that Darcs issues no warnings when using DARCS_SLOPPY_LOCKS=Yes 
over sshfs, and, from limited testing, seems to correctly block other instances 
from mutating the repository concurrently.  It would be convenient if darcs 
fell back to the sloppy locking mechanism in the presence of ENOSYS, as it 
already does in the presence of EOPNOTSUPP or EPERM.  In fact, I've attached a 
patch to do exactly that.  Thoughts?
Attachments
msg5211 (view) Author: jch Date: 2008-07-11.10:55:24
> Please note that Darcs issues no warnings when using
> DARCS_SLOPPY_LOCKS=Yes over sshfs, and, from limited testing, seems
> to correctly block other instances from mutating the repository
> concurrently.

There's a race condition.  You won't be able to demonstrate it while
testing, but it wil trigger at some point.

If you run darcs over sshfs, your repository will get corrupted, your
computer will explode, your son will become a Wall Street trader and
your daughter will go work for Microsoft.

This warranty is void in case of nuclear war, whether caused by Darcs
or otherwise.

> It would be convenient if darcs fell back to the sloppy locking
> mechanism in the presence of ENOSYS, as it already does in the
> presence of EOPNOTSUPP or EPERM.  In fact, I've attached a patch to
> do exactly that.

I strongly oppose doing that.

                                        Juliusz
msg5214 (view) Author: nwf Date: 2008-07-11.17:22:14
> There's a race condition.  You won't be able to demonstrate it while
> testing, but it wil trigger at some point.

Can you please be more verbose?  darcs already falls back to sloppy locks 
under several other circumstances, so what is intrinsically wrong with sshfs 
that induces a race where these other fallbacks don't?
msg5216 (view) Author: jch Date: 2008-07-11.19:32:08
>> There's a race condition.  You won't be able to demonstrate it while
>> testing, but it wil trigger at some point.

> Can you please be more verbose?  darcs already falls back to sloppy locks 
> under several other circumstances, so what is intrinsically wrong with sshfs 
> that induces a race where these other fallbacks don't?

open(O_CREAT|O_EXCL) is not atomic.

                                        Juliusz
msg5218 (view) Author: nwf Date: 2008-07-12.10:18:38
I'm afraid I don't understand...  Looking at "man 2 open", I see:

> O_EXCL Ensure that this call creates the file: if this flag is specified in
> conjunction with O_CREAT, and pathname already exists, then open() will
> fail.

Moreover, The Open Group agrees 
(http://www.opengroup.org/onlinepubs/000095399/functions/open.html; IEEE 
1003.1, 2004 edition):

> O_EXCL
>  If O_CREAT and O_EXCL are set, open() shall fail if the file exists. The
>  check for the existence of the file and the creation of the file if it does
>  not exist shall be atomic with respect to other threads executing open()
>  naming the same filename in the same directory with O_EXCL and O_CREAT set.

There are some warnings about NFS in the man page, but this isn't NFS, so 
let's venture onwards; perhaps the SFTP definition is inadequate... Reading 
the SFTP spec (http://www.openssh.org/txt/draft-ietf-secsh-filexfer-02.txt), I 
see

>    SSH_FXF_EXCL
>      Causes the request to fail if the named file already exists.

OK, that looks fine.  How about sshfs?  If this isn't behaving correctly, then 
it's a bug in sshfs, given the above definition from the protocol.  Looking 
inside sshfs's code (v2.1), I see, inside sshfs.c:/^sshfs_open_common

> 2147         if (fi->flags & O_EXCL)
> 2148                 pflags |= SSH_FXF_EXCL;

Also looks fine... so perhaps the problem is in the server, but if so this 
surely would be a server bug.  Inside OpenSSH's code (CVS rev 1.84 of 
sftp-server.c), in :/^flags_from_portable

> 125  	if (pflags & SSH2_FXF_EXCL)
> 126        flags |= O_EXCL;

AFAICT, sshfs and openssh understand open(O_CREAT|O_EXCL) just fine and carry 
the O_EXCL bit across the SFTP transport faithfully; further, open(O_CREAT|
O_EXCL) is inherently an atomic operation, or you've found a bug in the host 
OS, not sshfs.  If there is a problem or race here, I believe you have it 
misidentified.  In the interim, please reconsider your strong disapproval?

Thanks.
msg5219 (view) Author: jch Date: 2008-07-12.17:28:42
> AFAICT, sshfs and openssh understand open(O_CREAT|O_EXCL) just fine and carry 
> the O_EXCL bit across the SFTP transport faithfully; further, open(O_CREAT|
> O_EXCL) is inherently an atomic operation, or you've found a bug in the host 
> OS, not sshfs.

This was not the case when I looked at sshfs, which was over a year
ago.  I'm glad to hear that sshfs has improved since then.

> If there is a problem or race here, I believe you have it 
> misidentified.  In the interim, please reconsider your strong disapproval?

Only if I get a confirmation from the sshfs developers themselves.
(Search the list archives for ``AFS semantics'' for an example.)

                                        Juliusz
msg5221 (view) Author: nwf Date: 2008-07-14.18:30:00
Hello fuse-sshfs.  I'm attempting to get a patch included into darcs to make
it fall back to open(O_CREAT|O_EXCL) locking mechanisms when link() returns
ENOSYS as it does on sshfs.  While it certainly appears that this operation
is safe, the darcs developers would like confirmation that this is the case.

Thanks much.
--nwf;

On Sat, Jul 12, 2008 at 07:28:16PM +0200, Juliusz Chroboczek wrote:
> > AFAICT, sshfs and openssh understand open(O_CREAT|O_EXCL) just fine and carry 
> > the O_EXCL bit across the SFTP transport faithfully; further, open(O_CREAT|
> > O_EXCL) is inherently an atomic operation, or you've found a bug in the host 
> > OS, not sshfs.
> 
> This was not the case when I looked at sshfs, which was over a year
> ago.  I'm glad to hear that sshfs has improved since then.
> 
> > If there is a problem or race here, I believe you have it 
> > misidentified.  In the interim, please reconsider your strong disapproval?
> 
> Only if I get a confirmation from the sshfs developers themselves.
> (Search the list archives for ``AFS semantics'' for an example.)
> 
>                                         Juliusz
msg7193 (view) Author: twb Date: 2009-01-26.00:37:55
The strict requirements of roundup's Subject header field parser are annoying.

----- Forwarded message from Miklos Szeredi <miklos@szeredi.hu> -----

To: trentbuck@gmail.com
CC: fuse-sshfs@lists.sourceforge.net
Subject: Re: [sshfs] [issue904] record fails on sshfs (sshfs has no
	atomic_create)
From: Miklos Szeredi <miklos@szeredi.hu>
Date: Sun, 25 Jan 2009 20:32:00 +0100

On Sun, 25 Jan 2009, trentbuck@gmail.com (Trent W. Buck wrote:
> Nathaniel W Filardo <nwf@cs.jhu.edu> writes:
> >>> open(O_CREAT| O_EXCL) is inherently an atomic operation [...]
> >>> AFAICT, sshfs understands open(O_CREAT|O_EXCL) just fine and carries
> >>> the O_EXCL bit across the SFTP transport faithfully[...]
> 
> Would a developer here please confirm the above?

It has been true for a while now (since about 2006).  The requrements
for atomic create+open are:

  linux kernel >= 2.6.15
  libfuse >= 2.5
  sshfs >= 1.3

> 
> Such confirmation is necessary before Darcs developers will remove a
> paranoia setting that prevents Darcs from working over sshfs:
> 
> > Hello fuse-sshfs.  I'm attempting to get a patch included into
> > darcs to make it fall back to open(O_CREAT|O_EXCL) locking
> > mechanisms when link() returns ENOSYS as it does on sshfs.  While
> > it certainly appears that this operation is safe, the darcs
> > developers would like confirmation that this is the case.
> 
> It seems Nathaniel didn't receive a reply.  Perhaps this list is for
> users, and the developer list is elsewhere?

It seems the previous posting didn't make it to the list.  It may have
been discarded accidentally while moderating for spam.

Thanks,
Miklos

----- End forwarded message -----
msg8557 (view) Author: gour Date: 2009-08-28.05:42:31
Hi!

>It has been true for a while now (since about 2006).  The requrements
>for atomic create+open are:

>  linux kernel >= 2.6.15
>  libfuse >= 2.5
>  sshfs >= 1.3

>> Such confirmation is necessary before Darcs developers will remove a
>> paranoia setting that prevents Darcs from working over sshfs:

Any news regarding the above?


Sincerely,
Gour
Sincerely,
Gour
msg8568 (view) Author: kowey Date: 2009-08-28.11:06:11
Hi Juliusz,

Any chance you could have a second look at this ticket?  

All I understand about this thread is that Nathaniel (nwf) submitted a patch
which you disapproved of pending some confirmation from sshfs developers.

And that this reply from Miklos Szeredi <miklos@szeredi.hu> on the sshfs list
makes me think we have that confirmation:

> Nathaniel W Filardo <nwf@cs.jhu.edu> writes:
> >>> open(O_CREAT| O_EXCL) is inherently an atomic operation [...]
> >>> AFAICT, sshfs understands open(O_CREAT|O_EXCL) just fine and carries
> >>> the O_EXCL bit across the SFTP transport faithfully[...]

>It has been true for a while now (since about 2006).  The requrements
>for atomic create+open are:

>  linux kernel >= 2.6.15
>  libfuse >= 2.5
>  sshfs >= 1.3

>> Such confirmation is necessary before Darcs developers will remove a
>> paranoia setting that prevents Darcs from working over sshfs:

Thanks!
msg16595 (view) Author: gh Date: 2013-02-15.13:52:38
Sent Nathaniel's patch as http://bugs.darcs.net/patch1022 and will accept 
it.
msg16596 (view) Author: noreply Date: 2013-02-15.14:00:36
The following patch sent by Nathaniel Filardo <nwf@cs.jhu.edu> updated issue issue904 with
status=resolved;resolvedin=2.10.0 HEAD

* Resolve issue904: Fix record on Linux/FUSE/sshfs (fall back to sloppy locks automatically) 
Ignore-this: acdc2639207ccb4f73042397b549dc4b
History
Date User Action Args
2008-06-05 03:20:03twbcreate
2008-06-05 07:02:05koweylinkissue905 superseder
2008-06-05 07:33:09koweysetstatus: unread -> wont-fix
title: record fails on sshfs (sshfs has no atomic_create) -> record fails on sshfs (sshfs has no atomic_create) [expire 2008-06-12]
nosy: + jch, kowey
messages: + msg4961
priority: feature
assignedto: jch
2008-06-12 12:11:58koweysetnosy: jch, tommy, beschmi, kowey, dagit, twb
assignedto: jch -> (no value)
title: record fails on sshfs (sshfs has no atomic_create) [expire 2008-06-12] -> record fails on sshfs (sshfs has no atomic_create)
2008-07-11 05:22:03nwfsetstatus: wont-fix -> has-patch
nosy: + Serware, nwf, jaredj
topic: + Patch, ProbablyEasy, SSH, Darcs2
messages: + msg5209
files: + darcs-sshfs-sloppy-locks.dpatch
2008-07-11 10:55:28jchsetnosy: + serware
messages: + msg5211
2008-07-11 17:22:31nwfsetnosy: jch, tommy, beschmi, kowey, dagit, twb, jaredj, serware, Serware, nwf
messages: + msg5214
2008-07-11 19:32:12jchsetnosy: jch, tommy, beschmi, kowey, dagit, twb, jaredj, serware, Serware, nwf
messages: + msg5216
2008-07-12 10:18:52nwfsetnosy: jch, tommy, beschmi, kowey, dagit, twb, jaredj, serware, Serware, nwf
messages: + msg5218
2008-07-12 17:28:46jchsetnosy: jch, tommy, beschmi, kowey, dagit, twb, jaredj, serware, Serware, nwf
messages: + msg5219
2008-07-14 18:30:06nwfsetnosy: + fuse-sshfs
messages: + msg5221
2008-08-15 06:55:06koweysetstatus: has-patch -> waiting-for
nosy: + darcs-devel
2009-01-26 00:38:00twbsetnosy: + dmitry.kurochkin, simon, thorkilnaur
messages: + msg7193
2009-08-06 21:07:14adminsetnosy: - beschmi
2009-08-11 00:16:23adminsetnosy: - dagit
2009-08-25 17:39:54adminsetnosy: - simon
2009-08-27 14:15:21adminsetnosy: jch, tommy, kowey, darcs-devel, twb, thorkilnaur, jaredj, dmitry.kurochkin, serware, Serware, nwf, fuse-sshfs
2009-08-28 05:42:39goursettopic: - Patch
nosy: + gour
messages: + msg8557
2009-08-28 11:06:18koweysettopic: - Darcs2
nosy: jch, tommy, kowey, darcs-devel, twb, thorkilnaur, jaredj, gour, dmitry.kurochkin, serware, Serware, nwf, fuse-sshfs
messages: + msg8568
assignedto: jch
2009-10-23 22:43:36adminsetnosy: - Serware
2009-10-23 23:27:47adminsetnosy: + Serware, - serware
2010-02-11 02:28:41davidsarahsetnosy: + davidsarah
2013-02-15 13:52:39ghsetmessages: + msg16595
2013-02-15 14:00:38noreplysetstatus: waiting-for -> resolved
messages: + msg16596
resolvedin: 2.10.0