[PATCH] APR_LARGEFILE and svn_io_file_open

[Garrett Rooney <ro...@electricjellyfish.net>]

In tracking down issue 2453 (commits that create fsfs revfiles greater
than 2gigs crash and burn on linux) I noticed that we don't pass
APR_LARGEFILE to svn_io_file_open in all the places where we need to. 
Specifically, we do in a few spots related to hotcopying stuff, but we
don't in fs_fs.c when we open potentially huge files like revfiles.

So I got to thinking, why not just have svn_io_file_open tack it on to
flags all the time, just like we do with APR_BINARY.  I mean it's not
really going to hurt anything, it only has an effect on some Unix
systems, where it results in O_LARGEFILE being passed to open.  I
suppose this could result in a miniscule performance hit, but the
alternative is strange hard to track down bugs whenever we
accidentally forget to pass this flag and open a file whose size can't
be represented in 31 bits.  Note that the result of opening such a
file on linux seems to be receiving SIGXFSZ, which by default crashes
the process.  Ick.

With this in mind, any objection to the following patch?  When
combined with a fix to APR to correct the size of the filePtr member
of apr_file_t it completely fixes issue 2453.

-garrett

[[[
Pass APR_LARGEFILE to apr_file_open, since it has no real down side and
allows us to stop worrying about whether or not any given file is going
to be big enough to require it.

* subversion/libsvn_subr/io.c
  (svn_io_open_file,
   get_default_file_perms,
   svn_io_open_unique_file2): APR_LARGEFILE, it does a body good.
]]]

Re: [PATCH] APR_LARGEFILE and svn_io_file_open

[Garrett Rooney <ro...@electricjellyfish.net>]

On 2/8/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> On Tue, Feb 07, 2006 at 09:27:31PM -0800, Garrett Rooney wrote:
> > In tracking down issue 2453 (commits that create fsfs revfiles greater
> > than 2gigs crash and burn on linux) I noticed that we don't pass
> > APR_LARGEFILE to svn_io_file_open in all the places where we need to.
> > Specifically, we do in a few spots related to hotcopying stuff, but we
> > don't in fs_fs.c when we open potentially huge files like revfiles.
> >
>
> We don't need to seek when hotcopying, but we need to be able to seek in
> a revfile.  I agree with David-quoting-Greg: this is a bad idea on APR
> 0.9, because it'll either cause us to write some data that's corrupt,
> or alternatively will cause us to error out when trying to seek to the
> trailer line (at the end of the revfile).
>
> This might make sense for APR 1.x, but I'm surprised that APR doesn't
> just pass O_LARGEFILE unconditionally now - why is that?

Actually, it turns out I was mistaken, we don't need to pass
APR_LARGEFILE to make things work, just fixing the filePtr variable
type fixes things.  I SWEAR when I initially tested this it was
needed, but I must have seen that behavior in one of the strange
intermediate forms of this patch, cause right now it sure doesn't seem
to be necessary.  And yes, now that I think about it we certainly
can't be doing this in APR 0.9.x.

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] APR_LARGEFILE and svn_io_file_open

[Malcolm Rowe <ma...@farside.org.uk>]

On Tue, Feb 07, 2006 at 09:27:31PM -0800, Garrett Rooney wrote:
> In tracking down issue 2453 (commits that create fsfs revfiles greater
> than 2gigs crash and burn on linux) I noticed that we don't pass
> APR_LARGEFILE to svn_io_file_open in all the places where we need to. 
> Specifically, we do in a few spots related to hotcopying stuff, but we
> don't in fs_fs.c when we open potentially huge files like revfiles.
> 

We don't need to seek when hotcopying, but we need to be able to seek in
a revfile.  I agree with David-quoting-Greg: this is a bad idea on APR
0.9, because it'll either cause us to write some data that's corrupt,
or alternatively will cause us to error out when trying to seek to the
trailer line (at the end of the revfile).

This might make sense for APR 1.x, but I'm surprised that APR doesn't
just pass O_LARGEFILE unconditionally now - why is that?

Regards,
Malcolm

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] APR_LARGEFILE and svn_io_file_open

[David James <dj...@collab.net>]

On 2/7/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> So I got to thinking, why not just have svn_io_file_open tack it on to
> flags all the time, just like we do with APR_BINARY

In July 2005, Bryn Davies wrote a similar patch, which also updates
our test suite with APR_LARGEFILE flags. Please see
http://svn.haxx.se/users/archive-2005-06/0414.shtml and check whether
any of his additional changes should be included in your patch.

Also: Perhaps it'd be a good idea to conditionalize your patch to only
affect APR 1.x and later? If we add this flag for APR 0.9.x, I suspect
that FSFS might happily create files over 2GB, but then corrupt them.
Read what Greg Hudson said below.

On May 31, 2005, at 11:45 AM, Greg Hudson wrote:
> On Tue, 2005-05-31 at 11:18 -0500, Ben Collins-Sussman wrote:
> > In any case: I'm wondering if we should be passing APR_LARGEFILE to
> > all apr_file_io calls. Is it necessary? Should we expect problems
> > if we don't?
> Passing APR_LARGEFILE does not magically change the size of apr_off_t;
> it does not gain us the ability to seek around in >2GB files. It's
> essentially a promise to APR that we don't need to seek, so it's okay to
> go above the 2GB limit.
>
> However, we most definitely do need to seek around in FSFS rev files,
> which are the most common thing to go above 2GB. APR_LARGEFILE won't
> help us for that case.
Source: http://svn.haxx.se/dev/archive-2005-05/1412.shtml

Cheers,

David

--
David James -- http://www.cs.toronto.edu/~james

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org