Re: [PATCH] tmpfs: don't interrupt fallocate with EINTR

[Jan Kara <jack@suse.cz>]

Hello,

On Tue 05-03-24 09:42:27, Christian Brauner wrote:
> On Mon, Mar 04, 2024 at 07:43:39PM +0100, Mikulas Patocka wrote:
> > I have a program that sets up a periodic timer with 10ms interval. When
> > the program attempts to call fallocate on tmpfs, it goes into an infinite
> > loop. fallocate takes longer than 10ms, so it gets interrupted by a
> > signal and it returns EINTR. On EINTR, the fallocate call is restarted,
> > going into the same loop again.
> > 
> > fallocate(19, FALLOC_FL_KEEP_SIZE, 0, 206057565) = -1 EINTR (Přerušené volání systému)
> > --- SIGALRM {si_signo=SIGALRM, si_code=SI_TIMER, si_timerid=0, si_overrun=0, si_int=0, si_ptr=NULL} ---
> > sigreturn({mask=[]})                    = -1 EINTR (Přerušené volání systému)
> > fallocate(19, FALLOC_FL_KEEP_SIZE, 0, 206057565) = -1 EINTR (Přerušené volání systému)
> > --- SIGALRM {si_signo=SIGALRM, si_code=SI_TIMER, si_timerid=0, si_overrun=0, si_int=0, si_ptr=NULL} ---
> > sigreturn({mask=[]})                    = -1 EINTR (Přerušené volání systému)
> > fallocate(19, FALLOC_FL_KEEP_SIZE, 0, 206057565) = -1 EINTR (Přerušené volání systému)
> > --- SIGALRM {si_signo=SIGALRM, si_code=SI_TIMER, si_timerid=0, si_overrun=0, si_int=0, si_ptr=NULL} ---
> > sigreturn({mask=[]})                    = -1 EINTR (Přerušené volání systému)
> > fallocate(19, FALLOC_FL_KEEP_SIZE, 0, 206057565) = -1 EINTR (Přerušené volání systému)
> > --- SIGALRM {si_signo=SIGALRM, si_code=SI_TIMER, si_timerid=0, si_overrun=0, si_int=0, si_ptr=NULL} ---
> > sigreturn({mask=[]})                    = -1 EINTR (Přerušené volání systému)
> > 
> > Should there be fatal_signal_pending instead of signal_pending in the
> > shmem_fallocate loop?
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > ---
> >  mm/shmem.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Index: linux-2.6/mm/shmem.c
> > ===================================================================
> > --- linux-2.6.orig/mm/shmem.c	2024-01-18 19:18:31.000000000 +0100
> > +++ linux-2.6/mm/shmem.c	2024-03-04 19:05:25.000000000 +0100
> > @@ -3143,7 +3143,7 @@ static long shmem_fallocate(struct file
> >  		 * Good, the fallocate(2) manpage permits EINTR: we may have
> >  		 * been interrupted because we are using up too much memory.
> >  		 */
> > -		if (signal_pending(current))
> > +		if (fatal_signal_pending(current))
> 
> I think that's likely wrong and probably would cause regressions as
> there may be users relying on this?

I understand your concern about userspace regressions but is the EINTR
behavior that useful? Sure, something can be relying on terminating
fallocate(2) with any signal but since tmpfs is the only filesystem having
this behavior, it is fair to say there are even higher chances some
application will be surprised by this behavior when used on tmpfs as
Mikulas was? So I wouldn't be that opposed to this change. *But* tmpfs has
a comment explaining the signal_pending() check:

                /*
                 * Good, the fallocate(2) manpage permits EINTR: we may have
                 * been interrupted because we are using up too much memory.
                 */

Now I'd expect the signal to be fatal in this case but we definitely need
to make sure this is the case if we want to consider changing the test.
Hugh?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR