PostgreSQL's handling of fsync() errors is unsafe and risks data loss at least on XFS

Hi all

Some time ago I ran into an issue where a user encountered data corruption
after a storage error. PostgreSQL played a part in that corruption by
allowing checkpoint what should've been a fatal error.

TL;DR: Pg should PANIC on fsync() EIO return. Retrying fsync() is not OK at
least on Linux. When fsync() returns success it means "all writes since the
last fsync have hit disk" but we assume it means "all writes since the last
SUCCESSFUL fsync have hit disk".

Pg wrote some blocks, which went to OS dirty buffers for writeback.
Writeback failed due to an underlying storage error. The block I/O layer
and XFS marked the writeback page as failed (AS_EIO), but had no way to
tell the app about the failure. When Pg called fsync() on the FD during the
next checkpoint, fsync() returned EIO because of the flagged page, to tell
Pg that a previous async write failed. Pg treated the checkpoint as failed
and didn't advance the redo start position in the control file.

All good so far.

But then we retried the checkpoint, which retried the fsync(). The retry
succeeded, because the prior fsync() *cleared the AS_EIO bad page flag*.

The write never made it to disk, but we completed the checkpoint, and
merrily carried on our way. Whoops, data loss.

The clear-error-and-continue behaviour of fsync is not documented as far as
I can tell. Nor is fsync() returning EIO unless you have a very new linux
man-pages with the patch I wrote to add it. But from what I can see in the
POSIX standard we are not given any guarantees about what happens on
fsync() failure at all, so we're probably wrong to assume that retrying
fsync( ) is safe.

If the server had been using ext3 or ext4 with errors=remount-ro, the
problem wouldn't have occurred because the first I/O error would've
remounted the FS and stopped Pg from continuing. But XFS doesn't have that
option. There may be other situations where this can occur too, involving
LVM and/or multipath, but I haven't comprehensively dug out the details yet.

It proved possible to recover the system by faking up a backup label from
before the first incorrectly-successful checkpoint, forcing redo to repeat
and write the lost blocks. But ... what a mess.

I posted about the underlying fsync issue here some time ago:

https://stackoverflow.com/q/42434872/398670

but haven't had a chance to follow up about the Pg specifics.

I've been looking at the problem on and off and haven't come up with a good
answer. I think we should just PANIC and let redo sort it out by repeating
the failed write when it repeats work since the last checkpoint.

The API offered by async buffered writes and fsync offers us no way to find
out which page failed, so we can't just selectively redo that write. I
think we do know the relfilenode associated with the fd that failed to
fsync, but not much more. So the alternative seems to be some sort of
potentially complex online-redo scheme where we replay WAL only the
relation on which we had the fsync() error, while otherwise servicing
queries normally. That's likely to be extremely error-prone and hard to
test, and it's trying to solve a case where on other filesystems the whole
DB would grind to a halt anyway.

I looked into whether we can solve it with use of the AIO API instead, but
the mess is even worse there - from what I can tell you can't even reliably
guarantee fsync at all on all Linux kernel versions.

We already PANIC on fsync() failure for WAL segments. We just need to do
the same for data forks at least for EIO. This isn't as bad as it seems
because AFAICS fsync only returns EIO in cases where we should be stopping
the world anyway, and many FSes will do that for us.

There are rather a lot of pg_fsync() callers. While we could handle this
case-by-case for each one, I'm tempted to just make pg_fsync() itself
intercept EIO and PANIC. Thoughts?

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> TL;DR: Pg should PANIC on fsync() EIO return.

Surely you jest.

> Retrying fsync() is not OK at
> least on Linux. When fsync() returns success it means "all writes since the
> last fsync have hit disk" but we assume it means "all writes since the last
> SUCCESSFUL fsync have hit disk".

If that's actually the case, we need to push back on this kernel brain
damage, because as you're describing it fsync would be completely useless.

Moreover, POSIX is entirely clear that successful fsync means all
preceding writes for the file have been completed, full stop, doesn't
matter when they were issued.

regards, tom lane

On Tue, Mar 27, 2018 at 11:53:08PM -0400, Tom Lane wrote:
> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
>> TL;DR: Pg should PANIC on fsync() EIO return.
>
> Surely you jest.

Any callers of pg_fsync in the backend code are careful enough to check
the returned status, sometimes doing retries like in mdsync, so what is
proposed here would be a regression.
--
Michael

On Thu, Mar 29, 2018 at 3:30 PM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Tue, Mar 27, 2018 at 11:53:08PM -0400, Tom Lane wrote:
>> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
>>> TL;DR: Pg should PANIC on fsync() EIO return.
>>
>> Surely you jest.
>
> Any callers of pg_fsync in the backend code are careful enough to check
> the returned status, sometimes doing retries like in mdsync, so what is
> proposed here would be a regression.

Craig, is the phenomenon you described the same as the second issue
"Reporting writeback errors" discussed in this article?

https://lwn.net/Articles/724307/

"Current kernels might report a writeback error on an fsync() call,
but there are a number of ways in which that can fail to happen."

That's... I'm speechless.

--
Thomas Munro
http://www.enterprisedb.com

On Thu, Mar 29, 2018 at 11:30:59AM +0900, Michael Paquier wrote:
> On Tue, Mar 27, 2018 at 11:53:08PM -0400, Tom Lane wrote:
> > Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> >> TL;DR: Pg should PANIC on fsync() EIO return.
> >
> > Surely you jest.
>
> Any callers of pg_fsync in the backend code are careful enough to check
> the returned status, sometimes doing retries like in mdsync, so what is
> proposed here would be a regression.

The retries are the source of the problem ; the first fsync() can return EIO,
and also *clears the error* causing a 2nd fsync (of the same data) to return
success.

(Note, I can see that it might be useful to PANIC on EIO but retry for ENOSPC).

On Thu, Mar 29, 2018 at 03:48:27PM +1300, Thomas Munro wrote:
> Craig, is the phenomenon you described the same as the second issue
> "Reporting writeback errors" discussed in this article?
> https://lwn.net/Articles/724307/

Worse, the article acknowledges the behavior without apparently suggesting to
change it:

"Storing that value in the file structure has an important benefit: it makes
it possible to report a writeback error EXACTLY ONCE TO EVERY PROCESS THAT
CALLS FSYNC() .... In current kernels, ONLY THE FIRST CALLER AFTER AN ERROR
OCCURS HAS A CHANCE OF SEEING THAT ERROR INFORMATION."

I believe I reproduced the problem behavior using dmsetup "error" target, see
attached.

strace looks like this:

kernel is Linux 4.10.0-28-generic #32~16.04.2-Ubuntu SMP Thu Jul 20 10:19:48 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

1 open("/dev/mapper/eio", O_RDWR|O_CREAT, 0600) = 3
2 write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192) = 8192
3 write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192) = 8192
4 write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192) = 8192
5 write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192) = 8192
6 write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192) = 8192
7 write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192) = 8192
8 write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192) = 2560
9 write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 8192) = -1 ENOSPC (No space left on device)
10 dup(2) = 4
11 fcntl(4, F_GETFL) = 0x8402 (flags O_RDWR|O_APPEND|O_LARGEFILE)
12 brk(NULL) = 0x1299000
13 brk(0x12ba000) = 0x12ba000
14 fstat(4, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 2), ...}) = 0
15 write(4, "write(1): No space left on devic"..., 34write(1): No space left on device
16 ) = 34
17 close(4) = 0
18 fsync(3) = -1 EIO (Input/output error)
19 dup(2) = 4
20 fcntl(4, F_GETFL) = 0x8402 (flags O_RDWR|O_APPEND|O_LARGEFILE)
21 fstat(4, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 2), ...}) = 0
22 write(4, "fsync(1): Input/output error\n", 29fsync(1): Input/output error
23 ) = 29
24 close(4) = 0
25 close(3) = 0
26 open("/dev/mapper/eio", O_RDWR|O_CREAT, 0600) = 3
27 fsync(3) = 0
28 write(3, "\0", 1) = 1
29 fsync(3) = 0
30 exit_group(0) = ?

2: EIO isn't seen initially due to writeback page cache;
9: ENOSPC due to small device
18: original IO error reported by fsync, good
25: the original FD is closed
26: ..and file reopened
27: fsync on file with still-dirty data+EIO returns success BAD

10, 19: I'm not sure why there's dup(2), I guess glibc thinks that perror
should write to a separate FD (?)

Also note, close() ALSO returned success..which you might think exonerates the
2nd fsync(), but I think may itself be problematic, no? In any case, the 2nd
byte certainly never got written to DM error, and the failure status was lost
following fsync().

I get the exact same behavior if I break after one write() loop, such as to
avoid ENOSPC.

Justin

On Thu, Mar 29, 2018 at 6:00 PM, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> The retries are the source of the problem ; the first fsync() can return EIO,
> and also *clears the error* causing a 2nd fsync (of the same data) to return
> success.

What I'm failing to grok here is how that error flag even matters,
whether it's a single bit or a counter as described in that patch. If
write back failed, *the page is still dirty*. So all future calls to
fsync() need to try to try to flush it again, and (presumably) fail
again (unless it happens to succeed this time around).

--
Thomas Munro
http://www.enterprisedb.com

On 29 March 2018 at 13:06, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
wrote:

> On Thu, Mar 29, 2018 at 6:00 PM, Justin Pryzby <pryzby(at)telsasoft(dot)com>
> wrote:
> > The retries are the source of the problem ; the first fsync() can return
> EIO,
> > and also *clears the error* causing a 2nd fsync (of the same data) to
> return
> > success.
>
> What I'm failing to grok here is how that error flag even matters,
> whether it's a single bit or a counter as described in that patch. If
> write back failed, *the page is still dirty*. So all future calls to
> fsync() need to try to try to flush it again, and (presumably) fail
> again (unless it happens to succeed this time around).
> <http://www.enterprisedb.com>
>

You'd think so. But it doesn't appear to work that way. You can see
yourself with the error device-mapper destination mapped over part of a
volume.

I wrote a test case here.

https://github.com/ringerc/scrapcode/blob/master/testcases/fsync-error-clear.c

I don't pretend the kernel behaviour is sane. And it's possible I've made
an error in my analysis. But since I've observed this in the wild, and seen
it in a test case, I strongly suspect that's what I've described is just
what's happening, brain-dead or no.

Presumably the kernel marks the page clean when it dispatches it to the I/O
subsystem and doesn't dirty it again on I/O error? I haven't dug that deep
on the kernel side. See the stackoverflow post for details on what I found
in kernel code analysis.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 29 March 2018 at 10:48, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
wrote:

> On Thu, Mar 29, 2018 at 3:30 PM, Michael Paquier <michael(at)paquier(dot)xyz>
> wrote:
> > On Tue, Mar 27, 2018 at 11:53:08PM -0400, Tom Lane wrote:
> >> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> >>> TL;DR: Pg should PANIC on fsync() EIO return.
> >>
> >> Surely you jest.
> >
> > Any callers of pg_fsync in the backend code are careful enough to check
> > the returned status, sometimes doing retries like in mdsync, so what is
> > proposed here would be a regression.
>
> Craig, is the phenomenon you described the same as the second issue
> "Reporting writeback errors" discussed in this article?
>
> https://lwn.net/Articles/724307/

A variant of it, by the looks.

The problem in our case is that the kernel only tells us about the error
once. It then forgets about it. So yes, that seems like a variant of the
statement:

> "Current kernels might report a writeback error on an fsync() call,
> but there are a number of ways in which that can fail to happen."
>
> That's... I'm speechless.

Yeah.

It's a bit nuts.

I was astonished when I saw the behaviour, and that it appears undocumented.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 29 March 2018 at 10:30, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Tue, Mar 27, 2018 at 11:53:08PM -0400, Tom Lane wrote:
> > Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> >> TL;DR: Pg should PANIC on fsync() EIO return.
> >
> > Surely you jest.
>
> Any callers of pg_fsync in the backend code are careful enough to check
> the returned status, sometimes doing retries like in mdsync, so what is
> proposed here would be a regression.

I covered this in my original post.

Yes, we check the return value. But what do we do about it? For fsyncs of
heap files, we ERROR, aborting the checkpoint. We'll retry the checkpoint
later, which will retry the fsync(). **Which will now appear to succeed**
because the kernel forgot that it lost our writes after telling us the
first time. So we do check the error code, which returns success, and we
complete the checkpoint and move on.

But we only retried the fsync, not the writes before the fsync.

So we lost data. Or rather, failed to detect that the kernel did so, so our
checkpoint was bad and could not be completed.

The problem is that we keep retrying checkpoints *without* repeating the
writes leading up to the checkpoint, and retrying fsync.

I don't pretend the kernel behaviour is sane, but we'd better deal with it
anyway.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 28 March 2018 at 11:53, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> > TL;DR: Pg should PANIC on fsync() EIO return.
>
> Surely you jest.
>

No. I'm quite serious. Worse, we quite possibly have to do it for ENOSPC as
well to avoid similar lost-page-write issues.

It's not necessary on ext3/ext4 with errors=remount-ro, but that's only
because the FS stops us dead in our tracks.

I don't pretend it's sane. The kernel behaviour is IMO crazy. If it's going
to lose a write, it should at minimum mark the FD as broken so no further
fsync() or anything else can succeed on the FD, and an app that cares about
durability must repeat the whole set of work since the prior succesful
fsync(). Just reporting it once and forgetting it is madness.

But even if we convince the kernel folks of that, how do other platforms
behave? And how long before these kernels are out of use? We'd better deal
with it, crazy or no.

Please see my StackOverflow post for the kernel-level explanation. Note
also the test case link there. https://stackoverflow.com/a/42436054/398670

> Retrying fsync() is not OK at
> > least on Linux. When fsync() returns success it means "all writes since
> the
> > last fsync have hit disk" but we assume it means "all writes since the
> last
> > SUCCESSFUL fsync have hit disk".
>
> If that's actually the case, we need to push back on this kernel brain
> damage, because as you're describing it fsync would be completely useless.
>

It's not useless, it's just telling us something other than what we think
it means. The promise it seems to give us is that if it reports an error
once, everything *after* that is useless, so we should throw our toys,
close and reopen everything, and redo from the last known-good state.

Though as Tomas posted below, it provides rather weaker guarantees than I
thought in some other areas too. See that lwn.net article he linked.

> Moreover, POSIX is entirely clear that successful fsync means all
> preceding writes for the file have been completed, full stop, doesn't
> matter when they were issued.
>

I can't find anything that says so to me. Please quote relevant spec.

I'm working from
http://pubs.opengroup.org/onlinepubs/009695399/functions/fsync.html which
states that

"The fsync() function shall request that all data for the open file
descriptor named by fildes is to be transferred to the storage device
associated with the file described by fildes. The nature of the transfer is
implementation-defined. The fsync() function shall not return until the
system has completed that action or until an error is detected."

My reading is that POSIX does not specify what happens AFTER an error is
detected. It doesn't say that error has to be persistent and that
subsequent calls must also report the error. It also says:

"If the fsync() function fails, outstanding I/O operations are not
guaranteed to have been completed."

but that doesn't clarify matters much either, because it can be read to
mean that once there's been an error reported for some IO operations
there's no guarantee those operations are ever completed even after a
subsequent fsync returns success.

I'm not seeking to defend what the kernel seems to be doing. Rather, saying
that we might see similar behaviour on other platforms, crazy or not. I
haven't looked past linux yet, though.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Mar 29, 2018 at 6:58 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 28 March 2018 at 11:53, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
>> > TL;DR: Pg should PANIC on fsync() EIO return.
>>
>> Surely you jest.
>
> No. I'm quite serious. Worse, we quite possibly have to do it for ENOSPC as
> well to avoid similar lost-page-write issues.

I found your discussion with kernel hacker Jeff Layton at
https://lwn.net/Articles/718734/ in which he said: "The stackoverflow
writeup seems to want a scheme where pages stay dirty after a
writeback failure so that we can try to fsync them again. Note that
that has never been the case in Linux after hard writeback failures,
AFAIK, so programs should definitely not assume that behavior."

The article above that says the same thing a couple of different ways,
ie that writeback failure leaves you with pages that are neither
written to disk successfully nor marked dirty.

If I'm reading various articles correctly, the situation was even
worse before his errseq_t stuff landed. That fixed cases of
completely unreported writeback failures due to sharing of PG_error
for both writeback and read errors with certain filesystems, but it
doesn't address the clean pages problem.

Yeah, I see why you want to PANIC.

>> Moreover, POSIX is entirely clear that successful fsync means all
>> preceding writes for the file have been completed, full stop, doesn't
>> matter when they were issued.
>
> I can't find anything that says so to me. Please quote relevant spec.
>
> I'm working from
> http://pubs.opengroup.org/onlinepubs/009695399/functions/fsync.html which
> states that
>
> "The fsync() function shall request that all data for the open file
> descriptor named by fildes is to be transferred to the storage device
> associated with the file described by fildes. The nature of the transfer is
> implementation-defined. The fsync() function shall not return until the
> system has completed that action or until an error is detected."
>
> My reading is that POSIX does not specify what happens AFTER an error is
> detected. It doesn't say that error has to be persistent and that subsequent
> calls must also report the error. It also says:

FWIW my reading is the same as Tom's. It says "all data for the open
file descriptor" without qualification or special treatment after
errors. Not "some".

> I'm not seeking to defend what the kernel seems to be doing. Rather, saying
> that we might see similar behaviour on other platforms, crazy or not. I
> haven't looked past linux yet, though.

I see no reason to think that any other operating system would behave
that way without strong evidence... This is openly acknowledged to be
"a mess" and "a surprise" in the Filesystem Summit article. I am not
really qualified to comment, but from a cursory glance at FreeBSD's
vfs_bio.c I think it's doing what you'd hope for... see the code near
the comment "Failed write, redirty."

--
Thomas Munro
http://www.enterprisedb.com

On 29 March 2018 at 20:07, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
wrote:

> On Thu, Mar 29, 2018 at 6:58 PM, Craig Ringer <craig(at)2ndquadrant(dot)com>
> wrote:
> > On 28 March 2018 at 11:53, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>
> >> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> >> > TL;DR: Pg should PANIC on fsync() EIO return.
> >>
> >> Surely you jest.
> >
> > No. I'm quite serious. Worse, we quite possibly have to do it for ENOSPC
> as
> > well to avoid similar lost-page-write issues.
>
> I found your discussion with kernel hacker Jeff Layton at
> https://lwn.net/Articles/718734/ in which he said: "The stackoverflow
> writeup seems to want a scheme where pages stay dirty after a
> writeback failure so that we can try to fsync them again. Note that
> that has never been the case in Linux after hard writeback failures,
> AFAIK, so programs should definitely not assume that behavior."
>
> The article above that says the same thing a couple of different ways,
> ie that writeback failure leaves you with pages that are neither
> written to disk successfully nor marked dirty.
>
> If I'm reading various articles correctly, the situation was even
> worse before his errseq_t stuff landed. That fixed cases of
> completely unreported writeback failures due to sharing of PG_error
> for both writeback and read errors with certain filesystems, but it
> doesn't address the clean pages problem.
>
> Yeah, I see why you want to PANIC.
>

In more ways than one ;)

> I'm not seeking to defend what the kernel seems to be doing. Rather,
> saying
> > that we might see similar behaviour on other platforms, crazy or not. I
> > haven't looked past linux yet, though.
>
> I see no reason to think that any other operating system would behave
> that way without strong evidence... This is openly acknowledged to be
> "a mess" and "a surprise" in the Filesystem Summit article. I am not
> really qualified to comment, but from a cursory glance at FreeBSD's
> vfs_bio.c I think it's doing what you'd hope for... see the code near
> the comment "Failed write, redirty."

Ok, that's reassuring, but doesn't help us on the platform the great
majority of users deploy on :(

"If on Linux, PANIC"

Hrm.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Mar 29, 2018 at 2:07 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> I found your discussion with kernel hacker Jeff Layton at
> https://lwn.net/Articles/718734/ in which he said: "The stackoverflow
> writeup seems to want a scheme where pages stay dirty after a
> writeback failure so that we can try to fsync them again. Note that
> that has never been the case in Linux after hard writeback failures,
> AFAIK, so programs should definitely not assume that behavior."

And a bit below in the same comments, to this question about PG: "So,
what are the options at this point? The assumption was that we can
repeat the fsync (which as you point out is not the case), or shut
down the database and perform recovery from WAL", the same Jeff Layton
seems to agree PANIC is the appropriate response:
"Replaying the WAL synchronously sounds like the simplest approach
when you get an error on fsync. These are uncommon occurrences for the
most part, so having to fall back to slow, synchronous error recovery
modes when this occurs is probably what you want to do.".
And right after, he confirms the errseq_t patches are about always
detecting this, not more:
"The main thing I working on is to better guarantee is that you
actually get an error when this occurs rather than silently corrupting
your data. The circumstances where that can occur require some
corner-cases, but I think we need to make sure that it doesn't occur."

Jeff's comments in the pull request that merged errseq_t are worth
reading as well:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=088737f44bbf6378745f5b57b035e57ee3dc4750

> The article above that says the same thing a couple of different ways,
> ie that writeback failure leaves you with pages that are neither
> written to disk successfully nor marked dirty.
>
> If I'm reading various articles correctly, the situation was even
> worse before his errseq_t stuff landed. That fixed cases of
> completely unreported writeback failures due to sharing of PG_error
> for both writeback and read errors with certain filesystems, but it
> doesn't address the clean pages problem.

Indeed, that's exactly how I read it as well (opinion formed
independently before reading your sentence above). The errseq_t
patches landed in v4.13 by the way, so very recently.

> Yeah, I see why you want to PANIC.

Indeed. Even doing that leaves question marks about all the kernel
versions before v4.13, which at this point is pretty much everything
out there, not even detecting this reliably. This is messy.

On Fri, Mar 30, 2018 at 5:20 AM, Catalin Iacob <iacobcatalin(at)gmail(dot)com> wrote:
> Jeff's comments in the pull request that merged errseq_t are worth
> reading as well:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=088737f44bbf6378745f5b57b035e57ee3dc4750

Wow. It looks like there may be a separate question of when each
filesystem adopted this new infrastructure?

>> Yeah, I see why you want to PANIC.
>
> Indeed. Even doing that leaves question marks about all the kernel
> versions before v4.13, which at this point is pretty much everything
> out there, not even detecting this reliably. This is messy.

