[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 <***@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);
}

[Arne Jansen]

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

Is this patch a candidate for the next rc?

-Arne

Post by Arne Jansen
---
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
*/
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);
}

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

[Chris Mason]

Post by Arne Jansen

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

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

[Josef Bacik]

Post by Chris Mason

Post by Arne Jansen

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

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

[Arne Jansen]

Post by Josef Bacik

Post by Chris Mason

Post by Arne Jansen

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

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

Post by Josef Bacik
Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

[Josef Bacik]

Post by Arne Jansen

Post by Josef Bacik

Post by Chris Mason

Post by Arne Jansen

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

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

[Chris Mason]

Post by Josef Bacik

Post by Arne Jansen

Post by Josef Bacik

Post by Chris Mason

Post by Arne Jansen

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

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

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

[Josef Bacik]

Post by Chris Mason

Post by Josef Bacik

Post by Arne Jansen

Post by Josef Bacik

Post by Chris Mason

Post by Arne Jansen

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

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

[Chris Mason]

Post by Josef Bacik

Post by Chris Mason

Post by Josef Bacik
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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Arne Jansen]

Post by Chris Mason

Post by Josef Bacik

Post by Chris Mason

Post by Josef Bacik
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.

Post by Chris Mason
-chris

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

[Chris Samuel]

Post by Arne Jansen
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