Re: [xserver-xorg][PATCH 1/1] xwayland: ftruncate if posix_fallocate fails

[Pekka Paalanen]

On Tue, 26 Apr 2016 16:46:03 +0200 (CEST)
Mark Kettenis  wrote:

> > Date: Tue, 26 Apr 2016 15:58:47 +0300
> > From: Ian Ray 
> > 
> > On Mon, Apr 25, 2016 at 04:49:07PM +0300, Pekka Paalanen wrote:  
> > > On Mon, 25 Apr 2016 15:47:09 +0300
> > > Ian Ray  wrote:
> > >   
> > > > On a slow system that is configured with SMART_SCHEDULE_POSSIBLE, large
> > > > posix_fallocate() requests may be interrupted by the SmartScheduleTimer
> > > > (SIGALRM) continuously. Fallback to ftruncate if posix_fallocate fails.
> > > > 
> > > > Signed-off-by: Ian Ray 
> > > > ---
> > > >  hw/xwayland/xwayland-shm.c | 9 ++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/hw/xwayland/xwayland-shm.c b/hw/xwayland/xwayland-shm.c
> > > > index e8545b3..342b723 100644
> > > > --- a/hw/xwayland/xwayland-shm.c
> > > > +++ b/hw/xwayland/xwayland-shm.c
> > > > @@ -142,9 +142,12 @@ os_create_anonymous_file(off_t size)
> > > >  #ifdef HAVE_POSIX_FALLOCATE
> > > >  ret = posix_fallocate(fd, 0, size);
> > > >  if (ret != 0) {
> > > > -close(fd);
> > > > -errno = ret;
> > > > -return -1;
> > > > +/* Fallback to ftruncate in case of failure. */
> > > > +ret = ftruncate(fd, size);
> > > > +if (ret < 0) {
> > > > +close(fd);
> > > > +return -1;
> > > > +}
> > > >  }
> > > >  #else
> > > >  ret = ftruncate(fd, size);  
> > > 
> > > Hi Ian,
> > > 
> > > I indeed think this is a bit harsh. The first EINTR may happen on any
> > > system, so the very least we should try twice before giving up.
> > > 
> > > I'd also like to see some comments on whether fallocate making no
> > > progress when repeatedly restarted is normal or not. Maybe that should
> > > be asked on a kernel list?
> > > 
> > > 
> > > Thanks,
> > > pq  
> > 
> > Hi Pekka,
> > 
> > Referring to  http://lxr.free-electrons.com/source/mm/shmem.c#L2235 and 
> > http://lkml.iu.edu/hypermail/linux/kernel/1603.0/03523.html,  fallocate
> > is _not_ expected to make progress when repeatedly restarted.  
> 
> The crucial bit being that it does an explicit rollback if it gets
> EINTR.  Not very well thought out if you ask me (it is as if an
> interrupted write call would return EINTR and not make progress), but
> probably something that is cast into stone and cannot be fixed
> anymore.
> 
> That last message you cite is interesting.  POSIX states that system
> calls return EINTR should be automatically restarted for signals that
> have the SA_RESTART flag set.  So that means that the linux kernel
> should indeed return -ERESTARTSYS here.  But since the Xorg smart
> scheduler code sets SA_RESTART, that means you'd get an infinite loop
> on the systems where (allegedly) posix_fallocate() never completes in
> presence of the smart scheduler's repeated SIGALRM.
> 
> > Taking Yuriy's reply in to account, then there seems to be are a couple
> > of options:
> > 
> > 1. try fallocate() a couple of times and in case of EINTR fallback
> >to ftruncate
> > 
> > 2. mask SIGALRM while calling fallocate  
> 
> That probably is the right thing to do here.  Having the smart
> scheduler miss a "tick" isn't a disaster, and the other suggestions
> (looping while EINTR is returned, looping over smaller chunks) would
> block the request just as well.
> 
> Or perhaps take a step back and ask yourselves if using
> posix_fallocate() here is the right thing to do in the first place.
> SIGBUS really can't be prevented as a malicious client can still
> ftruncate() the file descriptor.  On OpenBSD I implemented a mmap
> option that prevents SIGBUS on access of the memory even if the
> request for backing store cannot be fulfilled.  But that isn't
> portable of course.

Hi,

here Xwayland is the client, that is creating the file in the first
place.