The pre-errseq_t problems are beyond our control. There's nothing we
can do about that in userspace (except perhaps abandon OS-buffered IO,
a big project). We just need to be aware that this problem exists in
certain kernel versions and be grateful to Layton for fixing it.

The dropped dirty flag problem is something we can and in my view
should do something about, whatever we might think about that design
choice. As Andrew Gierth pointed out to me in an off-list chat about
this, by the time you've reached this state, both PostgreSQL's buffer
and the kernel's buffer are clean and might be reused for another
block at any time, so your data might be gone from the known universe
-- we don't even have the option to rewrite our buffers in general.
Recovery is the only option.

Thank you to Craig for chasing this down and +1 for his proposal, on Linux only.

--
Thomas Munro
http://www.enterprisedb.com

On Fri, Mar 30, 2018 at 10:18:14AM +1300, Thomas Munro wrote:

> >> Yeah, I see why you want to PANIC.
> >
> > Indeed. Even doing that leaves question marks about all the kernel
> > versions before v4.13, which at this point is pretty much everything
> > out there, not even detecting this reliably. This is messy.

There may still be a way to reliably detect this on older kernel
versions from userspace, but it will be messy whatsoever. On EIO
errors, the kernel will not restore the dirty page flags, but it
will flip the error flags on the failed pages. One could mmap()
the file in question, obtain the PFNs (via /proc/pid/pagemap)
and enumerate those to match the ones with the error flag switched
on (via /proc/kpageflags). This could serve at least as a detection
mechanism, but one could also further use this info to logically
map the pages that failed IO back to the original file offsets,
and potentially retry IO just for those file ranges that cover
the failed pages. Just an idea, not tested.

Best regards,
Anthony

On 31 March 2018 at 21:24, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:

> On Fri, Mar 30, 2018 at 10:18:14AM +1300, Thomas Munro wrote:
>
> > >> Yeah, I see why you want to PANIC.
> > >
> > > Indeed. Even doing that leaves question marks about all the kernel
> > > versions before v4.13, which at this point is pretty much everything
> > > out there, not even detecting this reliably. This is messy.
>
> There may still be a way to reliably detect this on older kernel
> versions from userspace, but it will be messy whatsoever. On EIO
> errors, the kernel will not restore the dirty page flags, but it
> will flip the error flags on the failed pages. One could mmap()
> the file in question, obtain the PFNs (via /proc/pid/pagemap)
> and enumerate those to match the ones with the error flag switched
> on (via /proc/kpageflags). This could serve at least as a detection
> mechanism, but one could also further use this info to logically
> map the pages that failed IO back to the original file offsets,
> and potentially retry IO just for those file ranges that cover
> the failed pages. Just an idea, not tested.
>

That sounds like a huge amount of complexity, with uncertainty as to how
it'll behave kernel-to-kernel, for negligble benefit.

I was exploring the idea of doing selective recovery of one relfilenode,
based on the assumption that we know the filenode related to the fd that
failed to fsync(). We could redo only WAL on that relation. But it fails
the same test: it's too complex for a niche case that shouldn't happen in
the first place, so it'll probably have bugs, or grow bugs in bitrot over
time.

Remember, if you're on ext4 with errors=remount-ro, you get shut down even
harder than a PANIC. So we should just use the big hammer here.

I'll send a patch this week.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> So we should just use the big hammer here.

And bitch, loudly and publicly, about how broken this kernel behavior is.
If we make enough of a stink maybe it'll get fixed.

regards, tom lane

On Sat, Mar 31, 2018 at 12:38:12PM -0400, Tom Lane wrote:
> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
>> So we should just use the big hammer here.
>
> And bitch, loudly and publicly, about how broken this kernel behavior is.
> If we make enough of a stink maybe it'll get fixed.

That won't fix anything released already, so as per the information
gathered something has to be done anyway. The discussion of this thread
is spreading quite a lot actually.

Handling things at a low-level looks like a better plan for the backend.
Tools like pg_basebackup and pg_dump also issue fsync's on the data
created, we should do an equivalent for them, with some exit() calls in
file_utils.c. As of now failures are logged to stderr but not
considered fatal.
--
Michael

On Sun, Apr 01, 2018 at 12:13:09AM +0800, Craig Ringer wrote:
> On 31 March 2018 at 21:24, Anthony Iliopoulos <[1]ailiop(at)altatus(dot)com>
> wrote:
>
> On Fri, Mar 30, 2018 at 10:18:14AM +1300, Thomas Munro wrote:
>
> > >> Yeah, I see why you want to PANIC.
> > >
> > > Indeed. Even doing that leaves question marks about all the kernel
> > > versions before v4.13, which at this point is pretty much everything
> > > out there, not even detecting this reliably. This is messy.
>
> There may still be a way to reliably detect this on older kernel
> versions from userspace, but it will be messy whatsoever. On EIO
> errors, the kernel will not restore the dirty page flags, but it
> will flip the error flags on the failed pages. One could mmap()
> the file in question, obtain the PFNs (via /proc/pid/pagemap)
> and enumerate those to match the ones with the error flag switched
> on (via /proc/kpageflags). This could serve at least as a detection
> mechanism, but one could also further use this info to logically
> map the pages that failed IO back to the original file offsets,
> and potentially retry IO just for those file ranges that cover
> the failed pages. Just an idea, not tested.
>
> That sounds like a huge amount of complexity, with uncertainty as to how
> it'll behave kernel-to-kernel, for negligble benefit.

Those interfaces have been around since the kernel 2.6 times and are
rather stable, but I was merely responding to your original post comment
regarding having a way of finding out which page(s) failed. I assume
that indeed there would be no benefit, especially since those errors
are usually not transient (typically they come from hard medium faults),
and although a filesystem could theoretically mask the error by allocating
a different logical block, I am not aware of any implementation that
currently does that.

> I was exploring the idea of doing selective recovery of one relfilenode,
> based on the assumption that we know the filenode related to the fd that
> failed to fsync(). We could redo only WAL on that relation. But it fails
> the same test: it's too complex for a niche case that shouldn't happen in
> the first place, so it'll probably have bugs, or grow bugs in bitrot over
> time.

Fully agree, those cases should be sufficiently rare that a complex
and possibly non-maintainable solution is not really warranted.

> Remember, if you're on ext4 with errors=remount-ro, you get shut down even
> harder than a PANIC. So we should just use the big hammer here.

I am not entirely sure what you mean here, does Pg really treat write()
errors as fatal? Also, the kind of errors that ext4 detects with this
option is at the superblock level and govern metadata rather than actual
data writes (recall that those are buffered anyway, no actual device IO
has to take place at the time of write()).

Best regards,
Anthony

On Sat, Mar 31, 2018 at 12:38:12PM -0400, Tom Lane wrote:
> Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> > So we should just use the big hammer here.
>
> And bitch, loudly and publicly, about how broken this kernel behavior is.
> If we make enough of a stink maybe it'll get fixed.

It is not likely to be fixed (beyond what has been done already with the
manpage patches and errseq_t fixes on the reporting level). The issue is,
the kernel needs to deal with hard IO errors at that level somehow, and
since those errors typically persist, re-dirtying the pages would not
really solve the problem (unless some filesystem remaps the request to a
different block, assuming the device is alive). Keeping around dirty
pages that cannot possibly be written out is essentially a memory leak,
as those pages would stay around even after the application has exited.

Best regards,
Anthony

On Fri, Mar 30, 2018 at 10:18 AM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> ... on Linux only.

Apparently I was too optimistic. I had looked only at FreeBSD, which
keeps the page around and dirties it so we can retry, but the other
BSDs apparently don't (FreeBSD changed that in 1999). From what I can
tell from the sources below, we have:

Linux, OpenBSD, NetBSD: retrying fsync() after EIO lies
FreeBSD, Illumos: retrying fsync() after EIO tells the truth

Maybe my drive-by assessment of those kernel routines is wrong and
someone will correct me, but I'm starting to think you might be better
to assume the worst on all systems. Perhaps a GUC that defaults to
panicking, so that users on those rare OSes could turn that off? Even
then I'm not sure if the failure mode will be that great anyway or if
it's worth having two behaviours. Thoughts?

http://mail-index.netbsd.org/netbsd-users/2018/03/30/msg020576.html
https://github.com/NetBSD/src/blob/trunk/sys/kern/vfs_bio.c#L1059
https://github.com/openbsd/src/blob/master/sys/kern/vfs_bio.c#L867
https://github.com/freebsd/freebsd/blob/master/sys/kern/vfs_bio.c#L2631
https://github.com/freebsd/freebsd/commit/e4e8fec98ae986357cdc208b04557dba55a59266
https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/os/bio.c#L441

--
Thomas Munro
http://www.enterprisedb.com

On 2 April 2018 at 02:24, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
wrote:

>
> Maybe my drive-by assessment of those kernel routines is wrong and
> someone will correct me, but I'm starting to think you might be better
> to assume the worst on all systems. Perhaps a GUC that defaults to
> panicking, so that users on those rare OSes could turn that off? Even
> then I'm not sure if the failure mode will be that great anyway or if
> it's worth having two behaviours. Thoughts?
>
>
I see little benefit to not just PANICing unconditionally on EIO, really.
It shouldn't happen, and if it does, we want to be pretty conservative and
adopt a data-protective approach.

I'm rather more worried by doing it on ENOSPC. Which looks like it might be
necessary from what I recall finding in my test case + kernel code reading.
I really don't want to respond to a possibly-transient ENOSPC by PANICing
the whole server unnecessarily.

BTW, the support team at 2ndQ is presently working on two separate issues
where ENOSPC resulted in DB corruption, though neither of them involve logs
of lost page writes. I'm planning on taking some time tomorrow to write a
torture tester for Pg's ENOSPC handling and to verify ENOSPC handling in
the test case I linked to in my original StackOverflow post.

If this is just an EIO issue then I see no point doing anything other than
PANICing unconditionally.

If it's a concern for ENOSPC too, we should try harder to fail more nicely
whenever we possibly can.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

On 2018-04-01 03:14:46 +0200, Anthony Iliopoulos wrote:
> On Sat, Mar 31, 2018 at 12:38:12PM -0400, Tom Lane wrote:
> > Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> > > So we should just use the big hammer here.
> >
> > And bitch, loudly and publicly, about how broken this kernel behavior is.
> > If we make enough of a stink maybe it'll get fixed.
>
> It is not likely to be fixed (beyond what has been done already with the
> manpage patches and errseq_t fixes on the reporting level). The issue is,
> the kernel needs to deal with hard IO errors at that level somehow, and
> since those errors typically persist, re-dirtying the pages would not
> really solve the problem (unless some filesystem remaps the request to a
> different block, assuming the device is alive).

Throwing away the dirty pages *and* persisting the error seems a lot
more reasonable. Then provide a fcntl (or whatever) extension that can
clear the error status in the few cases that the application that wants
to gracefully deal with the case.

> Keeping around dirty
> pages that cannot possibly be written out is essentially a memory leak,
> as those pages would stay around even after the application has exited.

Why do dirty pages need to be kept around in the case of persistent
errors? I don't think the lack of automatic recovery in that case is
what anybody is complaining about. It's that the error goes away and
there's no reasonable way to separate out such an error from some
potential transient errors.

Greetings,

Andres Freund

On Mon, Apr 02, 2018 at 11:13:46AM -0700, Andres Freund wrote:
> Hi,
>
> On 2018-04-01 03:14:46 +0200, Anthony Iliopoulos wrote:
> > On Sat, Mar 31, 2018 at 12:38:12PM -0400, Tom Lane wrote:
> > > Craig Ringer <craig(at)2ndquadrant(dot)com> writes:
> > > > So we should just use the big hammer here.
> > >
> > > And bitch, loudly and publicly, about how broken this kernel behavior is.
> > > If we make enough of a stink maybe it'll get fixed.
> >
> > It is not likely to be fixed (beyond what has been done already with the
> > manpage patches and errseq_t fixes on the reporting level). The issue is,
> > the kernel needs to deal with hard IO errors at that level somehow, and
> > since those errors typically persist, re-dirtying the pages would not
> > really solve the problem (unless some filesystem remaps the request to a
> > different block, assuming the device is alive).
>
> Throwing away the dirty pages *and* persisting the error seems a lot
> more reasonable. Then provide a fcntl (or whatever) extension that can
> clear the error status in the few cases that the application that wants
> to gracefully deal with the case.

Given precisely that the dirty pages which cannot been written-out are
practically thrown away, the semantics of fsync() (after the 4.13 fixes)
are essentially correct: the first call indicates that a writeback error
indeed occurred, while subsequent calls have no reason to indicate an error
(assuming no other errors occurred in the meantime).

The error reporting is thus consistent with the intended semantics (which
are sadly not properly documented). Repeated calls to fsync() simply do not
imply that the kernel will retry to writeback the previously-failed pages,
so the application needs to be aware of that. Persisting the error at the
fsync() level would essentially mean moving application policy into the
kernel.

Best regards,
Anthony

On 2018-04-02 20:53:20 +0200, Anthony Iliopoulos wrote:
> On Mon, Apr 02, 2018 at 11:13:46AM -0700, Andres Freund wrote:
> > Throwing away the dirty pages *and* persisting the error seems a lot
> > more reasonable. Then provide a fcntl (or whatever) extension that can
> > clear the error status in the few cases that the application that wants
> > to gracefully deal with the case.
>
> Given precisely that the dirty pages which cannot been written-out are
> practically thrown away, the semantics of fsync() (after the 4.13 fixes)
> are essentially correct: the first call indicates that a writeback error
> indeed occurred, while subsequent calls have no reason to indicate an error
> (assuming no other errors occurred in the meantime).

Meh^2.

"no reason" - except that there's absolutely no way to know what state
the data is in. And that your application needs explicit handling of
such failures. And that one FD might be used in a lots of different
parts of the application, that fsyncs in one part of the application
might be an ok failure, and in another not. Requiring explicit actions
to acknowledge "we've thrown away your data for unknown reason" seems
entirely reasonable.

> The error reporting is thus consistent with the intended semantics (which
> are sadly not properly documented). Repeated calls to fsync() simply do not
> imply that the kernel will retry to writeback the previously-failed pages,
> so the application needs to be aware of that.

Which isn't what I've suggested.

> Persisting the error at the fsync() level would essentially mean
> moving application policy into the kernel.

Meh.

Greetings,

Andres Freund

On Mon, Apr 02, 2018 at 12:32:45PM -0700, Andres Freund wrote:
> On 2018-04-02 20:53:20 +0200, Anthony Iliopoulos wrote:
> > On Mon, Apr 02, 2018 at 11:13:46AM -0700, Andres Freund wrote:
> > > Throwing away the dirty pages *and* persisting the error seems a lot
> > > more reasonable. Then provide a fcntl (or whatever) extension that can
> > > clear the error status in the few cases that the application that wants
> > > to gracefully deal with the case.
> >
> > Given precisely that the dirty pages which cannot been written-out are
> > practically thrown away, the semantics of fsync() (after the 4.13 fixes)
> > are essentially correct: the first call indicates that a writeback error
> > indeed occurred, while subsequent calls have no reason to indicate an error
> > (assuming no other errors occurred in the meantime).
>
> Meh^2.
>
> "no reason" - except that there's absolutely no way to know what state
> the data is in. And that your application needs explicit handling of
> such failures. And that one FD might be used in a lots of different
> parts of the application, that fsyncs in one part of the application
> might be an ok failure, and in another not. Requiring explicit actions
> to acknowledge "we've thrown away your data for unknown reason" seems
> entirely reasonable.

As long as fsync() indicates error on first invocation, the application
is fully aware that between this point of time and the last call to fsync()
data has been lost. Persisting this error any further does not change this
or add any new info - on the contrary it adds confusion as subsequent write()s
and fsync()s on other pages can succeed, but will be reported as failures.

The application will need to deal with that first error irrespective of
subsequent return codes from fsync(). Conceptually every fsync() invocation
demarcates an epoch for which it reports potential errors, so the caller
needs to take responsibility for that particular epoch.

Callers that are not affected by the potential outcome of fsync() and
do not react on errors, have no reason for calling it in the first place
(and thus masking failure from subsequent callers that may indeed care).

Best regards,
Anthony

Greetings,

* Anthony Iliopoulos (ailiop(at)altatus(dot)com) wrote:
> On Mon, Apr 02, 2018 at 12:32:45PM -0700, Andres Freund wrote:
> > On 2018-04-02 20:53:20 +0200, Anthony Iliopoulos wrote:
> > > On Mon, Apr 02, 2018 at 11:13:46AM -0700, Andres Freund wrote:
> > > > Throwing away the dirty pages *and* persisting the error seems a lot
> > > > more reasonable. Then provide a fcntl (or whatever) extension that can
> > > > clear the error status in the few cases that the application that wants
> > > > to gracefully deal with the case.
> > >
> > > Given precisely that the dirty pages which cannot been written-out are
> > > practically thrown away, the semantics of fsync() (after the 4.13 fixes)
> > > are essentially correct: the first call indicates that a writeback error
> > > indeed occurred, while subsequent calls have no reason to indicate an error
> > > (assuming no other errors occurred in the meantime).
> >
> > Meh^2.
> >
> > "no reason" - except that there's absolutely no way to know what state
> > the data is in. And that your application needs explicit handling of
> > such failures. And that one FD might be used in a lots of different
> > parts of the application, that fsyncs in one part of the application
> > might be an ok failure, and in another not. Requiring explicit actions
> > to acknowledge "we've thrown away your data for unknown reason" seems
> > entirely reasonable.
>
> As long as fsync() indicates error on first invocation, the application
> is fully aware that between this point of time and the last call to fsync()
> data has been lost. Persisting this error any further does not change this
> or add any new info - on the contrary it adds confusion as subsequent write()s
> and fsync()s on other pages can succeed, but will be reported as failures.

fsync() doesn't reflect the status of given pages, however, it reflects
the status of the file descriptor, and as such the file, on which it's
called. This notion that fsync() is actually only responsible for the
changes which were made to a file since the last fsync() call is pure
foolishness. If we were able to pass a list of pages or data ranges to
fsync() for it to verify they're on disk then perhaps things would be
different, but we can't, all we can do is ask to "please flush all the
dirty pages associated with this file descriptor, which represents this
file we opened, to disk, and let us know if you were successful."

Give us a way to ask "are these specific pages written out to persistant
storage?" and we would certainly be happy to use it, and to repeatedly
try to flush out pages which weren't synced to disk due to some
transient error, and to track those cases and make sure that we don't
incorrectly assume that they've been transferred to persistent storage.

> The application will need to deal with that first error irrespective of
> subsequent return codes from fsync(). Conceptually every fsync() invocation
> demarcates an epoch for which it reports potential errors, so the caller
> needs to take responsibility for that particular epoch.

We do deal with that error- by realizing that it failed and later
*retrying* the fsync(), which is when we get back an "all good!
everything with this file descriptor you've opened is sync'd!" and
happily expect that to be truth, when, in reality, it's an unfortunate
lie and there are still pages associated with that file descriptor which
are, in reality, dirty and not sync'd to disk.

Consider two independent programs where the first one writes to a file
and then calls the second one whose job it is to go out and fsync(),
perhaps async from the first, those files. Is the second program
supposed to go write to each page that the first one wrote to, in order
to ensure that all the dirty bits are set so that the fsync() will
actually return if all the dirty pages are written?

> Callers that are not affected by the potential outcome of fsync() and
> do not react on errors, have no reason for calling it in the first place
> (and thus masking failure from subsequent callers that may indeed care).

Reacting on an error from an fsync() call could, based on how it's
documented and actually implemented in other OS's, mean "run another
fsync() to see if the error has resolved itself." Requiring that to
mean "you have to go dirty all of the pages you previously dirtied to
actually get a subsequent fsync() to do anything" is really just not
reasonable- a given program may have no idea what was written to
previously nor any particular reason to need to know, on the expectation
that the fsync() call will flush any dirty pages, as it's documented to
do.

Thanks!

Stephen

Hi Stephen,

On Mon, Apr 02, 2018 at 04:58:08PM -0400, Stephen Frost wrote:
>
> fsync() doesn't reflect the status of given pages, however, it reflects
> the status of the file descriptor, and as such the file, on which it's
> called. This notion that fsync() is actually only responsible for the
> changes which were made to a file since the last fsync() call is pure
> foolishness. If we were able to pass a list of pages or data ranges to
> fsync() for it to verify they're on disk then perhaps things would be
> different, but we can't, all we can do is ask to "please flush all the
> dirty pages associated with this file descriptor, which represents this
> file we opened, to disk, and let us know if you were successful."
>
> Give us a way to ask "are these specific pages written out to persistant
> storage?" and we would certainly be happy to use it, and to repeatedly
> try to flush out pages which weren't synced to disk due to some
> transient error, and to track those cases and make sure that we don't
> incorrectly assume that they've been transferred to persistent storage.

Indeed fsync() is simply a rather blunt instrument and a narrow legacy
interface but further changing its established semantics (no matter how
unreasonable they may be) is probably not the way to go.

Would using sync_file_range() be helpful? Potential errors would only
apply to pages that cover the requested file ranges. There are a few
caveats though:

(a) it still messes with the top-level error reporting so mixing it
with callers that use fsync() and do care about errors will produce
the same issue (clearing the error status).

(b) the error-reporting granularity is coarse (failure reporting applies
to the entire requested range so you still don't know which particular
pages/file sub-ranges failed writeback)

(c) the same "report and forget" semantics apply to repeated invocations
of the sync_file_range() call, so again action will need to be taken
upon first error encountered for the particular ranges.

> > The application will need to deal with that first error irrespective of
> > subsequent return codes from fsync(). Conceptually every fsync() invocation
> > demarcates an epoch for which it reports potential errors, so the caller
> > needs to take responsibility for that particular epoch.
>
> We do deal with that error- by realizing that it failed and later
> *retrying* the fsync(), which is when we get back an "all good!
> everything with this file descriptor you've opened is sync'd!" and
> happily expect that to be truth, when, in reality, it's an unfortunate
> lie and there are still pages associated with that file descriptor which
> are, in reality, dirty and not sync'd to disk.

It really turns out that this is not how the fsync() semantics work
though, exactly because the nature of the errors: even if the kernel
retained the dirty bits on the failed pages, retrying persisting them
on the same disk location would simply fail. Instead the kernel opts
for marking those pages clean (since there is no other recovery
strategy), and reporting once to the caller who can potentially deal
with it in some manner. It is sadly a bad and undocumented convention.

> Consider two independent programs where the first one writes to a file
> and then calls the second one whose job it is to go out and fsync(),
> perhaps async from the first, those files. Is the second program
> supposed to go write to each page that the first one wrote to, in order
> to ensure that all the dirty bits are set so that the fsync() will
> actually return if all the dirty pages are written?

I think what you have in mind are the semantics of sync() rather
than fsync(), but as long as an application needs to ensure data
are persisted to storage, it needs to retain those data in its heap
until fsync() is successful instead of discarding them and relying
on the kernel after write(). The pattern should be roughly like:
write() -> fsync() -> free(), rather than write() -> free() -> fsync().
For example, if a partition gets full upon fsync(), then the application
has a chance to persist the data in a different location, while
the kernel cannot possibly make this decision and recover.

> > Callers that are not affected by the potential outcome of fsync() and
> > do not react on errors, have no reason for calling it in the first place
> > (and thus masking failure from subsequent callers that may indeed care).
>
> Reacting on an error from an fsync() call could, based on how it's
> documented and actually implemented in other OS's, mean "run another
> fsync() to see if the error has resolved itself." Requiring that to
> mean "you have to go dirty all of the pages you previously dirtied to
> actually get a subsequent fsync() to do anything" is really just not
> reasonable- a given program may have no idea what was written to
> previously nor any particular reason to need to know, on the expectation
> that the fsync() call will flush any dirty pages, as it's documented to
> do.

I think we are conflating a few issues here: having the OS kernel being
responsible for error recovery (so that subsequent fsync() would fix
the problems) is one. This clearly is a design which most kernels have
not really adopted for reasons outlined above (although having the FS
layer recovering from hard errors transparently is open for discussion
from what it seems [1]). Now, there is the issue of granularity of
error reporting: userspace could benefit from a fine-grained indication
of failed pages (or file ranges). Another issue is that of reporting
semantics (report and clear), which is also a design choice made to
avoid having higher-resolution error tracking and the corresponding
memory overheads [1].

Best regards,
Anthony

[1] https://lwn.net/Articles/718734/

On 2018-04-03 01:05:44 +0200, Anthony Iliopoulos wrote:
> Would using sync_file_range() be helpful? Potential errors would only
> apply to pages that cover the requested file ranges. There are a few
> caveats though:

To quote sync_file_range(2):
Warning
This system call is extremely dangerous and should not be used in portable programs. None of these operations writes out the
file's metadata. Therefore, unless the application is strictly performing overwrites of already-instantiated disk blocks, there
are no guarantees that the data will be available after a crash. There is no user interface to know if a write is purely an over‐
write. On filesystems using copy-on-write semantics (e.g., btrfs) an overwrite of existing allocated blocks is impossible. When
writing into preallocated space, many filesystems also require calls into the block allocator, which this system call does not
sync out to disk. This system call does not flush disk write caches and thus does not provide any data integrity on systems with
volatile disk write caches.

Given the lack of metadata safety that seems entirely a no go. We use
sfr(2), but only to force the kernel's hand around writing back earlier
without throwing away cache contents.

> > > The application will need to deal with that first error irrespective of
> > > subsequent return codes from fsync(). Conceptually every fsync() invocation
> > > demarcates an epoch for which it reports potential errors, so the caller
> > > needs to take responsibility for that particular epoch.
> >
> > We do deal with that error- by realizing that it failed and later
> > *retrying* the fsync(), which is when we get back an "all good!
> > everything with this file descriptor you've opened is sync'd!" and
> > happily expect that to be truth, when, in reality, it's an unfortunate
> > lie and there are still pages associated with that file descriptor which
> > are, in reality, dirty and not sync'd to disk.
>
> It really turns out that this is not how the fsync() semantics work
> though

Except on freebsd and solaris, and perhaps others.

>, exactly because the nature of the errors: even if the kernel
> retained the dirty bits on the failed pages, retrying persisting them
> on the same disk location would simply fail.

That's not guaranteed at all, think NFS.

> Instead the kernel opts for marking those pages clean (since there is
> no other recovery strategy), and reporting once to the caller who can
> potentially deal with it in some manner. It is sadly a bad and
> undocumented convention.

It's broken behaviour justified post facto with the only rational that
was available, which explains why it's so unconvincing. You could just
say "this ship has sailed, and it's to onerous to change because xxx"
and this'd be a done deal. But claiming this is reasonable behaviour is
ridiculous.

Again, you could just continue to error for this fd and still throw away
the data.

> > Consider two independent programs where the first one writes to a file
> > and then calls the second one whose job it is to go out and fsync(),
> > perhaps async from the first, those files. Is the second program
> > supposed to go write to each page that the first one wrote to, in order
> > to ensure that all the dirty bits are set so that the fsync() will
> > actually return if all the dirty pages are written?
>
> I think what you have in mind are the semantics of sync() rather
> than fsync()

If you open the same file with two fds, and write with one, and fsync
with another that's definitely supposed to work. And sync() isn't a
realistic replacement in any sort of way because it's obviously
systemwide, and thus entirely and completely unsuitable. Nor does it
have any sort of better error reporting behaviour, does it?

Greetings,

Andres Freund

On 3 April 2018 at 07:05, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:

> Hi Stephen,
>
> On Mon, Apr 02, 2018 at 04:58:08PM -0400, Stephen Frost wrote:
> >
> > fsync() doesn't reflect the status of given pages, however, it reflects
> > the status of the file descriptor, and as such the file, on which it's
> > called. This notion that fsync() is actually only responsible for the
> > changes which were made to a file since the last fsync() call is pure
> > foolishness. If we were able to pass a list of pages or data ranges to
> > fsync() for it to verify they're on disk then perhaps things would be
> > different, but we can't, all we can do is ask to "please flush all the
> > dirty pages associated with this file descriptor, which represents this
> > file we opened, to disk, and let us know if you were successful."
> >
> > Give us a way to ask "are these specific pages written out to persistant
> > storage?" and we would certainly be happy to use it, and to repeatedly
> > try to flush out pages which weren't synced to disk due to some
> > transient error, and to track those cases and make sure that we don't
> > incorrectly assume that they've been transferred to persistent storage.
>
> Indeed fsync() is simply a rather blunt instrument and a narrow legacy
> interface but further changing its established semantics (no matter how
> unreasonable they may be) is probably not the way to go.
>

They're undocumented and extremely surprising semantics that are arguably a
violation of the POSIX spec for fsync(), or at least a surprising
interpretation of it.

So I don't buy this argument.

It really turns out that this is not how the fsync() semantics work
> though, exactly because the nature of the errors: even if the kernel
> retained the dirty bits on the failed pages, retrying persisting them
> on the same disk location would simply fail.

*might* simply fail.

It depends on why the error ocurred.

I originally identified this behaviour on a multipath system. Multipath
defaults to "throw the writes away, nobody really cares anyway" on error.
It seems to figure a higher level will retry, or the application will
receive the error and retry.

(See no_path_retry in multipath config. AFAICS the default is insanely
dangerous and only suitable for specialist apps that understand the quirks;
you should use no_path_retry=queue).

Instead the kernel opts
> for marking those pages clean (since there is no other recovery
> strategy),

and reporting once to the caller who can potentially deal
> with it in some manner. It is sadly a bad and undocumented convention.
>

It could mark the FD.

It's not just undocumented, it's a slightly creative interpretation of the
POSIX spec for fsync.

> > Consider two independent programs where the first one writes to a file
> > and then calls the second one whose job it is to go out and fsync(),
> > perhaps async from the first, those files. Is the second program
> > supposed to go write to each page that the first one wrote to, in order
> > to ensure that all the dirty bits are set so that the fsync() will
> > actually return if all the dirty pages are written?
>
> I think what you have in mind are the semantics of sync() rather
> than fsync(), but as long as an application needs to ensure data
> are persisted to storage, it needs to retain those data in its heap
> until fsync() is successful instead of discarding them and relying
> on the kernel after write().

This is almost exactly what we tell application authors using PostgreSQL:
the data isn't written until you receive a successful commit confirmation,
so you'd better not forget it.

We provide applications with *clear boundaries* so they can know *exactly*
what was, and was not, written. I guess the argument from the kernel is the
same is true: whatever was written since the last *successful* fsync is
potentially lost and must be redone.

But the fsync behaviour is utterly undocumented and dubiously standard.

> I think we are conflating a few issues here: having the OS kernel being
> responsible for error recovery (so that subsequent fsync() would fix
> the problems) is one. This clearly is a design which most kernels have
> not really adopted for reasons outlined above

[citation needed]

What do other major platforms do here? The post above suggests it's a bit
of a mix of behaviours.

Now, there is the issue of granularity of
> error reporting: userspace could benefit from a fine-grained indication
> of failed pages (or file ranges).

Yep. I looked at AIO in the hopes that, if we used AIO, we'd be able to map
a sync failure back to an individual AIO write.

But it seems AIO just adds more problems and fixes none. Flush behaviour
with AIO from what I can tell is inconsistent version to version and
generally unhelpful. The kernel should really report such sync failures
back to the app on its AIO write mapping, but it seems nothing of the sort
happens.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

> On Apr 2, 2018, at 16:27, Craig Ringer <craig(at)2ndQuadrant(dot)com> wrote:
>
> They're undocumented and extremely surprising semantics that are arguably a violation of the POSIX spec for fsync(), or at least a surprising interpretation of it.

Even accepting that (I personally go with surprising over violation, as if my vote counted), it is highly unlikely that we will convince every kernel team to declare "What fools we've been!" and push a change... and even if they did, PostgreSQL can look forward to many years of running on kernels with the broken semantics. Given that, I think the PANIC option is the soundest one, as unappetizing as it is.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

On April 2, 2018 5:03:39 PM PDT, Christophe Pettus <xof(at)thebuild(dot)com> wrote:
>
>> On Apr 2, 2018, at 16:27, Craig Ringer <craig(at)2ndQuadrant(dot)com> wrote:
>>
>> They're undocumented and extremely surprising semantics that are
>arguably a violation of the POSIX spec for fsync(), or at least a
>surprising interpretation of it.
>
>Even accepting that (I personally go with surprising over violation, as
>if my vote counted), it is highly unlikely that we will convince every
>kernel team to declare "What fools we've been!" and push a change...
>and even if they did, PostgreSQL can look forward to many years of
>running on kernels with the broken semantics. Given that, I think the
>PANIC option is the soundest one, as unappetizing as it is.

Don't we pretty much already have agreement in that? And Craig is the main proponent of it?

Andres

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

> On Apr 2, 2018, at 17:05, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Don't we pretty much already have agreement in that? And Craig is the main proponent of it?

For sure on the second sentence; the first was not clear to me.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

On Mon, Apr 2, 2018 at 5:05 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>Even accepting that (I personally go with surprising over violation, as
>>if my vote counted), it is highly unlikely that we will convince every
>>kernel team to declare "What fools we've been!" and push a change...
>>and even if they did, PostgreSQL can look forward to many years of
>>running on kernels with the broken semantics. Given that, I think the
>>PANIC option is the soundest one, as unappetizing as it is.
>
> Don't we pretty much already have agreement in that? And Craig is the main proponent of it?

I wonder how bad it will be in practice if we PANIC. Craig said "This
isn't as bad as it seems because AFAICS fsync only returns EIO in
cases where we should be stopping the world anyway, and many FSes will
do that for us". It would be nice to get more information on that.

--
Peter Geoghegan

On Tue, Apr 3, 2018 at 3:03 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> I see little benefit to not just PANICing unconditionally on EIO, really. It
> shouldn't happen, and if it does, we want to be pretty conservative and
> adopt a data-protective approach.
>
> I'm rather more worried by doing it on ENOSPC. Which looks like it might be
> necessary from what I recall finding in my test case + kernel code reading.
> I really don't want to respond to a possibly-transient ENOSPC by PANICing
> the whole server unnecessarily.

Yeah, it'd be nice to give an administrator the chance to free up some
disk space after ENOSPC is reported, and stay up. Running out of
space really shouldn't take down the database without warning! The
question is whether the data remains in cache and marked dirty, so
that retrying is a safe option (since it's potentially gone from our
own buffers, so if the OS doesn't have it the only place your
committed data can definitely still be found is the WAL... recovery
time). Who can tell us? Do we need a per-filesystem answer? Delayed
allocation is a somewhat filesystem-specific thing, so maybe.
Interestingly, there don't seem to be many operating systems that can
report ENOSPC from fsync(), based on a quick scan through some
documentation:

POSIX, AIX, HP-UX, FreeBSD, OpenBSD, NetBSD: no
Illumos/Solaris, Linux, macOS: yes

I don't know if macOS really means it or not; it just tells you to see
the errors for read(2) and write(2). By the way, speaking of macOS, I
was curious to see if the common BSD heritage would show here. Yeah,
somewhat. It doesn't appear to keep buffers on writeback error, if
this is the right code[1] (though it could be handling it somewhere
else for all I know).

[1] https://github.com/apple/darwin-xnu/blob/master/bsd/vfs/vfs_bio.c#L2695

--
Thomas Munro
http://www.enterprisedb.com

On Mon, Apr 2, 2018 at 2:53 PM, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:
> Given precisely that the dirty pages which cannot been written-out are
> practically thrown away, the semantics of fsync() (after the 4.13 fixes)
> are essentially correct: the first call indicates that a writeback error
> indeed occurred, while subsequent calls have no reason to indicate an error
> (assuming no other errors occurred in the meantime).

Like other people here, I think this is 100% unreasonable, starting
with "the dirty pages which cannot been written out are practically
thrown away". Who decided that was OK, and on the basis of what
wording in what specification? I think it's always unreasonable to
throw away the user's data. If the writes are going to fail, then let
them keep on failing every time. *That* wouldn't cause any data loss,
because we'd never be able to checkpoint, and eventually the user
would have to kill the server uncleanly, and that would trigger
recovery.

Also, this really does make it impossible to write reliable programs.
Imagine that, while the server is running, somebody runs a program
which opens a file in the data directory, calls fsync() on it, and
closes it. If the fsync() fails, postgres is now borked and has no
way of being aware of the problem. If we knew, we could PANIC, but
we'll never find out, because the unrelated process ate the error.
This is exactly the sort of ill-considered behavior that makes fcntl()
locking nearly useless.

Even leaving that aside, a PANIC means a prolonged outage on a
prolonged system - it could easily take tens of minutes or longer to
run recovery. So saying "oh, just do that" is not really an answer.
Sure, we can do it, but it's like trying to lose weight by
intentionally eating a tapeworm. Now, it's possible to shorten the
checkpoint_timeout so that recovery runs faster, but then performance
drops because data has to be fsync()'d more often instead of getting
buffered in the OS cache for the maximum possible time. We could also
dodge this issue in another way: suppose that when we write a page
out, we don't consider it really written until fsync() succeeds. Then
we wouldn't need to PANIC if an fsync() fails; we could just re-write
the page. Unfortunately, this would also be terrible for performance,
for pretty much the same reasons: letting the OS cache absorb lots of
dirty blocks and do write-combining is *necessary* for good
performance.

> The error reporting is thus consistent with the intended semantics (which
> are sadly not properly documented). Repeated calls to fsync() simply do not
> imply that the kernel will retry to writeback the previously-failed pages,
> so the application needs to be aware of that. Persisting the error at the
> fsync() level would essentially mean moving application policy into the
> kernel.

I might accept this argument if I accepted that it was OK to decide
that an fsync() failure means you can forget that the write() ever
happened in the first place, but it's hard to imagine an application
that wants that behavior. If the application didn't care about
whether the bytes really got to disk or not, it would not have called
fsync() in the first place. If it does care, reporting the error only
once is never an improvement.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Apr 2, 2018 at 7:54 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Also, this really does make it impossible to write reliable programs.
> Imagine that, while the server is running, somebody runs a program
> which opens a file in the data directory, calls fsync() on it, and
> closes it. If the fsync() fails, postgres is now borked and has no
> way of being aware of the problem. If we knew, we could PANIC, but
> we'll never find out, because the unrelated process ate the error.
> This is exactly the sort of ill-considered behavior that makes fcntl()
> locking nearly useless.

