[PATCH] btrfs: don't return EINTR

[PATCH] btrfs: don't return EINTR

[Arne Jansen]

It is basically a good thing if we are interruptible when waiting for
free space, but the generality in which it is implemented currently
leads to system calls being interruptible that are not documented this
way. For example git can't handle interrupted unlink(), leading to
corrupt repos under space pressure.

Signed-off-by: Arne Jansen <sensille@gmx.net>
---
 fs/btrfs/extent-tree.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a844204..db13e51 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3772,14 +3772,7 @@ again:
 		 */
 		if (current->journal_info)
 			return -EAGAIN;
-		ret = wait_event_interruptible(space_info->wait,
-					       !space_info->flush);
-		/* Must have been interrupted, return */
-		if (ret) {
-			printk(KERN_DEBUG "btrfs: %s returning -EINTR\n", __func__);
-			return -EINTR;
-		}
-
+		wait_event(space_info->wait, !space_info->flush);
 		spin_lock(&space_info->lock);
 	}
 
-- 
1.7.3.4

Re: [PATCH] btrfs: don't return EINTR

[Arne Jansen]

On 14.04.2012 14:56, Arne Jansen wrote:
> It is basically a good thing if we are interruptible when waiting for
> free space, but the generality in which it is implemented currently
> leads to system calls being interruptible that are not documented this
> way. For example git can't handle interrupted unlink(), leading to
> corrupt repos under space pressure.

Is this patch a candidate for the next rc?

-Arne

> 
> Signed-off-by: Arne Jansen <sensille@gmx.net>
> ---
>  fs/btrfs/extent-tree.c |    9 +--------
>  1 files changed, 1 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index a844204..db13e51 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3772,14 +3772,7 @@ again:
>  		 */
>  		if (current->journal_info)
>  			return -EAGAIN;
> -		ret = wait_event_interruptible(space_info->wait,
> -					       !space_info->flush);
> -		/* Must have been interrupted, return */
> -		if (ret) {
> -			printk(KERN_DEBUG "btrfs: %s returning -EINTR\n", __func__);
> -			return -EINTR;
> -		}
> -
> +		wait_event(space_info->wait, !space_info->flush);
>  		spin_lock(&space_info->lock);
>  	}
>  

Re: [PATCH] btrfs: don't return EINTR

[Chris Mason]

On Tue, Apr 17, 2012 at 04:15:32PM +0200, Arne Jansen wrote:
> On 14.04.2012 14:56, Arne Jansen wrote:
> > It is basically a good thing if we are interruptible when waiting for
> > free space, but the generality in which it is implemented currently
> > leads to system calls being interruptible that are not documented this
> > way. For example git can't handle interrupted unlink(), leading to
> > corrupt repos under space pressure.
> 
> Is this patch a candidate for the next rc?

The EINTR came from Josef.  We do want to be able to break out of long
flushes, but I want to check with him to see if there was a specific bug
this was solving?

-chris

Re: [PATCH] btrfs: don't return EINTR

[Josef Bacik]

On Tue, Apr 17, 2012 at 11:24:01AM -0400, Chris Mason wrote:
> On Tue, Apr 17, 2012 at 04:15:32PM +0200, Arne Jansen wrote:
> > On 14.04.2012 14:56, Arne Jansen wrote:
> > > It is basically a good thing if we are interruptible when waiting for
> > > free space, but the generality in which it is implemented currently
> > > leads to system calls being interruptible that are not documented this
> > > way. For example git can't handle interrupted unlink(), leading to
> > > corrupt repos under space pressure.
> > 
> > Is this patch a candidate for the next rc?
> 
> The EINTR came from Josef.  We do want to be able to break out of long
> flushes, but I want to check with him to see if there was a specific bug
> this was solving?

Sorry I was -ENOINTERNET, no the only thing I was fixing was being able to break
out of long flushes.  Maybe instead of using the big hammer here we just make
unlink ignore EINTR and try again, or maybe pass down a flag saying I can't be
interrupted?  Thanks,

Josef 

Re: [PATCH] btrfs: don't return EINTR

[Arne Jansen]

On 04/17/12 20:22, Josef Bacik wrote:
> On Tue, Apr 17, 2012 at 11:24:01AM -0400, Chris Mason wrote:
>> On Tue, Apr 17, 2012 at 04:15:32PM +0200, Arne Jansen wrote:
>>> On 14.04.2012 14:56, Arne Jansen wrote:
>>>> It is basically a good thing if we are interruptible when waiting for
>>>> free space, but the generality in which it is implemented currently
>>>> leads to system calls being interruptible that are not documented this
>>>> way. For example git can't handle interrupted unlink(), leading to
>>>> corrupt repos under space pressure.
>>>
>>> Is this patch a candidate for the next rc?
>>
>> The EINTR came from Josef.  We do want to be able to break out of long
>> flushes, but I want to check with him to see if there was a specific bug
>> this was solving?
>
> Sorry I was -ENOINTERNET, no the only thing I was fixing was being able to break
> out of long flushes.  Maybe instead of using the big hammer here we just make
> unlink ignore EINTR and try again, or maybe pass down a flag saying I can't be
> interrupted?  Thanks,
>

unlink() is the only call I've seen problems with, but there are
probably other calls where EINTR is also unexpected.
Also, just retrying the unlink internally won't help as the signal
is still pending.
How can we gather a list of calls where EINTR is ok?

-Arne

> Josef
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: don't return EINTR

[Josef Bacik]

On Tue, Apr 17, 2012 at 08:56:11PM +0200, Arne Jansen wrote:
> On 04/17/12 20:22, Josef Bacik wrote:
> >On Tue, Apr 17, 2012 at 11:24:01AM -0400, Chris Mason wrote:
> >>On Tue, Apr 17, 2012 at 04:15:32PM +0200, Arne Jansen wrote:
> >>>On 14.04.2012 14:56, Arne Jansen wrote:
> >>>>It is basically a good thing if we are interruptible when waiting for
> >>>>free space, but the generality in which it is implemented currently
> >>>>leads to system calls being interruptible that are not documented this
> >>>>way. For example git can't handle interrupted unlink(), leading to
> >>>>corrupt repos under space pressure.
> >>>
> >>>Is this patch a candidate for the next rc?
> >>
> >>The EINTR came from Josef.  We do want to be able to break out of long
> >>flushes, but I want to check with him to see if there was a specific bug
> >>this was solving?
> >
> >Sorry I was -ENOINTERNET, no the only thing I was fixing was being able to break
> >out of long flushes.  Maybe instead of using the big hammer here we just make
> >unlink ignore EINTR and try again, or maybe pass down a flag saying I can't be
> >interrupted?  Thanks,
> >
> 
> unlink() is the only call I've seen problems with, but there are
> probably other calls where EINTR is also unexpected.
> Also, just retrying the unlink internally won't help as the signal
> is still pending.
> How can we gather a list of calls where EINTR is ok?

Well then passing a flag down that says we can't interrupt I guess is what we're
going to have to do and just wait uninterruptible.  I think our best bet is to
just fix them as they come up, I thought all system calls could return EINTR but
apparently I was wrong :).  Thanks,

Josef

Re: [PATCH] btrfs: don't return EINTR

[Chris Mason]

On Tue, Apr 17, 2012 at 03:14:24PM -0400, Josef Bacik wrote:
> On Tue, Apr 17, 2012 at 08:56:11PM +0200, Arne Jansen wrote:
> > On 04/17/12 20:22, Josef Bacik wrote:
> > >On Tue, Apr 17, 2012 at 11:24:01AM -0400, Chris Mason wrote:
> > >>On Tue, Apr 17, 2012 at 04:15:32PM +0200, Arne Jansen wrote:
> > >>>On 14.04.2012 14:56, Arne Jansen wrote:
> > >>>>It is basically a good thing if we are interruptible when waiting for
> > >>>>free space, but the generality in which it is implemented currently
> > >>>>leads to system calls being interruptible that are not documented this
> > >>>>way. For example git can't handle interrupted unlink(), leading to
> > >>>>corrupt repos under space pressure.
> > >>>
> > >>>Is this patch a candidate for the next rc?
> > >>
> > >>The EINTR came from Josef.  We do want to be able to break out of long
> > >>flushes, but I want to check with him to see if there was a specific bug
> > >>this was solving?
> > >
> > >Sorry I was -ENOINTERNET, no the only thing I was fixing was being able to break
> > >out of long flushes.  Maybe instead of using the big hammer here we just make
> > >unlink ignore EINTR and try again, or maybe pass down a flag saying I can't be
> > >interrupted?  Thanks,
> > >
> > 
> > unlink() is the only call I've seen problems with, but there are
> > probably other calls where EINTR is also unexpected.
> > Also, just retrying the unlink internally won't help as the signal
> > is still pending.
> > How can we gather a list of calls where EINTR is ok?
> 
> Well then passing a flag down that says we can't interrupt I guess is what we're
> going to have to do and just wait uninterruptible.  I think our best bet is to
> just fix them as they come up, I thought all system calls could return EINTR but
> apparently I was wrong :).  Thanks,

I'd guess that EINTR is unexpected most of the time.  Including in reads
and writes.  The real question is how long we might end up waiting?

-chris

Re: [PATCH] btrfs: don't return EINTR

[Josef Bacik]

On Tue, Apr 17, 2012 at 03:34:17PM -0400, Chris Mason wrote:
> On Tue, Apr 17, 2012 at 03:14:24PM -0400, Josef Bacik wrote:
> > On Tue, Apr 17, 2012 at 08:56:11PM +0200, Arne Jansen wrote:
> > > On 04/17/12 20:22, Josef Bacik wrote:
> > > >On Tue, Apr 17, 2012 at 11:24:01AM -0400, Chris Mason wrote:
> > > >>On Tue, Apr 17, 2012 at 04:15:32PM +0200, Arne Jansen wrote:
> > > >>>On 14.04.2012 14:56, Arne Jansen wrote:
> > > >>>>It is basically a good thing if we are interruptible when waiting for
> > > >>>>free space, but the generality in which it is implemented currently
> > > >>>>leads to system calls being interruptible that are not documented this
> > > >>>>way. For example git can't handle interrupted unlink(), leading to
> > > >>>>corrupt repos under space pressure.
> > > >>>
> > > >>>Is this patch a candidate for the next rc?
> > > >>
> > > >>The EINTR came from Josef.  We do want to be able to break out of long
> > > >>flushes, but I want to check with him to see if there was a specific bug
> > > >>this was solving?
> > > >
> > > >Sorry I was -ENOINTERNET, no the only thing I was fixing was being able to break
> > > >out of long flushes.  Maybe instead of using the big hammer here we just make
> > > >unlink ignore EINTR and try again, or maybe pass down a flag saying I can't be
> > > >interrupted?  Thanks,
> > > >
> > > 
> > > unlink() is the only call I've seen problems with, but there are
> > > probably other calls where EINTR is also unexpected.
> > > Also, just retrying the unlink internally won't help as the signal
> > > is still pending.
> > > How can we gather a list of calls where EINTR is ok?
> > 
> > Well then passing a flag down that says we can't interrupt I guess is what we're
> > going to have to do and just wait uninterruptible.  I think our best bet is to
> > just fix them as they come up, I thought all system calls could return EINTR but
> > apparently I was wrong :).  Thanks,
> 
> I'd guess that EINTR is unexpected most of the time.  Including in reads
> and writes.  The real question is how long we might end up waiting?
> 

EINTR is valid for both reads and writes.  This was put into place when I would
run tests and get tired of waiting for them so I'd ctrl+c and it wouldn't stop
even though it's something that's completely stoppable.  So I'd like to leave it
in there so at the very least I can still ctrl+c when I accidently run something
I don't want to run ;).  Thanks,

Josef

Re: [PATCH] btrfs: don't return EINTR

[Chris Mason]

On Tue, Apr 17, 2012 at 03:36:20PM -0400, Josef Bacik wrote:
> > > Well then passing a flag down that says we can't interrupt I guess is what we're
> > > going to have to do and just wait uninterruptible.  I think our best bet is to
> > > just fix them as they come up, I thought all system calls could return EINTR but
> > > apparently I was wrong :).  Thanks,
> > 
> > I'd guess that EINTR is unexpected most of the time.  Including in reads
> > and writes.  The real question is how long we might end up waiting?
> > 
> 
> EINTR is valid for both reads and writes.  This was put into place when I would
> run tests and get tired of waiting for them so I'd ctrl+c and it wouldn't stop
> even though it's something that's completely stoppable.  So I'd like to leave it
> in there so at the very least I can still ctrl+c when I accidently run something
> I don't want to run ;).  Thanks,

Ok. lets just teach git how to eintr.


-chris

Re: [PATCH] btrfs: don't return EINTR

[Arne Jansen]

On 04/17/12 21:38, Chris Mason wrote:
> On Tue, Apr 17, 2012 at 03:36:20PM -0400, Josef Bacik wrote:
>>>> Well then passing a flag down that says we can't interrupt I guess is what we're
>>>> going to have to do and just wait uninterruptible.  I think our best bet is to
>>>> just fix them as they come up, I thought all system calls could return EINTR but
>>>> apparently I was wrong :).  Thanks,
>>>
>>> I'd guess that EINTR is unexpected most of the time.  Including in reads
>>> and writes.  The real question is how long we might end up waiting?
>>>
>>
>> EINTR is valid for both reads and writes.  This was put into place when I would
>> run tests and get tired of waiting for them so I'd ctrl+c and it wouldn't stop
>> even though it's something that's completely stoppable.  So I'd like to leave it
>> in there so at the very least I can still ctrl+c when I accidently run something
>> I don't want to run ;).  Thanks,
>
> Ok. lets just teach git how to eintr.

git known how to eintr, but just not on unlink. It'll be easy to add
that, but not even the manpage states EINTR as return value from unlink.
I'd go for Josef's solution and just make unlink non-interruptible,
adding more when they pop up.

>
>
> -chris

Re: [PATCH] btrfs: don't return EINTR

[Chris Samuel]

On 18/04/12 05:43, Arne Jansen wrote:

> but not even the manpage states EINTR as return value from unlink.

Yeah, it's not mentioned in the Single UNIX Spec:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/unlink.html

-- 
 Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC