[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 <ian.ray at ge.com>
> 
> On Mon, Apr 25, 2016 at 04:49:07PM +0300, Pekka Paalanen wrote:
> > On Mon, 25 Apr 2016 15:47:09 +0300
> > Ian Ray <ian.ray at ge.com> 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 <ian.ray at ge.com>
> > > ---
> > >  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.