I fear that the conventional wisdom from the Kernel people is now "you
should be using O_DIRECT for granular control". The LWN article
Thomas linked (https://lwn.net/Articles/718734) cites Ted Ts'o:

"Monakhov asked why a counter was needed; Layton said it was to handle
multiple overlapping writebacks. Effectively, the counter would record
whether a writeback had failed since the file was opened or since the
last fsync(). Ts'o said that should be fine; applications that want
more information should use O_DIRECT. For most applications, knowledge
that an error occurred somewhere in the file is all that is necessary;
applications that require better granularity already use O_DIRECT."

--
Peter Geoghegan

Hi Robert,

On Mon, Apr 02, 2018 at 10:54:26PM -0400, Robert Haas wrote:
> On Mon, Apr 2, 2018 at 2:53 PM, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:
> > Given precisely that the dirty pages which cannot been written-out are
> > practically thrown away, the semantics of fsync() (after the 4.13 fixes)
> > are essentially correct: the first call indicates that a writeback error
> > indeed occurred, while subsequent calls have no reason to indicate an error
> > (assuming no other errors occurred in the meantime).
>
> Like other people here, I think this is 100% unreasonable, starting
> with "the dirty pages which cannot been written out are practically
> thrown away". Who decided that was OK, and on the basis of what
> wording in what specification? I think it's always unreasonable to

If you insist on strict conformance to POSIX, indeed the linux
glibc configuration and associated manpage are probably wrong in
stating that _POSIX_SYNCHRONIZED_IO is supported. The implementation
matches that of the flexibility allowed by not supporting SIO.
There's a long history of brokenness between linux and posix,
and I think there was never an intention of conforming to the
standard.

> throw away the user's data. If the writes are going to fail, then let
> them keep on failing every time. *That* wouldn't cause any data loss,
> because we'd never be able to checkpoint, and eventually the user
> would have to kill the server uncleanly, and that would trigger
> recovery.

I believe (as tried to explain earlier) there is a certain assumption
being made that the writer and original owner of data is responsible
for dealing with potential errors in order to avoid data loss (which
should be only of interest to the original writer anyway). It would
be very questionable for the interface to persist the error while
subsequent writes and fsyncs to different offsets may as well go through.
Another process may need to write into the file and fsync, while being
unaware of those newly introduced semantics is now faced with EIO
because some unrelated previous process failed some earlier writes
and did not bother to clear the error for those writes. In a similar
scenario where the second process is aware of the new semantics, it would
naturally go ahead and clear the global error in order to proceed
with its own write()+fsync(), which would essentially amount to the
same problematic semantics you have now.

> Also, this really does make it impossible to write reliable programs.
> Imagine that, while the server is running, somebody runs a program
> which opens a file in the data directory, calls fsync() on it, and
> closes it. If the fsync() fails, postgres is now borked and has no
> way of being aware of the problem. If we knew, we could PANIC, but
> we'll never find out, because the unrelated process ate the error.
> This is exactly the sort of ill-considered behavior that makes fcntl()
> locking nearly useless.

Fully agree, and the errseq_t fixes have dealt exactly with the issue
of making sure that the error is reported to all file descriptors that
*happen to be open at the time of error*. But I think one would have a
hard time defending a modification to the kernel where this is further
extended to cover cases where:

process A does write() on some file offset which fails writeback,
fsync() gets EIO and exit()s.

process B does write() on some other offset which succeeds writeback,
but fsync() gets EIO due to (uncleared) failures of earlier process.

This would be a highly user-visible change of semantics from edge-
triggered to level-triggered behavior.

> dodge this issue in another way: suppose that when we write a page
> out, we don't consider it really written until fsync() succeeds. Then

That's the only way to think about fsync() guarantees unless you
are on a kernel that keeps retrying to persist dirty pages. Assuming
such a model, after repeated and unrecoverable hard failures the
process would have to explicitly inform the kernel to drop the dirty
pages. All the process could do at that point is read back to userspace
the dirty/failed pages and attempt to rewrite them at a different place
(which is current possible too). Most applications would not bother
though to inform the kernel and drop the permanently failed pages;
and thus someone eventually would hit the case that a large amount
of failed writeback pages are running his server out of memory,
at which point people will complain that those semantics are completely
unreasonable.

> we wouldn't need to PANIC if an fsync() fails; we could just re-write
> the page. Unfortunately, this would also be terrible for performance,
> for pretty much the same reasons: letting the OS cache absorb lots of
> dirty blocks and do write-combining is *necessary* for good
> performance.

Not sure I understand this case. The application may indeed re-write
a bunch of pages that have failed and proceed with fsync(). The kernel
will deal with combining the writeback of all the re-written pages. But
further the necessity of combining for performance really depends on
the exact storage medium. At the point you start caring about
write-combining, the kernel community will naturally redirect you to
use DIRECT_IO.

> > The error reporting is thus consistent with the intended semantics (which
> > are sadly not properly documented). Repeated calls to fsync() simply do not
> > imply that the kernel will retry to writeback the previously-failed pages,
> > so the application needs to be aware of that. Persisting the error at the
> > fsync() level would essentially mean moving application policy into the
> > kernel.
>
> I might accept this argument if I accepted that it was OK to decide
> that an fsync() failure means you can forget that the write() ever
> happened in the first place, but it's hard to imagine an application
> that wants that behavior. If the application didn't care about
> whether the bytes really got to disk or not, it would not have called
> fsync() in the first place. If it does care, reporting the error only
> once is never an improvement.

Again, conflating two separate issues, that of buffering and retrying
failed pages and that of error reporting. Yes it would be convenient
for applications not to have to care at all about recovery of failed
write-backs, but at some point they would have to face this issue one
way or another (I am assuming we are always talking about hard failures,
other kinds of failures are probably already being dealt with transparently
at the kernel level).

As for the reporting, it is also unreasonable to effectively signal
and persist an error on a file-wide granularity while it pertains
to subsets of that file and other writes can go through, but I am
repeating myself.

I suppose that if the check-and-clear semantics are problematic for
Pg, one could suggest a kernel patch that opts-in to a level-triggered
reporting of fsync() on a per-descriptor basis, which seems to be
non-intrusive and probably sufficient to cover your expected use-case.

Best regards,
Anthony

On 3 April 2018 at 11:35, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:
> Hi Robert,
>
> Fully agree, and the errseq_t fixes have dealt exactly with the issue
> of making sure that the error is reported to all file descriptors that
> *happen to be open at the time of error*. But I think one would have a
> hard time defending a modification to the kernel where this is further
> extended to cover cases where:
>
> process A does write() on some file offset which fails writeback,
> fsync() gets EIO and exit()s.
>
> process B does write() on some other offset which succeeds writeback,
> but fsync() gets EIO due to (uncleared) failures of earlier process.

Surely that's exactly what process B would want? If it calls fsync and
gets a success and later finds out that the file is corrupt and didn't
match what was in memory it's not going to be happy.

This seems like an attempt to co-opt fsync for a new and different
purpose for which it's poorly designed. It's not an async error
reporting mechanism for writes. It would be useless as that as any
process could come along and open your file and eat the errors for
writes you performed. An async error reporting mechanism would have to
document which writes it was giving errors for and give you ways to
control that.

The semantics described here are useless for everyone. For a program
needing to know the error status of the writes it executed, it doesn't
know which writes are included in which fsync call. For a program
using fsync for its original intended purpose of guaranteeing that the
all writes are synced to disk it no longer has any guarantee at all.

> This would be a highly user-visible change of semantics from edge-
> triggered to level-triggered behavior.

It was always documented as level-triggered. This edge-triggered
concept is a completely surprise to application writers.

--
greg

On Tue, Apr 03, 2018 at 12:26:05PM +0100, Greg Stark wrote:
> On 3 April 2018 at 11:35, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:
> > Hi Robert,
> >
> > Fully agree, and the errseq_t fixes have dealt exactly with the issue
> > of making sure that the error is reported to all file descriptors that
> > *happen to be open at the time of error*. But I think one would have a
> > hard time defending a modification to the kernel where this is further
> > extended to cover cases where:
> >
> > process A does write() on some file offset which fails writeback,
> > fsync() gets EIO and exit()s.
> >
> > process B does write() on some other offset which succeeds writeback,
> > but fsync() gets EIO due to (uncleared) failures of earlier process.
>
>
> Surely that's exactly what process B would want? If it calls fsync and
> gets a success and later finds out that the file is corrupt and didn't
> match what was in memory it's not going to be happy.

You can't possibly make this assumption. Process B may be reading
and writing to completely disjoint regions from those of process A,
and as such not really caring about earlier failures, only wanting
to ensure its own writes go all the way through. But even if it did
care, the file interfaces make no transactional guarantees. Even
without fsync() there is nothing preventing process B from reading
dirty pages from process A, and based on their content proceed to
to its own business and write/persist new data, while process A
further modifies the not-yet-flushed pages in-memory before flushing.
In this case you'd need explicit synchronization/locking between
the processes anyway, so why would fsync() be an exception?

> This seems like an attempt to co-opt fsync for a new and different
> purpose for which it's poorly designed. It's not an async error
> reporting mechanism for writes. It would be useless as that as any
> process could come along and open your file and eat the errors for
> writes you performed. An async error reporting mechanism would have to
> document which writes it was giving errors for and give you ways to
> control that.

The errseq_t fixes deal with that; errors will be reported to any
process that has an open fd, irrespective to who is the actual caller
of the fsync() that may have induced errors. This is anyway required
as the kernel may evict dirty pages on its own by doing writeback and
as such there needs to be a way to report errors on all open fds.

> The semantics described here are useless for everyone. For a program
> needing to know the error status of the writes it executed, it doesn't
> know which writes are included in which fsync call. For a program

If EIO persists between invocations until explicitly cleared, a process
cannot possibly make any decision as to if it should clear the error
and proceed or some other process will need to leverage that without
coordination, or which writes actually failed for that matter.
We would be back to the case of requiring explicit synchronization
between processes that care about this, in which case the processes
may as well synchronize over calling fsync() in the first place.

Having an opt-in persisting EIO per-fd would practically be a form
of "contract" between "cooperating" processes anyway.

But instead of deconstructing and debating the semantics of the
current mechanism, why not come up with the ideal desired form of
error reporting/tracking granularity etc., and see how this may be
fitted into kernels as a new interface.

Best regards,
Anthony

On 3 April 2018 at 10:54, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> I think it's always unreasonable to
> throw away the user's data.

Well, we do that. If a txn aborts, all writes in the txn are discarded.

I think that's perfectly reasonable. Though we also promise an all or
nothing effect, we make exceptions even there.

The FS doesn't offer transactional semantics, but the fsync behaviour can
be interpreted kind of similarly.

I don't *agree* with it, but I don't think it's as wholly unreasonable as
all that. I think leaving it undocumented is absolutely gobsmacking, and
it's dubious at best, but it's not totally insane.

> If the writes are going to fail, then let
> them keep on failing every time.

Like we do, where we require an explicit rollback.

But POSIX may pose issues there, it doesn't really define any interface for
that AFAIK. Unless you expect the app to close() and re-open() the file.
Replacing one nonstandard issue with another may not be a win.

> *That* wouldn't cause any data loss,
> because we'd never be able to checkpoint, and eventually the user
> would have to kill the server uncleanly, and that would trigger
> recovery.
>

Yep. That's what I expected to happen on unrecoverable I/O errors. Because,
y'know, unrecoverable.

I was stunned to learn it's not so. And I'm even more amazed to learn that
ext4's errors=remount-ro apparently doesn't concern its self with mere user
data, and may exhibit the same behaviour - I need to rerun my test case on
it tomorrow.

> Also, this really does make it impossible to write reliable programs.
>

In the presence of multiple apps interacting on the same file, yes. I think
that's a little bit of a stretch though.

For a single app, you can recover by remembering and redoing all the writes
you did.

Sucks if your app wants to have multiple processes working together on a
file without some kind of journal or WAL, relying on fsync() alone, mind
you. But at least we have WAL.

Hrm. I wonder how this interacts with wal_level=minimal.

> Even leaving that aside, a PANIC means a prolonged outage on a
> prolonged system - it could easily take tens of minutes or longer to
> run recovery. So saying "oh, just do that" is not really an answer.
> Sure, we can do it, but it's like trying to lose weight by
> intentionally eating a tapeworm. Now, it's possible to shorten the
> checkpoint_timeout so that recovery runs faster, but then performance
> drops because data has to be fsync()'d more often instead of getting
> buffered in the OS cache for the maximum possible time.

It's also spikier. Users have more issues with latency with short, frequent
checkpoints.

> We could also
> dodge this issue in another way: suppose that when we write a page
> out, we don't consider it really written until fsync() succeeds. Then
> we wouldn't need to PANIC if an fsync() fails; we could just re-write
> the page. Unfortunately, this would also be terrible for performance,
> for pretty much the same reasons: letting the OS cache absorb lots of
> dirty blocks and do write-combining is *necessary* for good
> performance.
>

Our double-caching is already plenty bad enough anyway, as well.

(Ideally I want to be able to swap buffers between shared_buffers and the
OS buffer-cache. Almost like a 2nd level of buffer pinning. When we write
out a block, we *transfer* ownership to the OS. Yeah, I'm dreaming. But
we'd sure need to be able to trust the OS not to just forget the block
then!)

> > The error reporting is thus consistent with the intended semantics (which
> > are sadly not properly documented). Repeated calls to fsync() simply do
> not
> > imply that the kernel will retry to writeback the previously-failed
> pages,
> > so the application needs to be aware of that. Persisting the error at the
> > fsync() level would essentially mean moving application policy into the
> > kernel.
>
> I might accept this argument if I accepted that it was OK to decide
> that an fsync() failure means you can forget that the write() ever
> happened in the first place, but it's hard to imagine an application
> that wants that behavior. If the application didn't care about
> whether the bytes really got to disk or not, it would not have called
> fsync() in the first place. If it does care, reporting the error only
> once is never an improvement.
>

Many RDBMSes do just that. It's hardly behaviour unique to the kernel. They
report an ERROR on a statement in a txn then go on with life, merrily
forgetting that anything was ever wrong.

I agree with PostgreSQL's stance that this is wrong. We require an explicit
rollback (or ROLLBACK TO SAVEPOINT) to restore the session to a usable
state. This is good.

But we're the odd one out there. Almost everyone else does much like what
fsync() does on Linux, report the error and forget it.

In any case, we're not going to get anyone to backpatch a fix for this into
all kernels, so we're stuck working around it.

I'll do some testing with ENOSPC tomorrow, propose a patch, report back.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 3 April 2018 at 14:36, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:

> If EIO persists between invocations until explicitly cleared, a process
> cannot possibly make any decision as to if it should clear the error

I still don't understand what "clear the error" means here. The writes
still haven't been written out. We don't care about tracking errors,
we just care whether all the writes to the file have been flushed to
disk. By "clear the error" you mean throw away the dirty pages and
revert part of the file to some old data? Why would anyone ever want
that?

> But instead of deconstructing and debating the semantics of the
> current mechanism, why not come up with the ideal desired form of
> error reporting/tracking granularity etc., and see how this may be
> fitted into kernels as a new interface.

Because Postgres is portable software that won't be able to use some
Linux-specific interface. And doesn't really need any granular error
reporting system anyways. It just needs to know when all writes have
been synced to disk.

--
greg

On Tue, Apr 03, 2018 at 03:37:30PM +0100, Greg Stark wrote:
> On 3 April 2018 at 14:36, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:
>
> > If EIO persists between invocations until explicitly cleared, a process
> > cannot possibly make any decision as to if it should clear the error
>
> I still don't understand what "clear the error" means here. The writes
> still haven't been written out. We don't care about tracking errors,
> we just care whether all the writes to the file have been flushed to
> disk. By "clear the error" you mean throw away the dirty pages and
> revert part of the file to some old data? Why would anyone ever want
> that?

It means that the responsibility of recovering the data is passed
back to the application. The writes may never be able to be written
out. How would a kernel deal with that? Either discard the data
(and have the writer acknowledge) or buffer the data until reboot
and simply risk going OOM. It's not what someone would want, but
rather *need* to deal with, one way or the other. At least on the
application-level there's a fighting chance for restoring to a
consistent state. The kernel does not have that opportunity.

> > But instead of deconstructing and debating the semantics of the
> > current mechanism, why not come up with the ideal desired form of
> > error reporting/tracking granularity etc., and see how this may be
> > fitted into kernels as a new interface.
>
> Because Postgres is portable software that won't be able to use some
> Linux-specific interface. And doesn't really need any granular error

I don't really follow this argument, Pg is admittedly using non-portable
interfaces (e.g the sync_file_range()). While it's nice to avoid platform
specific hacks, expecting that the POSIX semantics will be consistent
across systems is simply a 90's pipe dream. While it would be lovely
to have really consistent interfaces for application writers, this is
simply not going to happen any time soon.

And since those problematic semantics of fsync() appear to be prevalent
in other systems as well that are not likely to be changed, you cannot
rely on preconception that once buffers are handed over to kernel you
have a guarantee that they will be eventually persisted no matter what.
(Why even bother having fsync() in that case? The kernel would eventually
evict and writeback dirty pages anyway. The point of reporting the error
back to the application is to give it a chance to recover - the kernel
could repeat "fsync()" itself internally if this would solve anything).

> reporting system anyways. It just needs to know when all writes have
> been synced to disk.

Well, it does know when *some* writes have *not* been synced to disk,
exactly because the responsibility is passed back to the application.
I do realize this puts more burden back to the application, but what
would a viable alternative be? Would you rather have a kernel that
risks periodically going OOM due to this design decision?

Best regards,
Anthony

On Tue, Apr 3, 2018 at 6:35 AM, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:
>> Like other people here, I think this is 100% unreasonable, starting
>> with "the dirty pages which cannot been written out are practically
>> thrown away". Who decided that was OK, and on the basis of what
>> wording in what specification? I think it's always unreasonable to
>
> If you insist on strict conformance to POSIX, indeed the linux
> glibc configuration and associated manpage are probably wrong in
> stating that _POSIX_SYNCHRONIZED_IO is supported. The implementation
> matches that of the flexibility allowed by not supporting SIO.
> There's a long history of brokenness between linux and posix,
> and I think there was never an intention of conforming to the
> standard.

Well, then the man page probably shouldn't say CONFORMING TO 4.3BSD,
POSIX.1-2001, which on the first system I tested, it did. Also, the
summary should be changed from the current "fsync, fdatasync -
synchronize a file's in-core state with storage device" by adding ",
possibly by randomly undoing some of the changes you think you made to
the file".

> I believe (as tried to explain earlier) there is a certain assumption
> being made that the writer and original owner of data is responsible
> for dealing with potential errors in order to avoid data loss (which
> should be only of interest to the original writer anyway). It would
> be very questionable for the interface to persist the error while
> subsequent writes and fsyncs to different offsets may as well go through.

No, that's not questionable at all. fsync() doesn't take any argument
saying which part of the file you care about, so the kernel is
entirely not entitled to assume it knows to which writes a given
fsync() call was intended to apply.

> Another process may need to write into the file and fsync, while being
> unaware of those newly introduced semantics is now faced with EIO
> because some unrelated previous process failed some earlier writes
> and did not bother to clear the error for those writes. In a similar
> scenario where the second process is aware of the new semantics, it would
> naturally go ahead and clear the global error in order to proceed
> with its own write()+fsync(), which would essentially amount to the
> same problematic semantics you have now.

I don't deny that it's possible that somebody could have an
application which is utterly indifferent to the fact that earlier
modifications to a file failed due to I/O errors, but is A-OK with
that as long as later modifications can be flushed to disk, but I
don't think that's a normal thing to want.

>> Also, this really does make it impossible to write reliable programs.
>> Imagine that, while the server is running, somebody runs a program
>> which opens a file in the data directory, calls fsync() on it, and
>> closes it. If the fsync() fails, postgres is now borked and has no
>> way of being aware of the problem. If we knew, we could PANIC, but
>> we'll never find out, because the unrelated process ate the error.
>> This is exactly the sort of ill-considered behavior that makes fcntl()
>> locking nearly useless.
>
> Fully agree, and the errseq_t fixes have dealt exactly with the issue
> of making sure that the error is reported to all file descriptors that
> *happen to be open at the time of error*.

Well, in PostgreSQL, we have a background process called the
checkpointer which is the process that normally does all of the
fsync() calls but only a subset of the write() calls. The
checkpointer does not, however, necessarily have every file open all
the time, so these fixes aren't sufficient to make sure that the
checkpointer ever sees an fsync() failure. What you have (or someone
has) basically done here is made an undocumented assumption about
which file descriptors might care about a particular error, but it
just so happens that PostgreSQL has never conformed to that
assumption. You can keep on saying the problem is with our
assumptions, but it doesn't seem like a very good guess to me to
suppose that we're the only program that has ever made them. The
documentation for fsync() gives zero indication that it's
edge-triggered, and so complaining that people wouldn't like it if it
became level-triggered seems like an ex post facto justification for a
poorly-chosen behavior: they probably think (as we did prior to a week
ago) that it already is.

> Not sure I understand this case. The application may indeed re-write
> a bunch of pages that have failed and proceed with fsync(). The kernel
> will deal with combining the writeback of all the re-written pages. But
> further the necessity of combining for performance really depends on
> the exact storage medium. At the point you start caring about
> write-combining, the kernel community will naturally redirect you to
> use DIRECT_IO.

Well, the way PostgreSQL works today, we typically run with say 8GB of
shared_buffers even if the system memory is, say, 200GB. As pages are
evicted from our relatively small cache to the operating system, we
track which files need to be fsync()'d at checkpoint time, but we
don't hold onto the blocks. Until checkpoint time, the operating
system is left to decide whether it's better to keep caching the dirty
blocks (thus leaving less memory for other things, but possibly
allowing write-combining if the blocks are written again) or whether
it should clean them to make room for other things. This means that
only a small portion of the operating system memory is directly
managed by PostgreSQL, while allowing the effective size of our cache
to balloon to some very large number if the system isn't under heavy
memory pressure.

Now, I hear the DIRECT_IO thing and I assume we're eventually going to
have to go that way: Linux kernel developers seem to think that "real
men use O_DIRECT" and so if other forms of I/O don't provide useful
guarantees, well that's our fault for not using O_DIRECT. That's a
political reason, not a technical reason, but it's a reason all the
same.

Unfortunately, that is going to add a huge amount of complexity,
because if we ran with shared_buffers set to a large percentage of
system memory, we couldn't allocate large chunks of memory for sorts
and hash tables from the operating system any more. We'd have to
allocate it from our own shared_buffers because that's basically all
the memory there is and using substantially more might run the system
out entirely. So it's a huge, huge architectural change. And even
once it's done it is in some ways inferior to what we are doing today
-- true, it gives us superior control over writeback timing, but it
also makes PostgreSQL play less nicely with other things running on
the same machine, because now PostgreSQL has a dedicated chunk of
whatever size it has, rather than using some portion of the OS buffer
cache that can grow and shrink according to memory needs both of other
parts of PostgreSQL and other applications on the system.

> I suppose that if the check-and-clear semantics are problematic for
> Pg, one could suggest a kernel patch that opts-in to a level-triggered
> reporting of fsync() on a per-descriptor basis, which seems to be
> non-intrusive and probably sufficient to cover your expected use-case.

That would certainly be better than nothing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Apr 3, 2018 at 1:29 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> Interestingly, there don't seem to be many operating systems that can
> report ENOSPC from fsync(), based on a quick scan through some
> documentation:
>
> POSIX, AIX, HP-UX, FreeBSD, OpenBSD, NetBSD: no
> Illumos/Solaris, Linux, macOS: yes

Oops, reading comprehension fail. POSIX yes (since issue 5), via the
note that read() and write()'s error conditions can also be returned.

--
Thomas Munro
http://www.enterprisedb.com

On Tue, Apr 3, 2018 at 05:47:01PM -0400, Robert Haas wrote:
> Well, in PostgreSQL, we have a background process called the
> checkpointer which is the process that normally does all of the
> fsync() calls but only a subset of the write() calls. The
> checkpointer does not, however, necessarily have every file open all
> the time, so these fixes aren't sufficient to make sure that the
> checkpointer ever sees an fsync() failure.

There has been a lot of focus in this thread on the workflow:

write() -> blocks remain in kernel memory -> fsync() -> panic?

But what happens in this workflow:

write() -> kernel syncs blocks to storage -> fsync()

Is fsync() going to see a "kernel syncs blocks to storage" failure?

There was already discussion that if the fsync() causes the "syncs
blocks to storage", fsync() will only report the failure once, but will
it see any failure in the second workflow? There is indication that a
failed write to storage reports back an error once and clears the dirty
flag, but do we know it keeps things around long enough to report an
error to a future fsync()?

You would think it does, but I have to ask since our fsync() assumptions
have been wrong for so long.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On Wed, Apr 4, 2018 at 12:56 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> There has been a lot of focus in this thread on the workflow:
>
> write() -> blocks remain in kernel memory -> fsync() -> panic?
>
> But what happens in this workflow:
>
> write() -> kernel syncs blocks to storage -> fsync()
>
> Is fsync() going to see a "kernel syncs blocks to storage" failure?
>
> There was already discussion that if the fsync() causes the "syncs
> blocks to storage", fsync() will only report the failure once, but will
> it see any failure in the second workflow? There is indication that a
> failed write to storage reports back an error once and clears the dirty
> flag, but do we know it keeps things around long enough to report an
> error to a future fsync()?
>
> You would think it does, but I have to ask since our fsync() assumptions
> have been wrong for so long.

I believe there were some problems of that nature (with various
twists, based on other concurrent activity and possibly different
fds), and those problems were fixed by the errseq_t system developed
by Jeff Layton in Linux 4.13. Call that "bug #1".

The second issues is that the pages are marked clean after the error
is reported, so further attempts to fsync() the data (in our case for
a new attempt to checkpoint) will be futile but appear successful.
Call that "bug #2", with the proviso that some people apparently think
it's reasonable behaviour and not a bug. At least there is a
plausible workaround for that: namely the nuclear option proposed by
Craig.

--
Thomas Munro
http://www.enterprisedb.com

On Wed, Apr 4, 2018 at 01:54:50PM +1200, Thomas Munro wrote:
> On Wed, Apr 4, 2018 at 12:56 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > There has been a lot of focus in this thread on the workflow:
> >
> > write() -> blocks remain in kernel memory -> fsync() -> panic?
> >
> > But what happens in this workflow:
> >
> > write() -> kernel syncs blocks to storage -> fsync()
> >
> > Is fsync() going to see a "kernel syncs blocks to storage" failure?
> >
> > There was already discussion that if the fsync() causes the "syncs
> > blocks to storage", fsync() will only report the failure once, but will
> > it see any failure in the second workflow? There is indication that a
> > failed write to storage reports back an error once and clears the dirty
> > flag, but do we know it keeps things around long enough to report an
> > error to a future fsync()?
> >
> > You would think it does, but I have to ask since our fsync() assumptions
> > have been wrong for so long.
>
> I believe there were some problems of that nature (with various
> twists, based on other concurrent activity and possibly different
> fds), and those problems were fixed by the errseq_t system developed
> by Jeff Layton in Linux 4.13. Call that "bug #1".

So all our non-cutting-edge Linux systems are vulnerable and there is no
workaround Postgres can implement? Wow.

> The second issues is that the pages are marked clean after the error
> is reported, so further attempts to fsync() the data (in our case for
> a new attempt to checkpoint) will be futile but appear successful.
> Call that "bug #2", with the proviso that some people apparently think
> it's reasonable behaviour and not a bug. At least there is a
> plausible workaround for that: namely the nuclear option proposed by
> Craig.

Yes, that one I understood.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On Tue, Apr 3, 2018 at 10:05:19PM -0400, Bruce Momjian wrote:
> On Wed, Apr 4, 2018 at 01:54:50PM +1200, Thomas Munro wrote:
> > I believe there were some problems of that nature (with various
> > twists, based on other concurrent activity and possibly different
> > fds), and those problems were fixed by the errseq_t system developed
> > by Jeff Layton in Linux 4.13. Call that "bug #1".
>
> So all our non-cutting-edge Linux systems are vulnerable and there is no
> workaround Postgres can implement? Wow.

Uh, are you sure it fixes our use-case? From the email description it
sounded like it only reported fsync errors for every open file
descriptor at the time of the failure, but the checkpoint process might
open the file _after_ the failure and try to fsync a write that happened
_before_ the failure.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On 4 April 2018 at 05:47, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>
> Now, I hear the DIRECT_IO thing and I assume we're eventually going to
> have to go that way: Linux kernel developers seem to think that "real
> men use O_DIRECT" and so if other forms of I/O don't provide useful
> guarantees, well that's our fault for not using O_DIRECT. That's a
> political reason, not a technical reason, but it's a reason all the
> same.
>

I looked into buffered AIO a while ago, by the way, and just ... hell no.
Run, run as fast as you can.

The trouble with direct I/O is that it pushes a _lot_ of work back on
PostgreSQL regarding knowledge of the storage subsystem, I/O scheduling,
etc. It's absurd to have the kernel do this, unless you want it reliable,
in which case you bypass it and drive the hardware directly.

We'd need pools of writer threads to deal with all the blocking I/O. It'd
be such a nightmare. Hey, why bother having a kernel at all, except for
drivers?

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Apr 4, 2018 at 2:14 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> On Tue, Apr 3, 2018 at 10:05:19PM -0400, Bruce Momjian wrote:
>> On Wed, Apr 4, 2018 at 01:54:50PM +1200, Thomas Munro wrote:
>> > I believe there were some problems of that nature (with various
>> > twists, based on other concurrent activity and possibly different
>> > fds), and those problems were fixed by the errseq_t system developed
>> > by Jeff Layton in Linux 4.13. Call that "bug #1".
>>
>> So all our non-cutting-edge Linux systems are vulnerable and there is no
>> workaround Postgres can implement? Wow.
>
> Uh, are you sure it fixes our use-case? From the email description it
> sounded like it only reported fsync errors for every open file
> descriptor at the time of the failure, but the checkpoint process might
> open the file _after_ the failure and try to fsync a write that happened
> _before_ the failure.

I'm not sure of anything. I can see that it's designed to report
errors since the last fsync() of the *file* (presumably via any fd),
which sounds like the desired behaviour:

https://github.com/torvalds/linux/blob/master/mm/filemap.c#L682

* When userland calls fsync (or something like nfsd does the equivalent), we
* want to report any writeback errors that occurred since the last fsync (or
* since the file was opened if there haven't been any).

But I'm not sure what the lifetime of the passed-in "file" and more
importantly "file->f_wb_err" is. Specifically, what happens to it if
no one has the file open at all, between operations? It is reference
counted, see fs/file_table.c. I don't know enough about it to
comment.

--
Thomas Munro
http://www.enterprisedb.com

On Wed, Apr 4, 2018 at 2:44 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Wed, Apr 4, 2018 at 2:14 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> Uh, are you sure it fixes our use-case? From the email description it
>> sounded like it only reported fsync errors for every open file
>> descriptor at the time of the failure, but the checkpoint process might
>> open the file _after_ the failure and try to fsync a write that happened
>> _before_ the failure.
>
> I'm not sure of anything. I can see that it's designed to report
> errors since the last fsync() of the *file* (presumably via any fd),
> which sounds like the desired behaviour:
>
> [..]

Scratch that. Whenever you open a file descriptor you can't see any
preceding errors at all, because:

/* Ensure that we skip any errors that predate opening of the file */
f->f_wb_err = filemap_sample_wb_err(f->f_mapping);

https://github.com/torvalds/linux/blob/master/fs/open.c#L752

Our whole design is based on being able to open, close and reopen
files at will from any process, and in particular to fsync() from a
different process that didn't inherit the fd but instead opened it
later. But it looks like that might be able to eat errors that
occurred during asynchronous writeback (when there was nobody to
report them to), before you opened the file?

If so I'm not sure how that can possibly be considered to be an
implementation of _POSIX_SYNCHRONIZED_IO: "the fsync() function shall
force all currently queued I/O operations associated with the file
indicated by file descriptor fildes to the synchronized I/O completion
state." Note "the file", not "this file descriptor + copies", and
without reference to when you opened it.

> But I'm not sure what the lifetime of the passed-in "file" and more
> importantly "file->f_wb_err" is.

It's really inode->i_mapping->wb_err's lifetime that I should have
been asking about there, not file->f_wb_err, but I see now that that
question is irrelevant due to the above.

--
Thomas Munro
http://www.enterprisedb.com

On 4 April 2018 at 13:29, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
wrote:

> On Wed, Apr 4, 2018 at 2:44 PM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> > On Wed, Apr 4, 2018 at 2:14 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> Uh, are you sure it fixes our use-case? From the email description it
> >> sounded like it only reported fsync errors for every open file
> >> descriptor at the time of the failure, but the checkpoint process might
> >> open the file _after_ the failure and try to fsync a write that happened
> >> _before_ the failure.
> >
> > I'm not sure of anything. I can see that it's designed to report
> > errors since the last fsync() of the *file* (presumably via any fd),
> > which sounds like the desired behaviour:
> >
> > [..]
>
> Scratch that. Whenever you open a file descriptor you can't see any
> preceding errors at all, because:
>
> /* Ensure that we skip any errors that predate opening of the file */
> f->f_wb_err = filemap_sample_wb_err(f->f_mapping);
>
> https://github.com/torvalds/linux/blob/master/fs/open.c#L752
>
> Our whole design is based on being able to open, close and reopen
> files at will from any process, and in particular to fsync() from a
> different process that didn't inherit the fd but instead opened it
> later. But it looks like that might be able to eat errors that
> occurred during asynchronous writeback (when there was nobody to
> report them to), before you opened the file?
>

Holy hell. So even PANICing on fsync() isn't sufficient, because the kernel
will deliberately hide writeback errors that predate our fsync() call from
us?

I'll see if I can expand my testcase for that. I'm presently dockerizing it
to make it easier for others to use, but that turns out to be a major pain
when using devmapper etc. Docker in privileged mode doesn't seem to play
nice with device-mapper.

Does that mean that the ONLY ways to do reliable I/O are:

- single-process, single-file-descriptor write() then fsync(); on failure,
retry all work since last successful fsync()

or

- direct I/O

?

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Apr 4, 2018 at 6:00 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 4 April 2018 at 13:29, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
> wrote:
>> /* Ensure that we skip any errors that predate opening of the file */
>> f->f_wb_err = filemap_sample_wb_err(f->f_mapping);
>>
>> [...]
>
> Holy hell. So even PANICing on fsync() isn't sufficient, because the kernel
> will deliberately hide writeback errors that predate our fsync() call from
> us?

Predates the opening of the file by the process that calls fsync().
Yeah, it sure looks that way based on the above code fragment. Does
anyone know better?

> Does that mean that the ONLY ways to do reliable I/O are:
>
> - single-process, single-file-descriptor write() then fsync(); on failure,
> retry all work since last successful fsync()

I suppose you could some up with some crazy complicated IPC scheme to
make sure that the checkpointer always has an fd older than any writes
to be flushed, with some fallback strategy for when it can't take any
more fds.

I haven't got any good ideas right now.

> - direct I/O

As a bit of an aside, I gather that when you resize files (think
truncating/extending relation files) you still need to call fsync()
even if you read/write all data with O_DIRECT, to make it flush the
filesystem meta-data. I have no idea if that could also be affected
by eaten writeback errors.

--
Thomas Munro
http://www.enterprisedb.com

On 4 April 2018 at 14:00, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> On 4 April 2018 at 13:29, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
> wrote:
>
>> On Wed, Apr 4, 2018 at 2:44 PM, Thomas Munro
>> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>> > On Wed, Apr 4, 2018 at 2:14 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> >> Uh, are you sure it fixes our use-case? From the email description it
>> >> sounded like it only reported fsync errors for every open file
>> >> descriptor at the time of the failure, but the checkpoint process might
>> >> open the file _after_ the failure and try to fsync a write that
>> happened
>> >> _before_ the failure.
>> >
>> > I'm not sure of anything. I can see that it's designed to report
>> > errors since the last fsync() of the *file* (presumably via any fd),
>> > which sounds like the desired behaviour:
>> >
>> > [..]
>>
>> Scratch that. Whenever you open a file descriptor you can't see any
>> preceding errors at all, because:
>>
>> /* Ensure that we skip any errors that predate opening of the file */
>> f->f_wb_err = filemap_sample_wb_err(f->f_mapping);
>>
>> https://github.com/torvalds/linux/blob/master/fs/open.c#L752
>>
>> Our whole design is based on being able to open, close and reopen
>> files at will from any process, and in particular to fsync() from a
>> different process that didn't inherit the fd but instead opened it
>> later. But it looks like that might be able to eat errors that
>> occurred during asynchronous writeback (when there was nobody to
>> report them to), before you opened the file?
>>
>
> Holy hell. So even PANICing on fsync() isn't sufficient, because the
> kernel will deliberately hide writeback errors that predate our fsync()
> call from us?
>
> I'll see if I can expand my testcase for that. I'm presently dockerizing
> it to make it easier for others to use, but that turns out to be a major
> pain when using devmapper etc. Docker in privileged mode doesn't seem to
> play nice with device-mapper.
>
>
Done, you can find it in
https://github.com/ringerc/scrapcode/tree/master/testcases/fsync-error-clear
now.

Warning, this runs a Docker container in privileged mode on your system,
and it uses devicemapper. Read it before you run it, and while I've tried
to keep it safe, beware that it might eat your system.

For now it tests only xfs and EIO. Other FSs should be easy enough.

I haven't added coverage for multi-processing yet, but given what you found
above, I should. I'll probably just system() a copy of the same proc with
instructions to only fsync(). I'll do that next.

I haven't worked out a reliable way to trigger ENOSPC on fsync() yet, when
mapping without the error hole. It happens sometimes but I don't know why,
it almost always happens on write() instead. I know it can happen on nfs,
but I'm hoping for a saner example than that to test with. ext4 and xfs do
delayed allocation but eager reservation so it shouldn't happen to them.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Apr 4, 2018 at 07:32:04PM +1200, Thomas Munro wrote:
> On Wed, Apr 4, 2018 at 6:00 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> > On 4 April 2018 at 13:29, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
> > wrote:
> >> /* Ensure that we skip any errors that predate opening of the file */
> >> f->f_wb_err = filemap_sample_wb_err(f->f_mapping);
> >>
> >> [...]
> >
> > Holy hell. So even PANICing on fsync() isn't sufficient, because the kernel
> > will deliberately hide writeback errors that predate our fsync() call from
> > us?
>
> Predates the opening of the file by the process that calls fsync().
> Yeah, it sure looks that way based on the above code fragment. Does
> anyone know better?

Uh, just to clarify, what is new here is that it is ignoring any
_errors_ that happened before the open(). It is not ignoring write()'s
that happened but have not been written to storage before the open().

FYI, pg_test_fsync has always tested the ability to fsync() writes()
from from other processes:

Test if fsync on non-write file descriptor is honored:
(If the times are similar, fsync() can sync data written on a different
descriptor.)
write, fsync, close 5360.341 ops/sec 187 usecs/op
write, close, fsync 4785.240 ops/sec 209 usecs/op

Those two numbers should be similar. I added this as a check to make
sure the behavior we were relying on was working. I never tested sync
errors though.

I think the fundamental issue is that we always assumed that writes to
the kernel that could not be written to storage would remain in the
kernel until they succeeded, and that fsync() would report their
existence.

I can understand why kernel developers don't want to keep failed sync
buffers in memory, and once they are gone we lose reporting of their
failure. Also, if the kernel is going to not retry the syncs, how long
should it keep reporting the sync failure? To the first fsync that
happens after the failure? How long should it continue to record the
failure? What if no fsync() every happens, which is likely for
non-Postgres workloads? I think once they decided to discard failed
syncs and not retry them, the fsync behavior we are complaining about
was almost required.

Our only option might be to tell administrators to closely watch for
kernel write failure messages, and then restore or failover. :-(

The last time I remember being this surprised about storage was in the
early Postgres years when we learned that just because the BSD file
system uses 8k pages doesn't mean those are atomically written to
storage. We knew the operating system wrote the data in 8k chunks to
storage but:

o the 8k pages are written as separate 512-byte sectors
o the 8k might be contiguous logically on the drive but not physically
o even 512-byte sectors are not written atomically

This is why we added pre-page images are written to WAL, which is what
full_page_writes controls.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On Wed, Apr 4, 2018 at 10:40:16AM +0800, Craig Ringer wrote:
> The trouble with direct I/O is that it pushes a _lot_ of work back on
> PostgreSQL regarding knowledge of the storage subsystem, I/O scheduling, etc.
> It's absurd to have the kernel do this, unless you want it reliable, in which
> case you bypass it and drive the hardware directly.
>
> We'd need pools of writer threads to deal with all the blocking I/O. It'd be
> such a nightmare. Hey, why bother having a kernel at all, except for drivers?

I believe this is how Oracle views the kernel, so there is precedent for
this approach, though I am not advocating it.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On 4 April 2018 at 15:51, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> On 4 April 2018 at 14:00, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>
>> On 4 April 2018 at 13:29, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
>> wrote:
>>
>>> On Wed, Apr 4, 2018 at 2:44 PM, Thomas Munro
>>> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>> > On Wed, Apr 4, 2018 at 2:14 PM, Bruce Momjian <bruce(at)momjian(dot)us>
>>> wrote:
>>> >> Uh, are you sure it fixes our use-case? From the email description it
>>> >> sounded like it only reported fsync errors for every open file
>>> >> descriptor at the time of the failure, but the checkpoint process
>>> might
>>> >> open the file _after_ the failure and try to fsync a write that
>>> happened
>>> >> _before_ the failure.
>>> >
>>> > I'm not sure of anything. I can see that it's designed to report
>>> > errors since the last fsync() of the *file* (presumably via any fd),
>>> > which sounds like the desired behaviour:
>>> >
>>> > [..]
>>>
>>> Scratch that. Whenever you open a file descriptor you can't see any
>>> preceding errors at all, because:
>>>
>>> /* Ensure that we skip any errors that predate opening of the file */
>>> f->f_wb_err = filemap_sample_wb_err(f->f_mapping);
>>>
>>> https://github.com/torvalds/linux/blob/master/fs/open.c#L752
>>>
>>> Our whole design is based on being able to open, close and reopen
>>> files at will from any process, and in particular to fsync() from a
>>> different process that didn't inherit the fd but instead opened it
>>> later. But it looks like that might be able to eat errors that
>>> occurred during asynchronous writeback (when there was nobody to
>>> report them to), before you opened the file?
>>>
>>
>> Holy hell. So even PANICing on fsync() isn't sufficient, because the
>> kernel will deliberately hide writeback errors that predate our fsync()
>> call from us?
>>
>> I'll see if I can expand my testcase for that. I'm presently dockerizing
>> it to make it easier for others to use, but that turns out to be a major
>> pain when using devmapper etc. Docker in privileged mode doesn't seem to
>> play nice with device-mapper.
>>
>>
> Done, you can find it in https://github.com/ringerc/scrapcode/tree/master/
> testcases/fsync-error-clear now.
>
>
Update. Now supports multiple FSes.

I've tried xfs, jfs, ext3, ext4, even vfat. All behave the same on EIO.
Didn't try zfs-on-linux or other platforms yet.

Still working on getting ENOSPC on fsync() rather than write(). Kernel code
reading suggests this is possible, but all the above FSes reserve space
eagerly on write( ) even if they do delayed allocation of the actual
storage, so it doesn't seem to happen at least in my simple single-process
test.

I'm not overly inclined to complain about a fsync() succeeding after a
write() error. That seems reasonable enough, the kernel told the app at the
time of the failure. What else is it going to do? I don't personally even
object hugely to the current fsync() behaviour if it were, say, DOCUMENTED
and conformant to the relevant standards, though not giving us any sane way
to find out the affected file ranges makes it drastically harder to recover
sensibly.

But what's come out since on this thread, that we cannot even rely on
fsync() giving us an EIO *once* when it loses our data, because:

- all currently widely deployed kernels can fail to deliver info due to
recently fixed limitation; and
- the kernel deliberately hides errors from us if they relate to writes
that occurred before we opened the FD (?)

... that's really troubling. I thought we could at least fix this by
PANICing on EIO, and was mostly worried about ENOSPC. But now it seems we
can't even do that and expect reliability. So how the @#$ are we meant to
do?

It's the error reporting issues around closing and reopening files with
outstanding buffered I/O that's really going to hurt us here. I'll be
expanding my test case to cover that shortly.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 4 April 2018 at 22:00, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> It's the error reporting issues around closing and reopening files with
> outstanding buffered I/O that's really going to hurt us here. I'll be
> expanding my test case to cover that shortly.
>
>
Also, just to be clear, this is not in any way confined to xfs and/or lvm
as I originally thought it might be.

Nor is ext3/ext4's errors=remount-ro protective. data_err=abort doesn't
help either (so what does it do?).

What bewilders me is that running with data=journal doesn't seem to be safe
either. WTF?

[26438.846111] EXT4-fs (dm-0): mounted filesystem with journalled data
mode. Opts: errors=remount-ro,data_err=abort,data=journal
[26454.125319] EXT4-fs warning (device dm-0): ext4_end_bio:323: I/O error
10 writing to inode 12 (offset 0 size 0 starting block 59393)
[26454.125326] Buffer I/O error on device dm-0, logical block 59393
[26454.125337] Buffer I/O error on device dm-0, logical block 59394
[26454.125343] Buffer I/O error on device dm-0, logical block 59395
[26454.125350] Buffer I/O error on device dm-0, logical block 59396

and splat, there goes your data anyway.

It's possible that this is in some way related to using the device-mapper
"error" target and a loopback device in testing. But I don't really see how.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Apr 4, 2018 at 10:09:09PM +0800, Craig Ringer wrote:
> On 4 April 2018 at 22:00, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>  
>
> It's the error reporting issues around closing and reopening files with
> outstanding buffered I/O that's really going to hurt us here. I'll be
> expanding my test case to cover that shortly.
>
>
>
> Also, just to be clear, this is not in any way confined to xfs and/or lvm as I
> originally thought it might be. 
>
> Nor is ext3/ext4's errors=remount-ro protective. data_err=abort doesn't help
> either (so what does it do?).

Anthony Iliopoulos reported in this thread that errors=remount-ro is
only affected by metadata writes.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On 4 April 2018 at 22:25, Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> On Wed, Apr 4, 2018 at 10:09:09PM +0800, Craig Ringer wrote:
> > On 4 April 2018 at 22:00, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> >
> >
> > It's the error reporting issues around closing and reopening files
> with
> > outstanding buffered I/O that's really going to hurt us here. I'll be
> > expanding my test case to cover that shortly.
> >
> >
> >
> > Also, just to be clear, this is not in any way confined to xfs and/or
> lvm as I
> > originally thought it might be.
> >
> > Nor is ext3/ext4's errors=remount-ro protective. data_err=abort doesn't
> help
> > either (so what does it do?).
>
> Anthony Iliopoulos reported in this thread that errors=remount-ro is
> only affected by metadata writes.

Yep, I gathered. I was referring to data_err.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Apr 4, 2018 at 4:42 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>
> On 4 April 2018 at 22:25, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>>
>> On Wed, Apr 4, 2018 at 10:09:09PM +0800, Craig Ringer wrote:
>> > On 4 April 2018 at 22:00, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> >
>> >
>> > It's the error reporting issues around closing and reopening files
with
>> > outstanding buffered I/O that's really going to hurt us here. I'll
be
>> > expanding my test case to cover that shortly.
>> >
>> >
>> >
>> > Also, just to be clear, this is not in any way confined to xfs and/or
lvm as I
>> > originally thought it might be.
>> >
>> > Nor is ext3/ext4's errors=remount-ro protective. data_err=abort
doesn't help
>> > either (so what does it do?).
>>
>> Anthony Iliopoulos reported in this thread that errors=remount-ro is
>> only affected by metadata writes.
>
>
> Yep, I gathered. I was referring to data_err.

As far as I recall data_err=abort pertains to the jbd2 handling of
potential writeback errors. Jbd2 will inetrnally attempt to drain
the data upon txn commit (and it's even kind enough to restore
the EIO at the address space level, that otherwise would get eaten).

When data_err=abort is set, then jbd2 forcibly shuts down the
entire journal, with the error being propagated upwards to ext4.
I am not sure at which point this would be manifested to userspace
and how, but in principle any subsequent fs operations would get
some filesystem error due to the journal being down (I would
assume similar to remounting the fs read-only).

Since you are using data=journal, I would indeed expect to see
something more than what you saw in dmesg.

I can have a look later, I plan to also respond to some of the other
interesting issues that you guys raised in the thread.

Best regards,
Anthony

On 4 April 2018 at 21:49, Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> On Wed, Apr 4, 2018 at 07:32:04PM +1200, Thomas Munro wrote:
> > On Wed, Apr 4, 2018 at 6:00 PM, Craig Ringer <craig(at)2ndquadrant(dot)com>
> wrote:
> > > On 4 April 2018 at 13:29, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
> > > wrote:
> > >> /* Ensure that we skip any errors that predate opening of the file */
> > >> f->f_wb_err = filemap_sample_wb_err(f->f_mapping);
> > >>
> > >> [...]
> > >
> > > Holy hell. So even PANICing on fsync() isn't sufficient, because the
> kernel
> > > will deliberately hide writeback errors that predate our fsync() call
> from
> > > us?
> >
> > Predates the opening of the file by the process that calls fsync().
> > Yeah, it sure looks that way based on the above code fragment. Does
> > anyone know better?
>
> Uh, just to clarify, what is new here is that it is ignoring any
> _errors_ that happened before the open(). It is not ignoring write()'s
> that happened but have not been written to storage before the open().
>
> FYI, pg_test_fsync has always tested the ability to fsync() writes()
> from from other processes:
>
> Test if fsync on non-write file descriptor is honored:
> (If the times are similar, fsync() can sync data written on a
> different
> descriptor.)
> write, fsync, close 5360.341 ops/sec
> 187 usecs/op
> write, close, fsync 4785.240 ops/sec
> 209 usecs/op
>
> Those two numbers should be similar. I added this as a check to make
> sure the behavior we were relying on was working. I never tested sync
> errors though.
>
> I think the fundamental issue is that we always assumed that writes to
> the kernel that could not be written to storage would remain in the
> kernel until they succeeded, and that fsync() would report their
> existence.
>
> I can understand why kernel developers don't want to keep failed sync
> buffers in memory, and once they are gone we lose reporting of their
> failure. Also, if the kernel is going to not retry the syncs, how long
> should it keep reporting the sync failure?

Ideally until the app tells it not to.

But there's no standard API for that.

The obvious answer seems to be "until the FD is closed". But we just
discussed how Pg relies on being able to open and close files freely. That
may not be as reasonable a thing to do as we thought it was when you
consider error reporting. What's the kernel meant to do? How long should it
remember "I had an error while doing writeback on this file"? Should it
flag the file metadata and remember across reboots? Obviously not, but
where does it stop? Tell the next program that does an fsync() and forget?
How could it associate a dirty buffer on a file with no open FDs with any
particular program at all? And what if the app did a write then closed the
file and went away, never to bother to check the file again, like most apps
do?

Some I/O errors are transient (network issue, etc). Some are recoverable
with some sort of action, like disk space issues, but may take a long time
before an admin steps in. Some are entirely unrecoverable (disk 1 in
striped array is on fire) and there's no possible recovery. Currently we
kind of hope the kernel will deal with figuring out which is which and
retrying. Turns out it doesn't do that so much, and I don't think the
reasons for that are wholly unreasonable. We may have been asking too much.

That does leave us in a pickle when it comes to the checkpointer and
opening/closing FDs. I don't know what the "right" thing for the kernel to
do from our perspective even is here, but the best I can come up with is
actually pretty close to what it does now. Report the fsync() error to the
first process that does an fsync() since the writeback error if one has
occurred, then forget about it. Ideally I'd have liked it to mark all FDs
pointing to the file with a flag to report EIO on next fsync too, but it
turns out that won't even help us due to our opening and closing behaviour,
so we're going to have to take responsibility for handling and
communicating that ourselves, preventing checkpoint completion if any
backend gets an fsync error. Probably by PANICing. Some extra work may be
needed to ensure reliable ordering and stop checkpoints completing if their
fsync() succeeds due to a recent failed fsync() on a normal backend that
hasn't PANICed or where the postmaster hasn't noticed yet.

Our only option might be to tell administrators to closely watch for
> kernel write failure messages, and then restore or failover. :-(
>

Speaking of, there's not necessarily any lost page write error in the logs
AFAICS. My tests often just show "Buffer I/O error on device dm-0, logical
block 59393" or the like.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 04. 04. 2018 15:49, Bruce Momjian wrote:
> I can understand why kernel developers don't want to keep failed sync
> buffers in memory, and once they are gone we lose reporting of their
> failure. Also, if the kernel is going to not retry the syncs, how long
> should it keep reporting the sync failure? To the first fsync that
> happens after the failure? How long should it continue to record the
> failure? What if no fsync() every happens, which is likely for
> non-Postgres workloads? I think once they decided to discard failed
> syncs and not retry them, the fsync behavior we are complaining about
> was almost required.
Ideally the kernel would keep its data for as little time as possible.
With fsync, it doesn't really know which process is interested in
knowing about a write error, it just assumes the caller will know how to
deal with it. Most unfortunate issue is there's no way to get
information about a write error.

Thinking aloud - couldn't/shouldn't a write error also be a file system
event reported by inotify? Admittedly that's only a thing on Linux, but
still.

Kind regards,
Gasper

On Wed, Apr 4, 2018 at 11:23:51PM +0800, Craig Ringer wrote:
> On 4 April 2018 at 21:49, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> I can understand why kernel developers don't want to keep failed sync
> buffers in memory, and once they are gone we lose reporting of their
> failure.  Also, if the kernel is going to not retry the syncs, how long
> should it keep reporting the sync failure?
>
> Ideally until the app tells it not to.
>
> But there's no standard API for that.

You would almost need an API that registers _before_ the failure that
you care about sync failures, and that you plan to call fsync() to
gather such information. I am not sure how you would allow more than
the first fsync() to see the failure unless you added _another_ API to
clear the fsync failure, but I don't see the point since the first
fsync() might call that clear function. How many applications are going
to know there is _another_ application that cares about the failure? Not
many.

> Currently we kind of hope the kernel will deal with figuring out which
> is which and retrying. Turns out it doesn't do that so much, and I
> don't think the reasons for that are wholly unreasonable. We may have
> been asking too much.

Agreed.

> Our only option might be to tell administrators to closely watch for
> kernel write failure messages, and then restore or failover.  :-(
>
> Speaking of, there's not necessarily any lost page write error in the logs
> AFAICS. My tests often just show "Buffer I/O error on device dm-0, logical
> block 59393" or the like.

I assume that is the kernel logs. I am thinking the kernel logs have to
be monitored, but how many administrators do that? The other issue I
think you are pointing out is how is the administrator going to know
this is a Postgres file? I guess any sync error to a device that
contains Postgres has to assume Postgres is corrupted. :-(

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On Thu, Apr 5, 2018 at 2:00 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> I've tried xfs, jfs, ext3, ext4, even vfat. All behave the same on EIO.
> Didn't try zfs-on-linux or other platforms yet.

I think ZFS will be an outlier here, at least in a pure
write()/fsync() test. (1) It doesn't even use the OS page cache,
except when you mmap()*. (2) Its idea of syncing data is to journal
it, and its journal presumably isn't in the OS page cache. In other
words it doesn't use Linux's usual write-back code paths.

While contemplating what exactly it would do (not sure), I came across
an interesting old thread on the freebsd-current mailing list that
discusses UFS, ZFS and the meaning of POSIX fsync(). Here we see a
report of FreeBSD + UFS doing exactly what the code suggests:

https://lists.freebsd.org/pipermail/freebsd-current/2007-August/076578.html

That is, it keeps the pages dirty so it tells the truth later.
Apparently like Solaris/Illumos (based on drive-by code inspection,
see explicit treatment of retrying, though I'm not entirely sure if
the retry flag is set just for async write-back), and apparently
unlike every other kernel I've tried to grok so far (things descended
from ancestral BSD but not descended from FreeBSD, with macOS/Darwin
apparently in the first category for this purpose).

Here's a new ticket in the NetBSD bug database for this stuff:

http://gnats.netbsd.org/53152

As mentioned in that ticket and by Andres earlier in this thread,
keeping the page dirty isn't the only strategy that would work and may
be problematic in different ways (it tells the truth but floods your
cache with unflushable stuff until eventually you force unmount it and
your buffers are eventually invalidated after ENXIO errors? I don't
know.). I have no qualified opinion on that. I just know that we
need a way for fsync() to tell the truth about all preceding writes or
our checkpoints are busted.

*We mmap() + msync() in pg_flush_data() if you don't have
sync_file_range(), and I see now that that is probably not a great
idea on ZFS because you'll finish up double-buffering (or is that
triple-buffering?), flooding your page cache with transient data.
Oops. That is off-topic and not relevant for the checkpoint
correctness topic of this thread through, since pg_flush_data() is
advisory only.

--
Thomas Munro
http://www.enterprisedb.com

On Thu, Apr 5, 2018 at 9:28 AM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Thu, Apr 5, 2018 at 2:00 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> I've tried xfs, jfs, ext3, ext4, even vfat. All behave the same on EIO.
>> Didn't try zfs-on-linux or other platforms yet.
>
> While contemplating what exactly it would do (not sure),

See manual for failmode=wait | continue | panic. Even "continue"
returns EIO to all new write requests, so they apparently didn't
bother to supply an 'eat-my-data-but-tell-me-everything-is-fine' mode.
Figures.

--
Thomas Munro
http://www.enterprisedb.com

Summary to date:

It's worse than I thought originally, because:

- Most widely deployed kernels have cases where they don't tell you about
losing your writes at all; and
- Information about loss of writes can be masked by closing and re-opening
a file

So the checkpointer cannot trust that a successful fsync() means ... a
successful fsync().

Also, it's been reported to me off-list that anyone on the system calling
sync(2) or the sync shell command will also generally consume the write
error, causing us not to see it when we fsync(). The same is true
for /proc/sys/vm/drop_caches. I have not tested these yet.

There's some level of agreement that we should PANIC on fsync() errors, at
least on Linux, but likely everywhere. But we also now know it's
insufficient to be fully protective.

I previously though that errors=remount-ro was a sufficient safeguard. It
isn't. There doesn't seem to be anything that is, for ext3, ext4, btrfs or
xfs.

It's not clear to me yet why data_err=abort isn't sufficient in
data=ordered or data=writeback mode on ext3 or ext4, needs more digging.
(In my test tools that's:
make FSTYPE=ext4 MKFSOPTS="" MOUNTOPTS="errors=remount-ro,
data_err=abort,data=journal"
as of the current version d7fe802ec). AFAICS that's because
data_error=abort only affects data=ordered, not data=journal. If you use
data=ordered, you at least get retries of the same write failing. This post
https://lkml.org/lkml/2008/10/10/80 added the option and has some
explanation, but doesn't explain why it doesn't affect data=journal.

zfs is probably not affected by the issues, per Thomas Munro. I haven't run
my test scripts on it yet because my kernel doesn't have zfs support and
I'm prioritising the multi-process / open-and-close issues.

So far none of the FSes and options I've tried exhibit the behavour I
actually want, which is to make the fs readonly or inaccessible on I/O
error.

ENOSPC doesn't seem to be a concern during normal operation of major file
systems (ext3, ext4, btrfs, xfs) because they reserve space before
returning from write(). But if a buffered write does manage to fail due to
ENOSPC we'll definitely see the same problems. This makes ENOSPC on NFS a
potentially data corrupting condition since NFS doesn't preallocate space
before returning from write().

I think what we really need is a block-layer fix, where an I/O error flips
the block device into read-only mode, as if blockdev --setro had
been used. Though I'd settle for a kernel panic, frankly. I don't think
anybody really wants this, but I'd rather either of those to silent data
loss.

I'm currently tweaking my test to do some close and reopen the file between
each write() and fsync(), and to support running with nfs.

I've also just found the device-mapper "flakey" driver, which looks
fantastic for simulating unreliable I/O with intermittent faults. I've been
using the "error" target in a mapping, which lets me remap some of the
device to always error, but "flakey" looks very handy for actual PostgreSQL
testing.

For the sake of Google, these are errors known to be associated with the
problem:

ext4, and ext3 mounted with ext4 driver:

[42084.327345] EXT4-fs warning (device dm-0): ext4_end_bio:323: I/O error
10 writing to inode 12 (offset 0 size 0 starting block 59393)
[42084.327352] Buffer I/O error on device dm-0, logical block 59393

xfs:

[42193.771367] XFS (dm-0): writeback error on sector 118784
[42193.784477] XFS (dm-0): writeback error on sector 118784

jfs: (nil, silence in the kernel logs)

You should also beware of "lost page write" or "lost write" errors.

On 5 April 2018 at 15:09, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> Also, it's been reported to me off-list that anyone on the system calling
> sync(2) or the sync shell command will also generally consume the write
> error, causing us not to see it when we fsync(). The same is true
> for /proc/sys/vm/drop_caches. I have not tested these yet.
>

I just confirmed this with a tweak to the test that

records the file position
close()s the fd
sync()s
open(s) the file
lseek()s back to the recorded position

This causes the test to completely ignore the I/O error, which is not
reported to it at any time.

Fair enough, really, when you look at it from the kernel's point of view.
What else can it do? Nobody has the file open. It'd have to mark the file
its self as bad somehow. But that's pretty bad for our robustness AFAICS.

> There's some level of agreement that we should PANIC on fsync() errors, at
> least on Linux, but likely everywhere. But we also now know it's
> insufficient to be fully protective.
>

If dirty writeback fails between our close() and re-open() I see the same
behaviour as with sync(). To test that I set dirty_writeback_centisecs
and dirty_expire_centisecs to 1 and added a usleep(3*100*1000) between
close() and open(). (It's still plenty slow). So sync() is a convenient way
to simulate something other than our own fsync() writing out the dirty
buffer.

If I omit the sync() then we get the error reported by fsync() once when we
re open() the file and fsync() it, because the buffers weren't written out
yet, so the error wasn't generated until we re-open()ed the file. But I
doubt that'll happen much in practice because dirty writeback will get to
it first so the error will be seen and discarded before we reopen the file
in the checkpointer.

In other words, it looks like *even with a new kernel with the error
reporting bug fixes*, if I understand how the backends and checkpointer
interact when it comes to file descriptors, we're unlikely to notice I/O
errors and fail a checkpoint. We may notice I/O errors if a backend does
its own eager writeback for large I/O operations, or if the checkpointer
fsync()s a file before the kernel's dirty writeback gets around to trying
to flush the pages that will fail.

I haven't tested anything with multiple processes / multiple FDs yet, where
we keep one fd open while writing on another.

But at this point I don't see any way to make Pg reliably detect I/O errors
and fail a checkpoint then redo and retry. To even fix this by PANICing
like I proposed originally, we need to know we have to PANIC.

AFAICS it's completely unsafe to write(), close(), open() and fsync() and
expect that the fsync() makes any promises about the write(). Which if I
read Pg's low level storage code right, makes it completely unable to
reliably detect I/O errors.

When put it that way, it sounds fair enough too. How long is the kernel
meant to remember that there was a write error on the file triggered by a
write initiated by some seemingly unrelated process, some unbounded time
ago, on a since-closed file?

But it seems to put Pg on the fast track to O_DIRECT.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Apr 5, 2018 at 03:09:57PM +0800, Craig Ringer wrote:
> ENOSPC doesn't seem to be a concern during normal operation of major file
> systems (ext3, ext4, btrfs, xfs) because they reserve space before returning
> from write(). But if a buffered write does manage to fail due to ENOSPC we'll
> definitely see the same problems. This makes ENOSPC on NFS a potentially data
> corrupting condition since NFS doesn't preallocate space before returning from
> write().

This does explain why NFS has a reputation for unreliability for
Postgres.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

Note: as I've brought up in another thread, it turns out that PG is not
handling fsync errors correctly even when the OS _does_ do the right
thing (discovered by testing on FreeBSD).

--
Andrew (irc:RhodiumToad)

On 6 April 2018 at 07:37, Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:

> Note: as I've brought up in another thread, it turns out that PG is not
> handling fsync errors correctly even when the OS _does_ do the right
> thing (discovered by testing on FreeBSD).

Yikes. For other readers, the related thread for this is
https://www.postgresql.org/message-id/87y3i1ia4w.fsf@news-spur.riddles.org.uk

Meanwhile, I've extended my test to run postgres on a deliberately faulty
volume and confirmed my results there.

_____________

2018-04-06 01:11:40.555 UTC [58] LOG: checkpoint starting: immediate force
wait
2018-04-06 01:11:40.567 UTC [58] ERROR: could not fsync file
"base/12992/16386": Input/output error
2018-04-06 01:11:40.655 UTC [66] ERROR: checkpoint request failed
2018-04-06 01:11:40.655 UTC [66] HINT: Consult recent messages in the
server log for details.
2018-04-06 01:11:40.655 UTC [66] STATEMENT: CHECKPOINT

Checkpoint failed with checkpoint request failed
HINT: Consult recent messages in the server log for details.

Retrying

2018-04-06 01:11:41.568 UTC [58] LOG: checkpoint starting: immediate force
wait
2018-04-06 01:11:41.614 UTC [58] LOG: checkpoint complete: wrote 0 buffers
(0.0%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.001 s,
sync=0.000 s, total=0.046 s; sync files=3, longest=0.000 s, average=0.000
s; distance=2727 kB, estimate=2779 kB

Ooops, it worked! We ignored the error and checkpointed OK.
_____________

Given your report, now I have to wonder if we even reissued the fsync() at
all this time. 'perf' time. OK, with

sudo perf record -e syscalls:sys_enter_fsync,syscalls:sys_exit_fsync -a
sudo perf script

I see the failed fync, then the same fd being fsync()d without error on the
next checkpoint, which succeeds.

postgres 9602 [003] 72380.325817: syscalls:sys_enter_fsync: fd:
0x00000005
postgres 9602 [003] 72380.325931: syscalls:sys_exit_fsync:
0xfffffffffffffffb
...
postgres 9602 [000] 72381.336767: syscalls:sys_enter_fsync: fd:
0x00000005
postgres 9602 [000] 72381.336840: syscalls:sys_exit_fsync: 0x0

... and Pg continues merrily on its way without realising it lost data:

[72379.834872] XFS (dm-0): writeback error on sector 118752
[72380.324707] XFS (dm-0): writeback error on sector 118688

In this test I set things up so the checkpointer would see the first
fsync() error. But if I make checkpoints less frequent, the bgwriter
aggressive, and kernel dirty writeback aggressive, it should be possible to
have the failure go completely unobserved too. I'll try that next, because
we've already largely concluded that the solution to the issue above is to
PANIC on fsync() error. But if we don't see the error at all we're in
trouble.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Apr 6, 2018 at 1:27 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 6 April 2018 at 07:37, Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
>> Note: as I've brought up in another thread, it turns out that PG is not
>> handling fsync errors correctly even when the OS _does_ do the right
>> thing (discovered by testing on FreeBSD).
>
> Yikes. For other readers, the related thread for this is
> https://www.postgresql.org/message-id/87y3i1ia4w.fsf@news-spur.riddles.org.uk

Yeah. That's really embarrassing, especially after beating up on
various operating systems all week. It's also an independent issue --
let's keep that on the other thread and get it fixed.

> I see the failed fync, then the same fd being fsync()d without error on the
> next checkpoint, which succeeds.
>
> postgres 9602 [003] 72380.325817: syscalls:sys_enter_fsync: fd:
> 0x00000005
> postgres 9602 [003] 72380.325931: syscalls:sys_exit_fsync:
> 0xfffffffffffffffb
> ...
> postgres 9602 [000] 72381.336767: syscalls:sys_enter_fsync: fd:
> 0x00000005
> postgres 9602 [000] 72381.336840: syscalls:sys_exit_fsync: 0x0
>
> ... and Pg continues merrily on its way without realising it lost data:
>
> [72379.834872] XFS (dm-0): writeback error on sector 118752
> [72380.324707] XFS (dm-0): writeback error on sector 118688
>
> In this test I set things up so the checkpointer would see the first fsync()
> error. But if I make checkpoints less frequent, the bgwriter aggressive, and
> kernel dirty writeback aggressive, it should be possible to have the failure
> go completely unobserved too. I'll try that next, because we've already
> largely concluded that the solution to the issue above is to PANIC on
> fsync() error. But if we don't see the error at all we're in trouble.

I suppose you only see errors because the file descriptors linger open
in the virtual file descriptor cache, which is a matter of luck
depending on how many relation segment files you touched. One thing
you could try to confirm our understand of the Linux 4.13+ policy
would be to hack PostgreSQL so that it reopens the file descriptor
every time in mdsync(). See attached.

--
Thomas Munro
http://www.enterprisedb.com

On 6 April 2018 at 10:53, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
wrote:

> On Fri, Apr 6, 2018 at 1:27 PM, Craig Ringer <craig(at)2ndquadrant(dot)com>
> wrote:
> > On 6 April 2018 at 07:37, Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>
> wrote:
> >> Note: as I've brought up in another thread, it turns out that PG is not
> >> handling fsync errors correctly even when the OS _does_ do the right
> >> thing (discovered by testing on FreeBSD).
> >
> > Yikes. For other readers, the related thread for this is
> > https://www.postgresql.org/message-id/87y3i1ia4w.fsf@
> news-spur.riddles.org.uk
>
> Yeah. That's really embarrassing, especially after beating up on
> various operating systems all week. It's also an independent issue --
> let's keep that on the other thread and get it fixed.
>
> > I see the failed fync, then the same fd being fsync()d without error on
> the
> > next checkpoint, which succeeds.
> >
> > postgres 9602 [003] 72380.325817: syscalls:sys_enter_fsync: fd:
> > 0x00000005
> > postgres 9602 [003] 72380.325931: syscalls:sys_exit_fsync:
> > 0xfffffffffffffffb
> > ...
> > postgres 9602 [000] 72381.336767: syscalls:sys_enter_fsync: fd:
> > 0x00000005
> > postgres 9602 [000] 72381.336840: syscalls:sys_exit_fsync: 0x0
> >
> > ... and Pg continues merrily on its way without realising it lost data:
> >
> > [72379.834872] XFS (dm-0): writeback error on sector 118752
> > [72380.324707] XFS (dm-0): writeback error on sector 118688
> >
> > In this test I set things up so the checkpointer would see the first
> fsync()
> > error. But if I make checkpoints less frequent, the bgwriter aggressive,
> and
> > kernel dirty writeback aggressive, it should be possible to have the
> failure
> > go completely unobserved too. I'll try that next, because we've already
> > largely concluded that the solution to the issue above is to PANIC on
> > fsync() error. But if we don't see the error at all we're in trouble.
>
> I suppose you only see errors because the file descriptors linger open
> in the virtual file descriptor cache, which is a matter of luck
> depending on how many relation segment files you touched.

In this case I think it's because the kernel didn't get around to doing the
writeback before the eagerly forced checkpoint fsync()'d it. Or we didn't
even queue it for writeback from our own shared_buffers until just before
we fsync()'d it. After all, it's a contrived test case that tries to
reproduce the issue rapidly with big writes and frequent checkpoints.

So the checkpointer had the relation open to fsync() it, and it was the
checkpointer's fsync() that did writeback on the dirty page and noticed the
error.

If we the kernel had done the writeback before the checkpointer opened the
relation to fsync() it, we might not have seen the error at all - though as
you note this depends on the file descriptor cache. You can see the
silent-error behaviour in my standalone test case where I confirmed the
post-4.13 behaviour. (I'm on 4.14 here).

I can try to reproduce it with postgres too, but it not only requires
closing and reopening the FDs, it also requires forcing writeback before
opening the fd. To make it occur in a practical timeframe I have to make my
kernel writeback settings insanely aggressive and/or call sync() before
re-open()ing. I don't really think it's worth it, since I've confirmed the
behaviour already with the simpler test in standalone/ in the rest repo. To
try it yourself, clone

https://github.com/ringerc/scrapcode

and in the master branch

cd testcases/fsync-error-clear
less README
make REOPEN=reopen standalone-run

See
https://github.com/ringerc/scrapcode/blob/master/testcases/fsync-error-clear/standalone/fsync-error-clear.c#L118
.

I've pushed the postgres test to that repo too; "make postgres-run".

You'll need docker, and be warned, it's using privileged docker containers
and messing with dmsetup.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

So, what can we actually do about this new Linux behaviour?

Idea 1:

* whenever you open a file, either tell the checkpointer so it can
open it too (and wait for it to tell you that it has done so, because
it's not safe to write() until then), or send it a copy of the file
descriptor via IPC (since duplicated file descriptors share the same
f_wb_err)

* if the checkpointer can't take any more file descriptors (how would
that limit even work in the IPC case?), then it somehow needs to tell
you that so that you know that you're responsible for fsyncing that
file yourself, both on close (due to fd cache recycling) and also when
the checkpointer tells you to

Maybe it could be made to work, but sheesh, that seems horrible. Is
there some simpler idea along these lines that could make sure that
fsync() is only ever called on file descriptors that were opened
before all unflushed writes, or file descriptors cloned from such file
descriptors?

Idea 2:

Give up, complain that this implementation is defective and
unworkable, both on POSIX-compliance grounds and on POLA grounds, and
campaign to get it fixed more fundamentally (actual details left to
the experts, no point in speculating here, but we've seen a few
approaches that work on other operating systems including keeping
buffers dirty and marking the whole filesystem broken/read-only).

Idea 3:

Give up on buffered IO and develop an O_SYNC | O_DIRECT based system ASAP.

Any other ideas?

For a while I considered suggesting an idea which I now think doesn't
work. I thought we could try asking for a new fcntl interface that
spits out wb_err counter. Call it an opaque error token or something.
Then we could store it in our fsync queue and safely close the file.
Check again before fsync()ing, and if we ever see a different value,
PANIC because it means a writeback error happened while we weren't
looking. Sadly I think it doesn't work because AIUI inodes are not
pinned in kernel memory when no one has the file open and there are no
dirty buffers, so I think the counters could go away and be reset.
Perhaps you could keep inodes pinned by keeping the associated buffers
dirty after an error (like FreeBSD), but if you did that you'd have
solved the problem already and wouldn't really need the wb_err system
at all. Is there some other idea long these lines that could work?

--
Thomas Munro
http://www.enterprisedb.com

On Sun, Apr 8, 2018 at 02:16:07PM +1200, Thomas Munro wrote:
> So, what can we actually do about this new Linux behaviour?
>
> Idea 1:
>
> * whenever you open a file, either tell the checkpointer so it can
> open it too (and wait for it to tell you that it has done so, because
> it's not safe to write() until then), or send it a copy of the file
> descriptor via IPC (since duplicated file descriptors share the same
> f_wb_err)
>
> * if the checkpointer can't take any more file descriptors (how would
> that limit even work in the IPC case?), then it somehow needs to tell
> you that so that you know that you're responsible for fsyncing that
> file yourself, both on close (due to fd cache recycling) and also when
> the checkpointer tells you to
>
> Maybe it could be made to work, but sheesh, that seems horrible. Is
> there some simpler idea along these lines that could make sure that
> fsync() is only ever called on file descriptors that were opened
> before all unflushed writes, or file descriptors cloned from such file
> descriptors?
>
> Idea 2:
>
> Give up, complain that this implementation is defective and
> unworkable, both on POSIX-compliance grounds and on POLA grounds, and
> campaign to get it fixed more fundamentally (actual details left to
> the experts, no point in speculating here, but we've seen a few
> approaches that work on other operating systems including keeping
> buffers dirty and marking the whole filesystem broken/read-only).
>
> Idea 3:
>
> Give up on buffered IO and develop an O_SYNC | O_DIRECT based system ASAP.

Idea 4 would be for people to assume their database is corrupt if their
server logs report any I/O error on the file systems Postgres uses.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

> On Apr 7, 2018, at 19:33, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> Idea 4 would be for people to assume their database is corrupt if their
> server logs report any I/O error on the file systems Postgres uses.

Pragmatically, that's where we are right now. The best answer in this bad situation is (a) fix the error, then (b) replay from a checkpoint before the error occurred, but it appears we can't even guarantee that a PostgreSQL process will be the one to see the error.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

On 8 April 2018 at 10:16, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
wrote:

> So, what can we actually do about this new Linux behaviour?
>

Yeah, I've been cooking over that myself.

More below, but here's an idea #5: decide InnoDB has the right idea, and go
to using a single massive blob file, or a few giant blobs.

We have a storage abstraction that makes this way, way less painful than it
should be.

We can virtualize relfilenodes into storage extents in relatively few big
files. We could use sparse regions to make the addressing more convenient,
but that makes copying and backup painful, so I'd rather not.

Even one file per tablespace for persistent relation heaps, another for
indexes, another for each fork type.

That way we can use something like your #1 (which is what I was also
thinking about then rejecting previously), but reduce the pain by reducing
the FD count drastically so exhausting FDs stops being a problem.

Previously I was leaning toward what you've described here:

> * whenever you open a file, either tell the checkpointer so it can
> open it too (and wait for it to tell you that it has done so, because
> it's not safe to write() until then), or send it a copy of the file
> descriptor via IPC (since duplicated file descriptors share the same
> f_wb_err)
>
> * if the checkpointer can't take any more file descriptors (how would
> that limit even work in the IPC case?), then it somehow needs to tell
> you that so that you know that you're responsible for fsyncing that
> file yourself, both on close (due to fd cache recycling) and also when
> the checkpointer tells you to
>
> Maybe it could be made to work, but sheesh, that seems horrible. Is
> there some simpler idea along these lines that could make sure that
> fsync() is only ever called on file descriptors that were opened
> before all unflushed writes, or file descriptors cloned from such file
> descriptors?
>

... and got stuck on "yuck, that's awful".

I was assuming we'd force early checkpoints if the checkpointer hit its fd
limit, but that's even worse.

We'd need to urgently do away with segmented relations, and partitions
would start to become a hinderance.

Even then it's going to be an unworkable nightmare with heavily partitioned
systems, systems that use schema-sharding, etc. And it'll mean we need to
play with process limits and, often, system wide limits on FDs. I imagine
the performance implications won't be pretty.

Idea 2:
>
> Give up, complain that this implementation is defective and
> unworkable, both on POSIX-compliance grounds and on POLA grounds, and
> campaign to get it fixed more fundamentally (actual details left to
> the experts, no point in speculating here, but we've seen a few
> approaches that work on other operating systems including keeping
> buffers dirty and marking the whole filesystem broken/read-only).
>

This appears to be what SQLite does AFAICS.

https://www.sqlite.org/atomiccommit.html

though it has the huge luxury of a single writer, so it's probably only
subject to the original issue not the multiprocess / checkpointer issues we
face.

> Idea 3:
>
> Give up on buffered IO and develop an O_SYNC | O_DIRECT based system ASAP.
>

That seems to be what the kernel folks will expect. But that's going to
KILL performance. We'll need writer threads to have any hope of it not
*totally* sucking, because otherwise simple things like updating a heap
tuple and two related indexes will incur enormous disk latencies.

But I suspect it's the path forward.

Goody.

> Any other ideas?
>
> For a while I considered suggesting an idea which I now think doesn't
> work. I thought we could try asking for a new fcntl interface that
> spits out wb_err counter. Call it an opaque error token or something.
> Then we could store it in our fsync queue and safely close the file.
> Check again before fsync()ing, and if we ever see a different value,
> PANIC because it means a writeback error happened while we weren't
> looking. Sadly I think it doesn't work because AIUI inodes are not
> pinned in kernel memory when no one has the file open and there are no
> dirty buffers, so I think the counters could go away and be reset.
> Perhaps you could keep inodes pinned by keeping the associated buffers
> dirty after an error (like FreeBSD), but if you did that you'd have
> solved the problem already and wouldn't really need the wb_err system
> at all. Is there some other idea long these lines that could work?

I think our underlying data syncing concept is fundamentally broken, and
it's not really the kernel's fault.

We assume that we can safely:

procA: open()
procA: write()
procA: close()

... some long time later, unbounded as far as the kernel is concerned ...

procB: open()
procB: fsync()
procB: close()

If the kernel does writeback in the middle, how on earth is it supposed to
know we expect to reopen the file and check back later?

Should it just remember "this file had an error" forever, and tell every
caller? In that case how could we recover? We'd need some new API to say
"yeah, ok already, I'm redoing all my work since the last good fsync() so
you can clear the error flag now". Otherwise it'd keep reporting an error
after we did redo to recover, too.

I never really clicked to the fact that we closed relations with pending
buffered writes, left them closed, then reopened them to fsync. That's ....
well, the kernel isn't the only thing doing crazy things here.

Right now I think we're at option (4): If you see anything that smells like
a write error in your kernel logs, hard-kill postgres with -m immediate (do
NOT let it do a shutdown checkpoint). If it did a checkpoint since the
logs, fake up a backup label to force redo to start from the last
checkpoint before the error. Otherwise, it's safe to just let it start up
again and do redo again.

Fun times.

This also means AFAICS that running Pg on NFS is extremely unsafe, you MUST
make sure you don't run out of disk. Because the usual safeguard of space
reservation against ENOSPC in fsync doesn't apply to NFS. (I haven't tested
this with nfsv3 in sync,hard,nointr mode yet, *maybe* that's safe, but I
doubt it). The same applies to thin-provisioned storage. Just. Don't.

This helps explain various reports of corruption in Docker and various
other tools that use various sorts of thin provisioning. If you hit ENOSPC
in fsync(), bye bye data.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Apr 7, 2018 at 8:27 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> More below, but here's an idea #5: decide InnoDB has the right idea, and go
> to using a single massive blob file, or a few giant blobs.
>
> We have a storage abstraction that makes this way, way less painful than it
> should be.
>
> We can virtualize relfilenodes into storage extents in relatively few big
> files. We could use sparse regions to make the addressing more convenient,
> but that makes copying and backup painful, so I'd rather not.
>
> Even one file per tablespace for persistent relation heaps, another for
> indexes, another for each fork type.

I'm not sure that we can do that now, since it would break the new
"Optimize btree insertions for common case of increasing values"
optimization. (I did mention this before it went in.)

I've asked Pavan to at least add a note to the nbtree README that
explains the high level theory behind the optimization, as part of
post-commit clean-up. I'll ask him to say something about how it might
affect extent-based storage, too.

--
Peter Geoghegan

> On Apr 7, 2018, at 20:27, Craig Ringer <craig(at)2ndQuadrant(dot)com> wrote:
>
> Right now I think we're at option (4): If you see anything that smells like a write error in your kernel logs, hard-kill postgres with -m immediate (do NOT let it do a shutdown checkpoint). If it did a checkpoint since the logs, fake up a backup label to force redo to start from the last checkpoint before the error. Otherwise, it's safe to just let it start up again and do redo again.

Before we spiral down into despair and excessive alcohol consumption, this is basically the same situation as a checksum failure or some other kind of uncorrected media-level error. The bad part is that we have to find out from the kernel logs rather than from PostgreSQL directly. But this does not strike me as otherwise significantly different from, say, an infrequently-accessed disk block reporting an uncorrectable error when we finally get around to reading it.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

On 04/08/2018 05:27 AM, Craig Ringer wrote:> More below, but here's an
idea #5: decide InnoDB has the right idea, and
> go to using a single massive blob file, or a few giant blobs.

FYI: MySQL has by default one file per table these days. The old
approach with one massive file was a maintenance headache so they change
the default some releases ago.

https://dev.mysql.com/doc/refman/8.0/en/innodb-multiple-tablespaces.html

Andreas

On 8 April 2018 at 11:46, Christophe Pettus <xof(at)thebuild(dot)com> wrote:

>
> > On Apr 7, 2018, at 20:27, Craig Ringer <craig(at)2ndQuadrant(dot)com> wrote:
> >
> > Right now I think we're at option (4): If you see anything that smells
> like a write error in your kernel logs, hard-kill postgres with -m
> immediate (do NOT let it do a shutdown checkpoint). If it did a checkpoint
> since the logs, fake up a backup label to force redo to start from the last
> checkpoint before the error. Otherwise, it's safe to just let it start up
> again and do redo again.
>
> Before we spiral down into despair and excessive alcohol consumption, this
> is basically the same situation as a checksum failure or some other kind of
> uncorrected media-level error. The bad part is that we have to find out
> from the kernel logs rather than from PostgreSQL directly. But this does
> not strike me as otherwise significantly different from, say, an
> infrequently-accessed disk block reporting an uncorrectable error when we
> finally get around to reading it.
>

I don't entirely agree - because it affects ENOSPC, I/O errors on thin
provisioned storage, I/O errors on multipath storage, etc. (I identified
the original issue on a thin provisioned system that ran out of backing
space, mangling PostgreSQL in a way that made no sense at the time).

These are way more likely than bit flips or other storage level corruption,
and things that we previously expected to detect and fail gracefully for.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 8 April 2018 at 17:41, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:

> On 04/08/2018 05:27 AM, Craig Ringer wrote:> More below, but here's an
> idea #5: decide InnoDB has the right idea, and
>
>> go to using a single massive blob file, or a few giant blobs.
>>
>
> FYI: MySQL has by default one file per table these days. The old approach
> with one massive file was a maintenance headache so they change the default
> some releases ago.
>
> https://dev.mysql.com/doc/refman/8.0/en/innodb-multiple-tablespaces.html
>
>
Huh, thanks for the update.

We should see how they handle reliable flushing and see if they've looked
into it. If they haven't, we should give them a heads-up and if they have,
lets learn from them.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

> On Apr 8, 2018, at 03:30, Craig Ringer <craig(at)2ndQuadrant(dot)com> wrote:
>
> These are way more likely than bit flips or other storage level corruption, and things that we previously expected to detect and fail gracefully for.

This is definitely bad, and it explains a few otherwise-inexplicable corruption issues we've seen. (And great work tracking it down!) I think it's important not to panic, though; PostgreSQL doesn't have a reputation for horrible data integrity. I'm not sure it makes sense to do a major rearchitecting of the storage layer (especially with pluggable storage coming along) to address this. While the failure modes are more common, the solution (a PITR backup) is one that an installation should have anyway against media failures.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

On 8 April 2018 at 04:27, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 8 April 2018 at 10:16, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
> wrote:
>
> If the kernel does writeback in the middle, how on earth is it supposed to
> know we expect to reopen the file and check back later?
>
> Should it just remember "this file had an error" forever, and tell every
> caller? In that case how could we recover? We'd need some new API to say
> "yeah, ok already, I'm redoing all my work since the last good fsync() so
> you can clear the error flag now". Otherwise it'd keep reporting an error
> after we did redo to recover, too.

There is no spoon^H^H^H^H^Herror flag. We don't need fsync to keep
track of any errors. We just need fsync to accurately report whether
all the buffers in the file have been written out. When you call fsync
again the kernel needs to initiate i/o on all the dirty buffers and
block until they complete successfully. If they complete successfully
then nobody cares whether they had some failure in the past when i/o
was initiated at some point in the past.

The problem is not that errors aren't been tracked correctly. The
problem is that dirty buffers are being marked clean when they haven't
been written out. They consider dirty filesystem buffers when there's
hardware failure preventing them from being written "a memory leak".

As long as any error means the kernel has discarded writes then
there's no real hope of any reliable operation through that interface.

Going to DIRECTIO is basically recognizing this. That the kernel
filesystem buffer provides no reliable interface so we need to
reimplement it ourselves in user space.

It's rather disheartening. Aside from having to do all that work we
have the added barrier that we don't have as much information about
the hardware as the kernel has. We don't know where raid stripes begin
and end, how big the memory controller buffers are or how to tell when
they're full or empty or how to flush them. etc etc. We also don't
know what else is going on on the machine.

--
greg

> On Apr 8, 2018, at 14:23, Greg Stark <stark(at)mit(dot)edu> wrote:
>
> They consider dirty filesystem buffers when there's
> hardware failure preventing them from being written "a memory leak".

That's not an irrational position. File system buffers are *not* dedicated memory for file system caching; they're being used for that because no one has a better use for them at that moment. If an inability to flush them to disk meant that they suddenly became pinned memory, a large copy operation to a yanked USB drive could result in the system having no more allocatable memory. I guess in theory that they could swap them, but swapping out a file system buffer in hopes that sometime in the future it could be properly written doesn't seem very architecturally sound to me.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

On Sun, Apr 08, 2018 at 10:23:21PM +0100, Greg Stark wrote:
> On 8 April 2018 at 04:27, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> > On 8 April 2018 at 10:16, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
> > wrote:
> >
> > If the kernel does writeback in the middle, how on earth is it supposed to
> > know we expect to reopen the file and check back later?
> >
> > Should it just remember "this file had an error" forever, and tell every
> > caller? In that case how could we recover? We'd need some new API to say
> > "yeah, ok already, I'm redoing all my work since the last good fsync() so
> > you can clear the error flag now". Otherwise it'd keep reporting an error
> > after we did redo to recover, too.
>
> There is no spoon^H^H^H^H^Herror flag. We don't need fsync to keep
> track of any errors. We just need fsync to accurately report whether
> all the buffers in the file have been written out. When you call fsync

Instead, fsync() reports when some of the buffers have not been
written out, due to reasons outlined before. As such it may make
some sense to maintain some tracking regarding errors even after
marking failed dirty pages as clean (in fact it has been proposed,
but this introduces memory overhead).

> again the kernel needs to initiate i/o on all the dirty buffers and
> block until they complete successfully. If they complete successfully
> then nobody cares whether they had some failure in the past when i/o
> was initiated at some point in the past.

The question is, what should the kernel and application do in cases
where this is simply not possible (according to freebsd that keeps
dirty pages around after failure, for example, -EIO from the block
layer is a contract for unrecoverable errors so it is pointless to
keep them dirty). You'd need a specialized interface to clear-out
the errors (and drop the dirty pages), or potentially just remount
the filesystem.

> The problem is not that errors aren't been tracked correctly. The
> problem is that dirty buffers are being marked clean when they haven't
> been written out. They consider dirty filesystem buffers when there's
> hardware failure preventing them from being written "a memory leak".
>
> As long as any error means the kernel has discarded writes then
> there's no real hope of any reliable operation through that interface.

This does not necessarily follow. Whether the kernel discards writes
or not would not really help (see above). It is more a matter of
proper "reporting contract" between userspace and kernel, and tracking
would be a way for facilitating this vs. having a more complex userspace
scheme (as described by others in this thread) where synchronization
for fsync() is required in a multi-process application.

Best regards,
Anthony

On Sun, Apr 8, 2018 at 09:38:03AM -0700, Christophe Pettus wrote:
>
> > On Apr 8, 2018, at 03:30, Craig Ringer <craig(at)2ndQuadrant(dot)com>
> > wrote:
> >
> > These are way more likely than bit flips or other storage level
> > corruption, and things that we previously expected to detect and
> > fail gracefully for.
>
> This is definitely bad, and it explains a few otherwise-inexplicable
> corruption issues we've seen. (And great work tracking it down!) I
> think it's important not to panic, though; PostgreSQL doesn't have a
> reputation for horrible data integrity. I'm not sure it makes sense
> to do a major rearchitecting of the storage layer (especially with
> pluggable storage coming along) to address this. While the failure
> modes are more common, the solution (a PITR backup) is one that an
> installation should have anyway against media failures.

I think the big problem is that we don't have any way of stopping
Postgres at the time the kernel reports the errors to the kernel log, so
we are then returning potentially incorrect results and committing
transactions that might be wrong or lost. If we could stop Postgres
when such errors happen, at least the administrator could fix the
problem of fail-over to a standby.

An crazy idea would be to have a daemon that checks the logs and stops
Postgres when it seems something wrong.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

> On Apr 8, 2018, at 15:29, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> I think the big problem is that we don't have any way of stopping
> Postgres at the time the kernel reports the errors to the kernel log, so
> we are then returning potentially incorrect results and committing
> transactions that might be wrong or lost.

Yeah, it's bad. In the short term, the best advice to installations is to monitor their kernel logs for errors (which very few do right now), and make sure they have a backup strategy which can encompass restoring from an error like this. Even Craig's smart fix of patching the backup label to recover from a previous checkpoint doesn't do much good if we don't have WAL records back that far (or one of the required WAL records also took a hit).

In the longer term... O_DIRECT seems like the most plausible way out of this, but that might be popular with people running on file systems or OSes that don't have this issue. (Setting aside the daunting prospect of implementing that.)

--
-- Christophe Pettus
xof(at)thebuild(dot)com

On 2018-04-08 18:29:16 -0400, Bruce Momjian wrote:
> On Sun, Apr 8, 2018 at 09:38:03AM -0700, Christophe Pettus wrote:
> >
> > > On Apr 8, 2018, at 03:30, Craig Ringer <craig(at)2ndQuadrant(dot)com>
> > > wrote:
> > >
> > > These are way more likely than bit flips or other storage level
> > > corruption, and things that we previously expected to detect and
> > > fail gracefully for.
> >
> > This is definitely bad, and it explains a few otherwise-inexplicable
> > corruption issues we've seen. (And great work tracking it down!) I
> > think it's important not to panic, though; PostgreSQL doesn't have a
> > reputation for horrible data integrity. I'm not sure it makes sense
> > to do a major rearchitecting of the storage layer (especially with
> > pluggable storage coming along) to address this. While the failure
> > modes are more common, the solution (a PITR backup) is one that an
> > installation should have anyway against media failures.
>
> I think the big problem is that we don't have any way of stopping
> Postgres at the time the kernel reports the errors to the kernel log, so
> we are then returning potentially incorrect results and committing
> transactions that might be wrong or lost. If we could stop Postgres
> when such errors happen, at least the administrator could fix the
> problem of fail-over to a standby.
>
> An crazy idea would be to have a daemon that checks the logs and stops
> Postgres when it seems something wrong.

I think the danger presented here is far smaller than some of the
statements in this thread might make one think. In all likelihood, once
you've got an IO error that kernel level retries don't fix, your
database is screwed. Whether fsync reports that or not is really
somewhat besides the point. We don't panic that way when getting IO
errors during reads either, and they're more likely to be persistent
than errors during writes (because remapping on storage layer can fix
issues, but not during reads).

There's a lot of not so great things here, but I don't think there's any
need to panic.

We should fix things so that reported errors are treated with crash
recovery, and for the rest I think there's very fair arguments to be
made that that's far outside postgres's remit.

I think there's pretty good reasons to go to direct IO where supported,
but error handling doesn't strike me as a particularly good reason for
the move.

Greetings,

Andres Freund

> On Apr 8, 2018, at 16:16, Andres Freund <andres(at)anarazel(dot)de> wrote:
> We don't panic that way when getting IO
> errors during reads either, and they're more likely to be persistent
> than errors during writes (because remapping on storage layer can fix
> issues, but not during reads).

There is a distinction to be drawn there, though, because we immediately pass an error back to the client on a read, but a write problem in this situation can be masked for an extended period of time.

That being said...

> There's a lot of not so great things here, but I don't think there's any
> need to panic.

No reason to panic, yes. We can assume that if this was a very big persistent problem, it would be much more widely reported. It would, however, be good to find a way to get the error surfaced back up to the client in a way that is not just monitoring the kernel logs.

--
-- Christophe Pettus
xof(at)thebuild(dot)com

On 9 April 2018 at 05:28, Christophe Pettus <xof(at)thebuild(dot)com> wrote:

>
> > On Apr 8, 2018, at 14:23, Greg Stark <stark(at)mit(dot)edu> wrote:
> >
> > They consider dirty filesystem buffers when there's
> > hardware failure preventing them from being written "a memory leak".
>
> That's not an irrational position. File system buffers are *not*
> dedicated memory for file system caching; they're being used for that
> because no one has a better use for them at that moment. If an inability
> to flush them to disk meant that they suddenly became pinned memory, a
> large copy operation to a yanked USB drive could result in the system
> having no more allocatable memory. I guess in theory that they could swap
> them, but swapping out a file system buffer in hopes that sometime in the
> future it could be properly written doesn't seem very architecturally sound
> to me.
>

Yep.

Another example is a write to an NFS or iSCSI volume that goes away
forever. What if the app keeps write()ing in the hopes it'll come back, and
by the time the kernel starts reporting EIO for write(), it's already
saddled with a huge volume of dirty writeback buffers it can't get rid of
because someone, one day, might want to know about them?

You could make the argument that it's OK to forget if the entire file
system goes away. But actually, why is that ok? What if it's remounted
again? That'd be really bad too, for someone expecting write reliability.

You can coarsen from dirty buffer tracking to marking the FD(s) bad, but
what if there's no FD to mark because the file isn't open at the moment?

You can mark the inode cache entry and pin it, I guess. But what if your
app triggered I/O errors over vast numbers of small files? Again, the
kernel's left holding the ball.

It doesn't know if/when an app will return to check. It doesn't know how
long to remember the failure for. It doesn't know when all interested
clients have been informed and it can treat the fault as cleared/repaired,
either, so it'd have to *keep on reporting EIO for PostgreSQL's own writes
and fsyncs() indefinitely*, even once we do recovery.

The only way it could avoid that would be to keep the dirty writeback pages
around and flagged bad, then clear the flag when a new write() replaces the
same file range. I can't imagine that being practical.

Blaming the kernel for this sure is the easy way out.

But IMO we cannot rationally expect the kernel to remember error state
forever for us, then forget it when we expect, all without actually telling
it anything about our activities or even that we still exist and are still
interested in the files/writes. We've closed the files and gone away.

Whatever we do, it's likely going to have to involve not doing that anymore.

Even if we can somehow convince the kernel folks to add a new interface for
us that reports I/O errors to some listener, like an
inotify/fnotify/dnotify/whatever-it-is-today-notify extension reporting
errors in buffered async writes, we won't be able to rely on having it for
5-10 years, and only on Linux.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 9 April 2018 at 06:29, Bruce Momjian <bruce(at)momjian(dot)us> wrote:

>
> I think the big problem is that we don't have any way of stopping
> Postgres at the time the kernel reports the errors to the kernel log, so
> we are then returning potentially incorrect results and committing
> transactions that might be wrong or lost.

Right.

Specifically, we need a way to ask the kernel at checkpoint time "was
everything written to [this set of files] flushed successfully since the
last time I asked, no matter who did the writing and no matter how the
writes were flushed?"

If the result is "no" we PANIC and redo. If the hardware/volume is screwed,
the user can fail over to a standby, do PITR, etc.

But we don't have any way to ask that reliably at present.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

On 2018-04-08 16:27:57 -0700, Christophe Pettus wrote:
> > On Apr 8, 2018, at 16:16, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > We don't panic that way when getting IO
> > errors during reads either, and they're more likely to be persistent
> > than errors during writes (because remapping on storage layer can fix
> > issues, but not during reads).
>
> There is a distinction to be drawn there, though, because we
> immediately pass an error back to the client on a read, but a write
> problem in this situation can be masked for an extended period of
> time.

Only if you're "lucky" enough that your clients actually read that data,
and then you're somehow able to figure out across the whole stack that
these 0.001% of transactions that fail are due to IO errors. Or you also
need to do log analysis.

If you want to solve things like that you need regular reads of all your
data, including verifications etc.

Greetings,

Andres Freund

On 9 April 2018 at 07:16, Andres Freund <andres(at)anarazel(dot)de> wrote:

>
> I think the danger presented here is far smaller than some of the
> statements in this thread might make one think.

Clearly it's not happening a huge amount or we'd have a lot of noise about
Pg eating people's data, people shouting about how unreliable it is, etc.
We don't. So it's not some earth shattering imminent threat to everyone's
data. It's gone unnoticed, or the root cause unidentified, for a long time.

I suspect we've written off a fair few issues in the past as "it'd bad
hardware" when actually, the hardware fault was the trigger for a Pg/kernel
interaction bug. And blamed containers for things that weren't really the
container's fault. But even so, if it were happening tons, we'd hear more
noise.

I've already been very surprised there when I learned that PostgreSQL
completely ignores wholly absent relfilenodes. Specifically, if you
unlink() a relation's backing relfilenode while Pg is down and that file
has writes pending in the WAL. We merrily re-create it with uninitalized
pages and go on our way. As Andres pointed out in an offlist discussion,
redo isn't a consistency check, and it's not obliged to fail in such cases.
We can say "well, don't do that then" and define away file losses from FS
corruption etc as not our problem, the lower levels we expect to take care
of this have failed.

We have to look at what checkpoints are and are not supposed to promise,
and whether this is a problem we just define away as "not our problem, the
lower level failed, we're not obliged to detect this and fail gracefully."

We can choose to say that checkpoints are required to guarantee crash/power
loss safety ONLY and do not attempt to protect against I/O errors of any
sort. In fact, I think we should likely amend the documentation for release
versions to say just that.

In all likelihood, once
> you've got an IO error that kernel level retries don't fix, your
> database is screwed.

Your database is going to be down or have interrupted service. It's
possible you may have some unreadable data. This could result in localised
damage to one or more relations. That could affect FK relationships,
indexes, all sorts. If you're really unlucky you might lose something
critical like pg_clog/ contents.

But in general your DB should be repairable/recoverable even in those cases.

And in many failure modes there's no reason to expect any data loss at all,
like:

* Local disk fills up (seems to be safe already due to space reservation at
write() time)
* Thin-provisioned storage backing local volume iSCSI or paravirt block
device fills up
* NFS volume fills up
* Multipath I/O error
* Interruption of connectivity to network block device
* Disk develops localized bad sector where we haven't previously written
data

Except for the ENOSPC on NFS, all the rest of the cases can be handled by
expecting the kernel to retry forever and not return until the block is
written or we reach the heat death of the universe. And NFS, well...

Part of the trouble is that the kernel *won't* retry forever in all these
cases, and doesn't seem to have a way to ask it to in all cases.

And if the user hasn't configured it for the right behaviour in terms of
I/O error resilience, we don't find out about it.

So it's not the end of the world, but it'd sure be nice to fix.

Whether fsync reports that or not is really
> somewhat besides the point. We don't panic that way when getting IO
> errors during reads either, and they're more likely to be persistent
> than errors during writes (because remapping on storage layer can fix
> issues, but not during reads).
>

That's because reads don't make promises about what's committed and synced.
I think that's quite different.

> We should fix things so that reported errors are treated with crash
> recovery, and for the rest I think there's very fair arguments to be
> made that that's far outside postgres's remit.
>

Certainly for current versions.

I think we need to think about a more robust path in future. But it's
certainly not "stop the world" territory.

The docs need an update to indicate that we explicitly disclaim
responsibility for I/O errors on async writes, and that the kernel and I/O
stack must be configured never to give up on buffered writes. If it does,
that's not our problem anymore.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2018-04-09 10:00:41 +0800, Craig Ringer wrote:
> I suspect we've written off a fair few issues in the past as "it'd bad
> hardware" when actually, the hardware fault was the trigger for a Pg/kernel
> interaction bug. And blamed containers for things that weren't really the
> container's fault. But even so, if it were happening tons, we'd hear more
> noise.

Agreed on that, but I think that's FAR more likely to be things like
multixacts, index structure corruption due to logic bugs etc.

> I've already been very surprised there when I learned that PostgreSQL
> completely ignores wholly absent relfilenodes. Specifically, if you
> unlink() a relation's backing relfilenode while Pg is down and that file
> has writes pending in the WAL. We merrily re-create it with uninitalized
> pages and go on our way. As Andres pointed out in an offlist discussion,
> redo isn't a consistency check, and it's not obliged to fail in such cases.
> We can say "well, don't do that then" and define away file losses from FS
> corruption etc as not our problem, the lower levels we expect to take care
> of this have failed.

And it'd be a realy bad idea to behave differently.

> And in many failure modes there's no reason to expect any data loss at all,
> like:
>
> * Local disk fills up (seems to be safe already due to space reservation at
> write() time)

That definitely should be treated separately.

> * Thin-provisioned storage backing local volume iSCSI or paravirt block
> device fills up
> * NFS volume fills up

Those should be the same as the above.

> I think we need to think about a more robust path in future. But it's
> certainly not "stop the world" territory.

I think you're underestimating the complexity of doing that by at least
two orders of magnitude.

Greetings,

Andres Freund

On 9 April 2018 at 10:06, Andres Freund <andres(at)anarazel(dot)de> wrote:

>
> > And in many failure modes there's no reason to expect any data loss at
> all,
> > like:
> >
> > * Local disk fills up (seems to be safe already due to space reservation
> at
> > write() time)
>
> That definitely should be treated separately.
>

It is, because all the FSes I looked at reserve space before returning from
write(), even if they do delayed allocation. So they won't fail with ENOSPC
at fsync() time or silently due to lost errors on background writeback.
Otherwise we'd be hearing a LOT more noise about this.

> > * Thin-provisioned storage backing local volume iSCSI or paravirt block
> > device fills up
> > * NFS volume fills up
>
> Those should be the same as the above.
>

Unfortunately, they aren't.

AFAICS NFS doesn't reserve space with the other end before returning from
write(), even if mounted with the sync option. So we can get ENOSPC lazily
when the buffer writeback fails due to a full backing file system. This
then travels the same paths as EIO: we fsync(), ERROR, retry, appear to
succeed, and carry on with life losing the data. Or we never hear about the
error in the first place.

(There's a proposed extension that'd allow this, see
https://tools.ietf.org/html/draft-iyer-nfsv4-space-reservation-ops-02#page-5,
but I see no mention of it in fs/nfs. All the reserve_space /
xdr_reserve_space stuff seems to be related to space in protocol messages
at a quick read.)

Thin provisioned storage could vary a fair bit depending on the
implementation. But the specific failure case I saw, prompting this thread,
was on a volume using the stack:

xfs -> lvm2 -> multipath -> ??? -> SAN

(the HBA/iSCSI/whatever was not recorded by the looks, but IIRC it was
iSCSI. I'm checking.)

The SAN ran out of space. Due to use of thin provisioning, Linux *thought*
there was plenty of space on the volume; LVM thought it had plenty of
physical extents free and unallocated, XFS thought there was tons of free
space, etc. The space exhaustion manifested as I/O errors on flushes of
writeback buffers.

The logs were like this:

kernel: sd 2:0:0:1: [sdd] Unhandled sense code
kernel: sd 2:0:0:1: [sdd]
kernel: Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
kernel: sd 2:0:0:1: [sdd]
kernel: Sense Key : Data Protect [current]
kernel: sd 2:0:0:1: [sdd]
kernel: Add. Sense: Space allocation failed write protect
kernel: sd 2:0:0:1: [sdd] CDB:
kernel: Write(16): **HEX-DATA-CUT-OUT**
kernel: Buffer I/O error on device dm-0, logical block 3098338786
kernel: lost page write due to I/O error on dm-0
kernel: Buffer I/O error on device dm-0, logical block 3098338787

The immediate cause was that Linux's multipath driver didn't seem to
recognise the sense code as retryable, so it gave up and reported it to the
next layer up (LVM). LVM and XFS both seem to think that the lower layer is
responsible for retries, so they toss the write away, and tell any
interested writers if they feel like it, per discussion upthread.

In this case Pg did get the news and reported fsync() errors on
checkpoints, but it only reported an error once per relfilenode. Once it
ran out of failed relfilenodes to cause the checkpoint to ERROR, it
"completed" a "successful" checkpoint and kept on running until the
resulting corruption started to manifest its self and it segfaulted some
time later. As we've now learned, there's no guarantee we'd even get the
news about the I/O errors at all.

WAL was on a separate volume that didn't run out of room immediately, so we
didn't PANIC on WAL write failure and prevent the issue.

In this case if Pg had PANIC'd (and been able to guarantee to get the news
of write failures reliably), there'd have been no corruption and no data
loss despite the underlying storage issue.

If, prior to seeing this, you'd asked me "will my PostgreSQL database be
corrupted if my thin-provisioned volume runs out of space" I'd have said
"Surely not. PostgreSQL won't be corrupted by running out of disk space, it
orders writes carefully and forces flushes so that it will recover
gracefully from write failures."

Except not. I was very surprised.

BTW, it also turns out that the *default* for multipath is to give up on
errors anyway; see the queue_if_no_path option and no_path_retries options.
(Hint: run PostgreSQL with no_path_retries=queue). That's a sane default if
you use O_DIRECT|O_SYNC, and otherwise pretty much a data-eating setup.

I regularly see rather a lot of multipath systems, iSCSI systems, SAN
backed systems, etc. I think we need to be pretty clear that we expect them
to retry indefinitely, and if they report an I/O error we cannot reliably
handle it. We need to patch Pg to PANIC on any fsync() failure and document
that Pg won't notice some storage failure modes that might otherwise be
considered nonfatal or transient, so very specific storage configuration
and testing is required. (Not that anyone will do it). Also warn against
running on NFS even with "hard,sync,nointr".

It'd be interesting to have a tool that tested error handling, allowing
people to do iSCSI plug-pull tests, that sort of thing. But as far as I can
tell nobody ever tests their storage stack anyway, so I don't plan on
writing something that'll never get used.

> > I think we need to think about a more robust path in future. But it's
> > certainly not "stop the world" territory.
>
> I think you're underestimating the complexity of doing that by at least
> two orders of magnitude.

Oh, it's just a minor total rewrite of half Pg, no big deal ;)

I'm sure that no matter how big I think it is, I'm still underestimating it.

The most workable option IMO would be some sort of fnotify/dnotify/whatever
that reports all I/O errors on a volume. Some kind of error reporting
handle we can keep open on a volume level that we can check for each
volume/tablespace after we fsync() everything to see if it all really
worked. If we PANIC if that gives us a bad answer, and PANIC on fsync
errors, we guard against the great majority of these sorts of
should-be-transient-if-the-kernel-didn't-give-up-and-throw-away-our-data
errors.

Even then, good luck getting those events from an NFS volume in which the
backing volume experiences an issue.

And it's kind of moot because AFAICS no such interface exists.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 8 April 2018 at 22:47, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:
> On Sun, Apr 08, 2018 at 10:23:21PM +0100, Greg Stark wrote:
>> On 8 April 2018 at 04:27, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> > On 8 April 2018 at 10:16, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
>
> The question is, what should the kernel and application do in cases
> where this is simply not possible (according to freebsd that keeps
> dirty pages around after failure, for example, -EIO from the block
> layer is a contract for unrecoverable errors so it is pointless to
> keep them dirty). You'd need a specialized interface to clear-out
> the errors (and drop the dirty pages), or potentially just remount
> the filesystem.

Well firstly that's not necessarily the question. ENOSPC is not an
unrecoverable error. And even unrecoverable errors for a single write
doesn't mean the write will never be able to succeed in the future.
But secondly doesn't such an interface already exist? When the device
is dropped any dirty pages already get dropped with it. What's the
point in dropping them but keeping the failing device?

But just to underline the point. "pointless to keep them dirty" is
exactly backwards from the application's point of view. If the error
writing to persistent media really is unrecoverable then it's all the
more critical that the pages be kept so the data can be copied to some
other device. The last thing user space expects to happen is if the
data can't be written to persistent storage then also immediately
delete it from RAM. (And the *really* last thing user space expects is
for this to happen and return no error.)

--
greg

On Mon, Apr 09, 2018 at 09:45:40AM +0100, Greg Stark wrote:
> On 8 April 2018 at 22:47, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:
> > On Sun, Apr 08, 2018 at 10:23:21PM +0100, Greg Stark wrote:
> >> On 8 April 2018 at 04:27, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> >> > On 8 April 2018 at 10:16, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
> >
> > The question is, what should the kernel and application do in cases
> > where this is simply not possible (according to freebsd that keeps
> > dirty pages around after failure, for example, -EIO from the block
> > layer is a contract for unrecoverable errors so it is pointless to
> > keep them dirty). You'd need a specialized interface to clear-out
> > the errors (and drop the dirty pages), or potentially just remount
> > the filesystem.
>
> Well firstly that's not necessarily the question. ENOSPC is not an
> unrecoverable error. And even unrecoverable errors for a single write
> doesn't mean the write will never be able to succeed in the future.

To make things a bit simpler, let us focus on EIO for the moment.
The contract between the block layer and the filesystem layer is
assumed to be that of, when an EIO is propagated up to the fs,
then you may assume that all possibilities for recovering have
been exhausted in lower layers of the stack. Mind you, I am not
claiming that this contract is either documented or necessarily
respected (in fact there have been studies on the error propagation
and handling of the block layer, see [1]). Let us assume that
this is the design contract though (which appears to be the case
across a number of open-source kernels), and if not - it's a bug.
In this case, indeed the specific write()s will never be able
to succeed in the future, at least not as long as the BIOs are
allocated to the specific failing LBAs.

> But secondly doesn't such an interface already exist? When the device
> is dropped any dirty pages already get dropped with it. What's the
> point in dropping them but keeping the failing device?

I think there are degrees of failure. There are certainly cases
where one may encounter localized unrecoverable medium errors
(specific to certain LBAs) that are non-maskable from the block
layer and below. That does not mean that the device is dropped
at all, so it does make sense to continue all other operations
to all other regions of the device that are functional. In cases
of total device failure, then the filesystem will prevent you
from proceeding anyway.

> But just to underline the point. "pointless to keep them dirty" is
> exactly backwards from the application's point of view. If the error
> writing to persistent media really is unrecoverable then it's all the
> more critical that the pages be kept so the data can be copied to some
> other device. The last thing user space expects to happen is if the
> data can't be written to persistent storage then also immediately
> delete it from RAM. (And the *really* last thing user space expects is
> for this to happen and return no error.)

Right. This implies though that apart from the kernel having
to keep around the dirtied-but-unrecoverable pages for an
unbounded time, that there's further an interface for obtaining
the exact failed pages so that you can read them back. This in
turn means that there needs to be an association between the
fsync() caller and the specific dirtied pages that the caller
intents to drain (for which we'd need an fsync_range(), among
other things). BTW, currently the failed writebacks are not
dropped from memory, but rather marked clean. They could be
lost though due to memory pressure or due to explicit request
(e.g. proc drop_caches), unless mlocked.

There is a clear responsibility of the application to keep
its buffers around until a successful fsync(). The kernels
do report the error (albeit with all the complexities of
dealing with the interface), at which point the application
may not assume that the write()s where ever even buffered
in the kernel page cache in the first place.

What you seem to be asking for is the capability of dropping
buffers over the (kernel) fence and idemnifying the application
from any further responsibility, i.e. a hard assurance
that either the kernel will persist the pages or it will
keep them around till the application recovers them
asynchronously, the filesystem is unmounted, or the system
is rebooted.

Best regards,
Anthony

[1] https://www.usenix.org/legacy/event/fast08/tech/full_papers/gunawi/gunawi.pdf

On 9 April 2018 at 11:50, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:

> What you seem to be asking for is the capability of dropping
> buffers over the (kernel) fence and idemnifying the application
> from any further responsibility, i.e. a hard assurance
> that either the kernel will persist the pages or it will
> keep them around till the application recovers them
> asynchronously, the filesystem is unmounted, or the system
> is rebooted.
>

That seems like a perfectly reasonable position to take, frankly.

The whole _point_ of an Operating System should be that you can do exactly
that. As a developer I should be able to call write() and fsync() and know
that if both calls have succeeded then the result is on disk, no matter
what another application has done in the meantime. If that's a "difficult"
problem then that's the OS's problem, not mine. If the OS doesn't do that,
it's _not_doing_its_job_.

Geoff

On 9 April 2018 at 18:50, Anthony Iliopoulos <ailiop(at)altatus(dot)com> wrote:

>
> There is a clear responsibility of the application to keep
> its buffers around until a successful fsync(). The kernels
> do report the error (albeit with all the complexities of
> dealing with the interface), at which point the application
> may not assume that the write()s where ever even buffered
> in the kernel page cache in the first place.
>

> What you seem to be asking for is the capability of dropping
> buffers over the (kernel) fence and idemnifying the application
> from any further responsibility, i.e. a hard assurance
> that either the kernel will persist the pages or it will
> keep them around till the application recovers them
> asynchronously, the filesystem is unmounted, or the system
> is rebooted.
>

That's what Pg appears to assume now, yes.

Whether that's reasonable is a whole different topic.

I'd like a middle ground where the kernel lets us register our interest and
tells us if it lost something, without us having to keep eight million FDs
open for some long period. "Tell us about anything that happens under
pgdata/" or an inotify-style per-directory-registration option. I'd even
say that's ideal.

In the mean time, I propose that we fsync() on close() before we age FDs
out of the LRU on backends. Yes, that will hurt throughput and cause
stalls, but we don't seem to have many better options. At least it'll only
flush what we actually wrote to the OS buffers not what we may have in
shared_buffers. If the bgwriter does the same thing, we should be 100% safe
from this problem on 4.13+, and it'd be trivial to make it a GUC much like
the fsync or full_page_writes options that people can turn off if they know
the risks / know their storage is safe / don't care.

Some keen person who wants to later could optimise it by adding a fsync
worker thread pool in backends, so we don't block the main thread. Frankly
that might be a nice thing to have in the checkpointer anyway. But it's out
of scope for fixing this in durability terms.

I'm partway through a patch that makes fsync panic on errors now. Once
that's done, the next step will be to force fsync on close() in md and see
how we go with that.

Thoughts?

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services