From: Linus Torvalds <torvalds@linux-foundation.org>
To: Andreas Gruenbacher <agruenba@redhat.com>,
Christoph Hellwig <hch@infradead.org>,
"Darrick J. Wong" <djwong@kernel.org>,
Dave Chinner <dchinner@redhat.com>
Cc: cluster-devel <cluster-devel@redhat.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [GIT PULL] gfs2 fix
Date: Mon, 2 May 2022 11:58:31 -0700 [thread overview]
Message-ID: <CAHk-=whrt9ofcyonPEbgPOaCG+15mDdz+O9bb0RKrJVTt7vR4w@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=whL74iP6v2P+OafGO0H72ag4wt42k+Kc_01boLP8aqUNQ@mail.gmail.com>
On Mon, May 2, 2022 at 11:31 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> NOTE! This patch is entirely untested. I also didn't actually yet go
> look at what gfs2 does when 'bytes' and 'copied' are different.
Oh, it's a lot of generic iomap_write_end() code, so I guess it's just
as well that I brought in the iomap people.
And the iomap code has many different cases. Some of them do
if (unlikely(copied < len && !folio_test_uptodate(folio)))
return 0;
to force the whole IO to be re-done (looks sane - that's the "the
underlying folio wasn't uptodate, because we expected the write to
make it so").
And that might not have happened before, but it looks like gfs2 does
actually try to deal with that case.
But since Andreas said originally that the IO wasn't aligned, I don't
think that "not uptodate" case is what is going on, and it's more
about some "partial write in the middle of a buffer succeeded"
And the code also has things like
if (ret < len)
iomap_write_failed(iter->inode, pos, len);
which looks very wrong - it's not that the write failed, it's just
incomplete because it was done with page faults disabled. It seems to
try to do some page cache truncation based on the original 'len', but
not taking the successful part into account. Which all sounds
horrifically wrong.
But I don't know the code well enough to really judge. It just makes
me uncomfortable, and I do suspect this code may be quite buggy if the
copy of the full 'len' doesn't succeed.
Again, the patch I sent only _hides_ any issues and makes them
practically impossible to see. It doesn't really _fix_ anything, since
- as mentioned - regardless of fault_in_iov_iter_readable()
succeeding, racing with page-out could then cause the later
copy_page_from_iter_atomic() to have a partial copy anyway.
And hey, maybe there's something entirely different going on, and my
"Heureka! It might be explained by that partial write_end that
generally didn't happen before" is only my shouting at windmills.
Linus
WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds@linux-foundation.org>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] [GIT PULL] gfs2 fix
Date: Mon, 2 May 2022 11:58:31 -0700 [thread overview]
Message-ID: <CAHk-=whrt9ofcyonPEbgPOaCG+15mDdz+O9bb0RKrJVTt7vR4w@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=whL74iP6v2P+OafGO0H72ag4wt42k+Kc_01boLP8aqUNQ@mail.gmail.com>
On Mon, May 2, 2022 at 11:31 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> NOTE! This patch is entirely untested. I also didn't actually yet go
> look at what gfs2 does when 'bytes' and 'copied' are different.
Oh, it's a lot of generic iomap_write_end() code, so I guess it's just
as well that I brought in the iomap people.
And the iomap code has many different cases. Some of them do
if (unlikely(copied < len && !folio_test_uptodate(folio)))
return 0;
to force the whole IO to be re-done (looks sane - that's the "the
underlying folio wasn't uptodate, because we expected the write to
make it so").
And that might not have happened before, but it looks like gfs2 does
actually try to deal with that case.
But since Andreas said originally that the IO wasn't aligned, I don't
think that "not uptodate" case is what is going on, and it's more
about some "partial write in the middle of a buffer succeeded"
And the code also has things like
if (ret < len)
iomap_write_failed(iter->inode, pos, len);
which looks very wrong - it's not that the write failed, it's just
incomplete because it was done with page faults disabled. It seems to
try to do some page cache truncation based on the original 'len', but
not taking the successful part into account. Which all sounds
horrifically wrong.
But I don't know the code well enough to really judge. It just makes
me uncomfortable, and I do suspect this code may be quite buggy if the
copy of the full 'len' doesn't succeed.
Again, the patch I sent only _hides_ any issues and makes them
practically impossible to see. It doesn't really _fix_ anything, since
- as mentioned - regardless of fault_in_iov_iter_readable()
succeeding, racing with page-out could then cause the later
copy_page_from_iter_atomic() to have a partial copy anyway.
And hey, maybe there's something entirely different going on, and my
"Heureka! It might be explained by that partial write_end that
generally didn't happen before" is only my shouting at windmills.
Linus
next prev parent reply other threads:[~2022-05-02 18:58 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-26 14:54 [GIT PULL] gfs2 fix Andreas Gruenbacher
2022-04-26 14:54 ` [Cluster-devel] " Andreas Gruenbacher
2022-04-26 18:31 ` Linus Torvalds
2022-04-26 18:31 ` [Cluster-devel] " Linus Torvalds
2022-04-26 21:27 ` Andreas Gruenbacher
2022-04-26 21:27 ` [Cluster-devel] " Andreas Gruenbacher
2022-04-26 23:33 ` Linus Torvalds
2022-04-26 23:33 ` [Cluster-devel] " Linus Torvalds
2022-04-27 12:29 ` Andreas Gruenbacher
2022-04-27 12:29 ` [Cluster-devel] " Andreas Gruenbacher
2022-04-27 17:13 ` Linus Torvalds
2022-04-27 17:13 ` [Cluster-devel] " Linus Torvalds
2022-04-27 19:41 ` Andreas Gruenbacher
2022-04-27 19:41 ` [Cluster-devel] " Andreas Gruenbacher
2022-04-27 20:25 ` Linus Torvalds
2022-04-27 20:25 ` [Cluster-devel] " Linus Torvalds
2022-04-27 21:26 ` Andreas Gruenbacher
2022-04-27 21:26 ` [Cluster-devel] " Andreas Gruenbacher
2022-04-27 22:20 ` Linus Torvalds
2022-04-27 22:20 ` [Cluster-devel] " Linus Torvalds
2022-04-28 0:00 ` Linus Torvalds
2022-04-28 0:00 ` [Cluster-devel] " Linus Torvalds
2022-04-28 13:26 ` Andreas Gruenbacher
2022-04-28 13:26 ` [Cluster-devel] " Andreas Gruenbacher
2022-04-28 17:09 ` Linus Torvalds
2022-04-28 17:09 ` [Cluster-devel] " Linus Torvalds
2022-04-28 17:17 ` Linus Torvalds
2022-04-28 17:17 ` [Cluster-devel] " Linus Torvalds
2022-04-28 17:21 ` Andreas Gruenbacher
2022-04-28 17:21 ` [Cluster-devel] " Andreas Gruenbacher
2022-04-28 17:38 ` Andreas Gruenbacher
2022-04-28 17:38 ` [Cluster-devel] " Andreas Gruenbacher
2022-05-02 18:31 ` Linus Torvalds
2022-05-02 18:31 ` [Cluster-devel] " Linus Torvalds
2022-05-02 18:58 ` Linus Torvalds [this message]
2022-05-02 18:58 ` Linus Torvalds
2022-05-02 20:24 ` Andreas Gruenbacher
2022-05-02 20:24 ` [Cluster-devel] " Andreas Gruenbacher
2022-05-03 8:56 ` Andreas Gruenbacher
2022-05-03 8:56 ` [Cluster-devel] " Andreas Gruenbacher
2022-05-03 13:30 ` Andreas Gruenbacher
2022-05-03 13:30 ` [Cluster-devel] " Andreas Gruenbacher
2022-05-03 16:19 ` Linus Torvalds
2022-05-03 16:19 ` [Cluster-devel] " Linus Torvalds
2022-05-03 16:41 ` Andreas Gruenbacher
2022-05-03 16:41 ` [Cluster-devel] " Andreas Gruenbacher
2022-05-03 16:50 ` Linus Torvalds
2022-05-03 16:50 ` [Cluster-devel] " Linus Torvalds
2022-05-03 21:35 ` Andreas Gruenbacher
2022-05-03 21:35 ` [Cluster-devel] " Andreas Gruenbacher
2022-05-03 22:41 ` Linus Torvalds
2022-05-03 22:41 ` [Cluster-devel] " Linus Torvalds
2022-05-04 17:52 ` Andreas Gruenbacher
2022-05-04 17:52 ` [Cluster-devel] " Andreas Gruenbacher
2022-04-28 18:16 ` pr-tracker-bot
2022-04-28 18:16 ` [Cluster-devel] " pr-tracker-bot
2022-04-26 19:07 ` pr-tracker-bot
2022-04-26 19:07 ` [Cluster-devel] " pr-tracker-bot
2023-06-06 12:48 Andreas Gruenbacher
2023-06-06 12:55 ` Linus Torvalds
2023-06-06 13:32 ` Andreas Gruenbacher
2023-06-06 13:22 ` pr-tracker-bot
2024-03-25 12:10 Andreas Gruenbacher
2024-03-25 18:18 ` pr-tracker-bot
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='CAHk-=whrt9ofcyonPEbgPOaCG+15mDdz+O9bb0RKrJVTt7vR4w@mail.gmail.com' \
--to=torvalds@linux-foundation.org \
--cc=agruenba@redhat.com \
--cc=cluster-devel@redhat.com \
--cc=dchinner@redhat.com \
--cc=djwong@kernel.org \
--cc=hch@infradead.org \
--cc=linux-kernel@vger.kernel.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.