linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Steve French <smfrench@gmail.com>
Cc: CIFS <linux-cifs@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Pavel Shilovsky <piastryyy@gmail.com>
Subject: Re: scp bug due to progress indicator when copying from remote to local on Linux
Date: Fri, 11 Jan 2019 13:22:40 -0800	[thread overview]
Message-ID: <20190111212240.GL6310@bombadil.infradead.org> (raw)
In-Reply-To: <CAH2r5mveiLSTS6W_yQzhEbEdVb0AuEkaP1XO9WxXLWpLXE7JAA@mail.gmail.com>

On Fri, Jan 11, 2019 at 03:13:05PM -0600, Steve French wrote:
> On Fri, Jan 11, 2019 at 7:28 AM Matthew Wilcox <willy@infradead.org> wrote:
> > Are you saying the SIGALRM interrupts ftruncate() and causes the ftruncate
> > to fail?
> 
> So ftruncate does not really fail (the file contents and size match on
> source and target after the copy) but the scp 'fails' and the user
> would be quite confused (and presumably the network stack doesn't like
> this signal, which can cause disconnects etc. which in theory could
> cause reconnect/data loss issues in some corner cases).

You've run into the problem that userspace simply doesn't check the
return value from syscalls.  It's not just scp, it's every program.
Looking through cifs, you seem to do a lot of wait_event_interruptible()
where you maybe should be doing wait_event_killable()?

> ftruncate(3, 262144000)                 = ? ERESTARTSYS (To be
> restarted if SA_RESTART is set)
> --- SIGALRM {si_signo=SIGALRM, si_code=SI_KERNEL} ---
> --- SIGWINCH {si_signo=SIGWINCH, si_code=SI_KERNEL} ---
> rt_sigreturn({mask=[ALRM]})             = 0
> ioctl(1, TIOCGWINSZ, {ws_row=51, ws_col=156, ws_xpixel=0, ws_ypixel=0}) = 0
> getpgrp()                               = 82563

Right ... so the code never calls ftruncate() again.  Changing all of
userspace is just not going to happen; maybe you could get stuff fixed in
libc, but really ftruncate() should only be interrupted by a fatal signal
and not by SIGALRM.

  reply	other threads:[~2019-01-11 21:22 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-11  6:11 scp bug due to progress indicator when copying from remote to local on Linux Steve French
2019-01-11  8:50 ` Dr. Bernd Feige
2019-01-11 13:28 ` Matthew Wilcox
2019-01-11 21:13   ` Steve French
2019-01-11 21:22     ` Matthew Wilcox [this message]
2019-01-11 21:50       ` Steve French
2019-01-11 23:05         ` Matthew Wilcox
2019-01-11 23:18           ` Steve French
2019-01-14 19:48       ` Pavel Shilovsky
2019-01-11 21:48     ` Andreas Dilger
2019-01-11 22:02       ` Steve French
2019-01-11 23:51         ` Andreas Dilger
2019-01-12  0:06           ` Steve French

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190111212240.GL6310@bombadil.infradead.org \
    --to=willy@infradead.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=piastryyy@gmail.com \
    --cc=smfrench@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).