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
--
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
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); > } >-- 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
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 -- 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
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 -- 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
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-- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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-- 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
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 -- 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