* [PATCH] btrfs: don't return EINTR @ 2012-04-14 12:56 Arne Jansen 2012-04-17 14:15 ` Arne Jansen 0 siblings, 1 reply; 11+ messages in thread From: Arne Jansen @ 2012-04-14 12:56 UTC (permalink / raw) To: linux-btrfs 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-14 12:56 [PATCH] btrfs: don't return EINTR Arne Jansen @ 2012-04-17 14:15 ` Arne Jansen 2012-04-17 15:24 ` Chris Mason 0 siblings, 1 reply; 11+ messages in thread From: Arne Jansen @ 2012-04-17 14:15 UTC (permalink / raw) To: linux-btrfs, Chris Mason 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); > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-17 14:15 ` Arne Jansen @ 2012-04-17 15:24 ` Chris Mason 2012-04-17 18:22 ` Josef Bacik 0 siblings, 1 reply; 11+ messages in thread From: Chris Mason @ 2012-04-17 15:24 UTC (permalink / raw) To: Arne Jansen, Josef Bacik; +Cc: linux-btrfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-17 15:24 ` Chris Mason @ 2012-04-17 18:22 ` Josef Bacik 2012-04-17 18:56 ` Arne Jansen 0 siblings, 1 reply; 11+ messages in thread From: Josef Bacik @ 2012-04-17 18:22 UTC (permalink / raw) To: Chris Mason, Arne Jansen, Josef Bacik, linux-btrfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-17 18:22 ` Josef Bacik @ 2012-04-17 18:56 ` Arne Jansen 2012-04-17 19:14 ` Josef Bacik 0 siblings, 1 reply; 11+ messages in thread From: Arne Jansen @ 2012-04-17 18:56 UTC (permalink / raw) To: Josef Bacik; +Cc: Chris Mason, linux-btrfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-17 18:56 ` Arne Jansen @ 2012-04-17 19:14 ` Josef Bacik 2012-04-17 19:34 ` Chris Mason 0 siblings, 1 reply; 11+ messages in thread From: Josef Bacik @ 2012-04-17 19:14 UTC (permalink / raw) To: Arne Jansen; +Cc: Josef Bacik, Chris Mason, linux-btrfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-17 19:14 ` Josef Bacik @ 2012-04-17 19:34 ` Chris Mason 2012-04-17 19:36 ` Josef Bacik 0 siblings, 1 reply; 11+ messages in thread From: Chris Mason @ 2012-04-17 19:34 UTC (permalink / raw) To: Josef Bacik; +Cc: Arne Jansen, linux-btrfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-17 19:34 ` Chris Mason @ 2012-04-17 19:36 ` Josef Bacik 2012-04-17 19:38 ` Chris Mason 0 siblings, 1 reply; 11+ messages in thread From: Josef Bacik @ 2012-04-17 19:36 UTC (permalink / raw) To: Chris Mason, Josef Bacik, Arne Jansen, linux-btrfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-17 19:36 ` Josef Bacik @ 2012-04-17 19:38 ` Chris Mason 2012-04-17 19:43 ` Arne Jansen 0 siblings, 1 reply; 11+ messages in thread From: Chris Mason @ 2012-04-17 19:38 UTC (permalink / raw) To: Josef Bacik; +Cc: Arne Jansen, linux-btrfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-17 19:38 ` Chris Mason @ 2012-04-17 19:43 ` Arne Jansen 2012-04-17 23:39 ` Chris Samuel 0 siblings, 1 reply; 11+ messages in thread From: Arne Jansen @ 2012-04-17 19:43 UTC (permalink / raw) To: Chris Mason, Josef Bacik, linux-btrfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btrfs: don't return EINTR 2012-04-17 19:43 ` Arne Jansen @ 2012-04-17 23:39 ` Chris Samuel 0 siblings, 0 replies; 11+ messages in thread From: Chris Samuel @ 2012-04-17 23:39 UTC (permalink / raw) To: linux-btrfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-04-17 23:39 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-14 12:56 [PATCH] btrfs: don't return EINTR Arne Jansen 2012-04-17 14:15 ` Arne Jansen 2012-04-17 15:24 ` Chris Mason 2012-04-17 18:22 ` Josef Bacik 2012-04-17 18:56 ` Arne Jansen 2012-04-17 19:14 ` Josef Bacik 2012-04-17 19:34 ` Chris Mason 2012-04-17 19:36 ` Josef Bacik 2012-04-17 19:38 ` Chris Mason 2012-04-17 19:43 ` Arne Jansen 2012-04-17 23:39 ` Chris Samuel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).