The point here is to avoid SIGBUS because of running out of tmpfs
space, not to protect against malice. Whether that is worth is another
question. I'd like to keep it as far as possible, until fallocate
simply doesn't work anymore.

If blocking the signal has no bad effects, that seems like the desired
solution to me. After all, the fallocated memory is going to be used
soon anyway, I believe, so need to be paged in and all.

We may still need the restart in case other signals arrive, right?


Thanks,
pq


pgpW_Hv4MGFBk.pgp
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [xserver-xorg][PATCH 1/1] xwayland: ftruncate if posix_fallocate fails

[Mark Kettenis]

> Date: Tue, 26 Apr 2016 15:58:47 +0300
> From: Ian Ray 
> 
> On Mon, Apr 25, 2016 at 04:49:07PM +0300, Pekka Paalanen wrote:
> > On Mon, 25 Apr 2016 15:47:09 +0300
> > Ian Ray  wrote:
> > 
> > > On a slow system that is configured with SMART_SCHEDULE_POSSIBLE, large
> > > posix_fallocate() requests may be interrupted by the SmartScheduleTimer
> > > (SIGALRM) continuously. Fallback to ftruncate if posix_fallocate fails.
> > > 
> > > Signed-off-by: Ian Ray 
> > > ---
> > >  hw/xwayland/xwayland-shm.c | 9 ++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/xwayland/xwayland-shm.c b/hw/xwayland/xwayland-shm.c
> > > index e8545b3..342b723 100644
> > > --- a/hw/xwayland/xwayland-shm.c
> > > +++ b/hw/xwayland/xwayland-shm.c
> > > @@ -142,9 +142,12 @@ os_create_anonymous_file(off_t size)
> > >  #ifdef HAVE_POSIX_FALLOCATE
> > >  ret = posix_fallocate(fd, 0, size);
> > >  if (ret != 0) {
> > > -close(fd);
> > > -errno = ret;
> > > -return -1;
> > > +/* Fallback to ftruncate in case of failure. */
> > > +ret = ftruncate(fd, size);
> > > +if (ret < 0) {
> > > +close(fd);
> > > +return -1;
> > > +}
> > >  }
> > >  #else
> > >  ret = ftruncate(fd, size);
> > 
> > Hi Ian,
> > 
> > I indeed think this is a bit harsh. The first EINTR may happen on any
> > system, so the very least we should try twice before giving up.
> > 
> > I'd also like to see some comments on whether fallocate making no
> > progress when repeatedly restarted is normal or not. Maybe that should
> > be asked on a kernel list?
> > 
> > 
> > Thanks,
> > pq
> 
> Hi Pekka,
> 
> Referring to  http://lxr.free-electrons.com/source/mm/shmem.c#L2235 and 
> http://lkml.iu.edu/hypermail/linux/kernel/1603.0/03523.html,  fallocate
> is _not_ expected to make progress when repeatedly restarted.

The crucial bit being that it does an explicit rollback if it gets
EINTR.  Not very well thought out if you ask me (it is as if an
interrupted write call would return EINTR and not make progress), but
probably something that is cast into stone and cannot be fixed
anymore.

That last message you cite is interesting.  POSIX states that system
calls return EINTR should be automatically restarted for signals that
have the SA_RESTART flag set.  So that means that the linux kernel
should indeed return -ERESTARTSYS here.  But since the Xorg smart
scheduler code sets SA_RESTART, that means you'd get an infinite loop
on the systems where (allegedly) posix_fallocate() never completes in
presence of the smart scheduler's repeated SIGALRM.

> Taking Yuriy's reply in to account, then there seems to be are a couple
> of options:
> 
> 1. try fallocate() a couple of times and in case of EINTR fallback
>to ftruncate
> 
> 2. mask SIGALRM while calling fallocate

That probably is the right thing to do here.  Having the smart
scheduler miss a "tick" isn't a disaster, and the other suggestions
(looping while EINTR is returned, looping over smaller chunks) would
block the request just as well.

Or perhaps take a step back and ask yourselves if using
posix_fallocate() here is the right thing to do in the first place.
SIGBUS really can't be prevented as a malicious client can still
ftruncate() the file descriptor.  On OpenBSD I implemented a mmap
option that prevents SIGBUS on access of the memory even if the
request for backing store cannot be fulfilled.  But that isn't
portable of course.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [xserver-xorg][PATCH 1/1] xwayland: ftruncate if posix_fallocate fails

[Pekka Paalanen]

On Tue, 26 Apr 2016 15:58:47 +0300
Ian Ray  wrote:

> On Mon, Apr 25, 2016 at 04:49:07PM +0300, Pekka Paalanen wrote:
> > On Mon, 25 Apr 2016 15:47:09 +0300
> > Ian Ray  wrote:
> >   
> > > On a slow system that is configured with SMART_SCHEDULE_POSSIBLE, large
> > > posix_fallocate() requests may be interrupted by the SmartScheduleTimer
> > > (SIGALRM) continuously. Fallback to ftruncate if posix_fallocate fails.
> > > 
> > > Signed-off-by: Ian Ray 
> > > ---
> > >  hw/xwayland/xwayland-shm.c | 9 ++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/xwayland/xwayland-shm.c b/hw/xwayland/xwayland-shm.c
> > > index e8545b3..342b723 100644
> > > --- a/hw/xwayland/xwayland-shm.c
> > > +++ b/hw/xwayland/xwayland-shm.c
> > > @@ -142,9 +142,12 @@ os_create_anonymous_file(off_t size)
> > >  #ifdef HAVE_POSIX_FALLOCATE
> > >  ret = posix_fallocate(fd, 0, size);
> > >  if (ret != 0) {
> > > -close(fd);
> > > -errno = ret;
> > > -return -1;
> > > +/* Fallback to ftruncate in case of failure. */
> > > +ret = ftruncate(fd, size);
> > > +if (ret < 0) {
> > > +close(fd);
> > > +return -1;
> > > +}
> > >  }
> > >  #else
> > >  ret = ftruncate(fd, size);  
> > 
> > Hi Ian,
> > 
> > I indeed think this is a bit harsh. The first EINTR may happen on any
> > system, so the very least we should try twice before giving up.
> > 
> > I'd also like to see some comments on whether fallocate making no
> > progress when repeatedly restarted is normal or not. Maybe that should
> > be asked on a kernel list?
> > 
> > 
> > Thanks,
> > pq  
> 
> Hi Pekka,
> 
> Referring to  http://lxr.free-electrons.com/source/mm/shmem.c#L2235 and 
> http://lkml.iu.edu/hypermail/linux/kernel/1603.0/03523.html,  fallocate
> is _not_ expected to make progress when repeatedly restarted.

Hi Ian,

that's sad.

> Taking Yuriy's reply in to account, then there seems to be are a couple
> of options:
> 
> 1. try fallocate() a couple of times and in case of EINTR fallback
>to ftruncate
> 
> 2. mask SIGALRM while calling fallocate

3. After failing with EINTR twice, fall back to fallocate() in small
chunks.

4. Launch Xwayland with -dumbSched. (Not sure if 1.18 series had the
latest patches affecting it, though.)

> Any preference?

I don't know what Xwayland does with SIGALRM to say, it's such a
pathological case. I'm hoping an Xorg dev can recommend something.

I suppose I could go with option 1. I'd expect it to be able to find
out if there is enough free space or not on most systems before it
would fall back to fallocate.

> The time taken, in seconds, by fallocate() for various lengths is shown
> below (measured on iMX6 dual-core system).  Average of 10 measurements.
> 
> len1024, min 0.90, max 0.000113, mean 0.95, stddev 0.06
> len2048, min 0.90, max 0.97, mean 0.93, stddev 0.03
> len4096, min 0.91, max 0.000542, mean 0.000139, stddev 0.000134
> len8192, min 0.000134, max 0.000141, mean 0.000136, stddev 0.02
> len   16384, min 0.000207, max 0.001136, mean 0.000305, stddev 0.000277
> len   32768, min 0.000189, max 0.001858, mean 0.000418, stddev 0.000488
> len   65536, min 0.000353, max 0.000369, mean 0.000360, stddev 0.06
> len  131072, min 0.000672, max 0.000691, mean 0.000678, stddev 0.05
> len  262144, min 0.001314, max 0.001416, mean 0.001337, stddev 0.28
> len  524288, min 0.002621, max 0.003185, mean 0.002698, stddev 0.000164
> len 1048576, min 0.004323, max 0.006196, mean 0.004679, stddev 0.000615
> len 2097152, min 0.008710, max 0.009571, mean 0.008841, stddev 0.000246
> len 4194304, min 0.017518, max 0.017709, mean 0.017625, stddev 0.70

