On Wed, Mar 06, 2024 at 06:49:11PM +0100, Jan Kara wrote:
> 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@xxxxxxxxxx>
> > >
> > > ---
> > > 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
I don't know.
> fallocate(2) with any signal but since tmpfs is the only filesystem having
Hugetlbfs has the same logic.
> 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.
Right now fallocate() is restartable. You could get EINTR and then
retry. Changing this to fatal_signal_pending() would mean that this
property is lost. The task will have to be wiped.
If this is only done for the sake of the OOM killer then we can probably
try and change it. But then we'd need to also reflect that on the
manpage.
