[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 <mark.kettenis at xs4all.nl> wrote:

> > 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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160426/60549c49/attachment.sig>