I wonder if those numbers are typical for iMX6. They are so high.
Almost makes me want to make a comparison with RaspberryPi 1 to get
some feeling of what a slow system is.


Thanks,
pq


pgp5qDO5xo_Sz.pgp
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [xserver-xorg][PATCH 1/1] xwayland: ftruncate if posix_fallocate fails

[Ian Ray]

On Mon, Apr 25, 2016 at 04:49:07PM +0300, Pekka Paalanen wrote:
> On Mon, 25 Apr 2016 15:47:09 +0300
> Ian Ray  wrote:
> 
> > On a slow system that is configured with SMART_SCHEDULE_POSSIBLE, large
> > posix_fallocate() requests may be interrupted by the SmartScheduleTimer
> > (SIGALRM) continuously. Fallback to ftruncate if posix_fallocate fails.
> > 
> > Signed-off-by: Ian Ray 
> > ---
> >  hw/xwayland/xwayland-shm.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/xwayland/xwayland-shm.c b/hw/xwayland/xwayland-shm.c
> > index e8545b3..342b723 100644
> > --- a/hw/xwayland/xwayland-shm.c
> > +++ b/hw/xwayland/xwayland-shm.c
> > @@ -142,9 +142,12 @@ os_create_anonymous_file(off_t size)
> >  #ifdef HAVE_POSIX_FALLOCATE
> >  ret = posix_fallocate(fd, 0, size);
> >  if (ret != 0) {
> > -close(fd);
> > -errno = ret;
> > -return -1;
> > +/* Fallback to ftruncate in case of failure. */
> > +ret = ftruncate(fd, size);
> > +if (ret < 0) {
> > +close(fd);
> > +return -1;
> > +}
> >  }
> >  #else
> >  ret = ftruncate(fd, size);
> 
> Hi Ian,
> 
> I indeed think this is a bit harsh. The first EINTR may happen on any
> system, so the very least we should try twice before giving up.
> 
> I'd also like to see some comments on whether fallocate making no
> progress when repeatedly restarted is normal or not. Maybe that should
> be asked on a kernel list?
> 
> 
> Thanks,
> pq

Hi Pekka,

Referring to  http://lxr.free-electrons.com/source/mm/shmem.c#L2235 and 
http://lkml.iu.edu/hypermail/linux/kernel/1603.0/03523.html,  fallocate
is _not_ expected to make progress when repeatedly restarted.

Taking Yuriy's reply in to account, then there seems to be are a couple
of options:

1. try fallocate() a couple of times and in case of EINTR fallback
   to ftruncate

2. mask SIGALRM while calling fallocate

Any preference?


The time taken, in seconds, by fallocate() for various lengths is shown
below (measured on iMX6 dual-core system).  Average of 10 measurements.

len1024, min 0.90, max 0.000113, mean 0.95, stddev 0.06
len2048, min 0.90, max 0.97, mean 0.93, stddev 0.03
len4096, min 0.91, max 0.000542, mean 0.000139, stddev 0.000134
len8192, min 0.000134, max 0.000141, mean 0.000136, stddev 0.02
len   16384, min 0.000207, max 0.001136, mean 0.000305, stddev 0.000277
len   32768, min 0.000189, max 0.001858, mean 0.000418, stddev 0.000488
len   65536, min 0.000353, max 0.000369, mean 0.000360, stddev 0.06
len  131072, min 0.000672, max 0.000691, mean 0.000678, stddev 0.05
len  262144, min 0.001314, max 0.001416, mean 0.001337, stddev 0.28
len  524288, min 0.002621, max 0.003185, mean 0.002698, stddev 0.000164
len 1048576, min 0.004323, max 0.006196, mean 0.004679, stddev 0.000615
len 2097152, min 0.008710, max 0.009571, mean 0.008841, stddev 0.000246
len 4194304, min 0.017518, max 0.017709, mean 0.017625, stddev 0.70

Blue skies,
Ian

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [xserver-xorg][PATCH 1/1] xwayland: ftruncate if posix_fallocate fails

[Yuriy M. Kaminskiy]

Ian Ray  writes:

> On a slow system that is configured with SMART_SCHEDULE_POSSIBLE, large
> posix_fallocate() requests may be interrupted by the SmartScheduleTimer
> (SIGALRM) continuously. Fallback to ftruncate if posix_fallocate fails.
>
> Signed-off-by: Ian Ray 
> ---
>  hw/xwayland/xwayland-shm.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-shm.c b/hw/xwayland/xwayland-shm.c
> index e8545b3..342b723 100644
> --- a/hw/xwayland/xwayland-shm.c
> +++ b/hw/xwayland/xwayland-shm.c
> @@ -142,9 +142,12 @@ os_create_anonymous_file(off_t size)
>  #ifdef HAVE_POSIX_FALLOCATE
>  ret = posix_fallocate(fd, 0, size);
>  if (ret != 0) {
> -close(fd);
> -errno = ret;
> -return -1;

But posix_fallocate() was used here for a reason?

When you use ftruncate(), it creates file with hole. If you mmap() file
with hole, and there will be no disk space at the moment page is
touched, it will trigger SIGBUS.

And this fallback is *especially* bad if posix_fallocate() returned ENOSPACE
or EIO.

> +/* Fallback to ftruncate in case of failure. */
> +ret = ftruncate(fd, size);
> +if (ret < 0) {
> +close(fd);
> +return -1;
> +}
>  }
>  #else
>  ret = ftruncate(fd, size);

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [xserver-xorg][PATCH 1/1] xwayland: ftruncate if posix_fallocate fails

[Pekka Paalanen]

On Mon, 25 Apr 2016 15:47:09 +0300
Ian Ray  wrote:

> On a slow system that is configured with SMART_SCHEDULE_POSSIBLE, large
> posix_fallocate() requests may be interrupted by the SmartScheduleTimer
> (SIGALRM) continuously. Fallback to ftruncate if posix_fallocate fails.
> 
> Signed-off-by: Ian Ray 
> ---
>  hw/xwayland/xwayland-shm.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/xwayland/xwayland-shm.c b/hw/xwayland/xwayland-shm.c
> index e8545b3..342b723 100644
> --- a/hw/xwayland/xwayland-shm.c
> +++ b/hw/xwayland/xwayland-shm.c
> @@ -142,9 +142,12 @@ os_create_anonymous_file(off_t size)
>  #ifdef HAVE_POSIX_FALLOCATE
>  ret = posix_fallocate(fd, 0, size);
>  if (ret != 0) {
> -close(fd);
> -errno = ret;
> -return -1;
> +/* Fallback to ftruncate in case of failure. */
> +ret = ftruncate(fd, size);
> +if (ret < 0) {
> +close(fd);
> +return -1;
> +}
>  }
>  #else
>  ret = ftruncate(fd, size);

Hi Ian,

I indeed think this is a bit harsh. The first EINTR may happen on any
system, so the very least we should try twice before giving up.

I'd also like to see some comments on whether fallocate making no
progress when repeatedly restarted is normal or not. Maybe that should
be asked on a kernel list?


Thanks,
pq


pgpPMWqjYXfNO.pgp
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[xserver-xorg][PATCH 1/1] xwayland: ftruncate if posix_fallocate fails

[Ian Ray]

On a slow system that is configured with SMART_SCHEDULE_POSSIBLE, large
posix_fallocate() requests may be interrupted by the SmartScheduleTimer
(SIGALRM) continuously. Fallback to ftruncate if posix_fallocate fails.

Signed-off-by: Ian Ray 
---
 hw/xwayland/xwayland-shm.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/xwayland/xwayland-shm.c b/hw/xwayland/xwayland-shm.c
index e8545b3..342b723 100644
--- a/hw/xwayland/xwayland-shm.c
+++ b/hw/xwayland/xwayland-shm.c
@@ -142,9 +142,12 @@ os_create_anonymous_file(off_t size)
 #ifdef HAVE_POSIX_FALLOCATE
 ret = posix_fallocate(fd, 0, size);
 if (ret != 0) {
-close(fd);
-errno = ret;
-return -1;
+/* Fallback to ftruncate in case of failure. */
+ret = ftruncate(fd, size);
+if (ret < 0) {
+close(fd);
+return -1;
+}
 }
 #else
 ret = ftruncate(fd, size);
-- 
2.4.5

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel