Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit8e293e6

Browse files
anarazelnmisch
andcommitted
aio: Make AIO more compatible with valgrind
In some edge cases valgrind flags issues with the memory referenced byIOs. All of the cases addressed in this change are false positives.Most of the false positives are caused by UnpinBuffer[NoOwner] marking bufferdata as inaccessible. This happens even though the AIO subsystem still holds apin. That's good, there shouldn't be accesses to the buffer outside of AIOrelated code until it is pinned by "user" code again. But it requires someexplicit work - if the buffer is not pinned by the current backend, we need toexplicitly mark the buffer data accessible/inaccessible while executingcompletion callbacks.That however causes a cascading issue in IO workers: After the completioncallbacks for a buffer is executed, the page is marked as inaccessible. Ifsubsequently the same worker is executing IO targeting the same buffer, wewould get an error, as the memory is still marked inaccessible. To avoid that,we need to explicitly mark the memory as accessible in IO workers.Another issue is that IO executed in workers or via io_uring will not markmemory as DEFINED. In the case of workers that is because valgrind does nottrack memory definedness across processes. For io_uring that is becausevalgrind does not understand io_uring, and therefore its IOs never mark memoryas defined, whether the completions are processed in the defining process orin another context. It's not entirely clear how to best solve that. Thecurrent user of AIO is not affected, as it explicitly marks buffers as DEFINED& NOACCESS anyway. Defer solving this issue until we have a user withdifferent needs.Per buildfarm animal skink.Reviewed-by: Noah Misch <noah@leadboat.com>Co-authored-by: Noah Misch <noah@leadboat.com>Discussion:https://postgr.es/m/3pd4322mogfmdd5nln3zphdwhtmq3rzdldqjwb2sfqzcgs22lf@ok2gletdaoe6
1 parent8ab4241 commit8e293e6

File tree

5 files changed

+64
-0
lines changed

5 files changed

+64
-0
lines changed

‎src/backend/storage/aio/aio_io.c

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,3 +210,26 @@ pgaio_io_uses_fd(PgAioHandle *ioh, int fd)
210210

211211
return false;/* silence compiler */
212212
}
213+
214+
/*
215+
* Return the iovec and its length. Currently only expected to be used by
216+
* debugging infrastructure
217+
*/
218+
int
219+
pgaio_io_get_iovec_length(PgAioHandle*ioh,structiovec**iov)
220+
{
221+
Assert(ioh->state >=PGAIO_HS_DEFINED);
222+
223+
*iov=&pgaio_ctl->iovecs[ioh->iovec_off];
224+
225+
switch (ioh->op)
226+
{
227+
casePGAIO_OP_READV:
228+
returnioh->op_data.read.iov_length;
229+
casePGAIO_OP_WRITEV:
230+
returnioh->op_data.write.iov_length;
231+
default:
232+
pg_unreachable();
233+
return0;
234+
}
235+
}

‎src/backend/storage/aio/method_worker.c

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
#include"storage/latch.h"
4343
#include"storage/proc.h"
4444
#include"tcop/tcopprot.h"
45+
#include"utils/memdebug.h"
4546
#include"utils/ps_status.h"
4647
#include"utils/wait_event.h"
4748

@@ -529,6 +530,24 @@ IoWorkerMain(const void *startup_data, size_t startup_data_len)
529530
error_errno=0;
530531
error_ioh=NULL;
531532

533+
/*
534+
* As part of IO completion the buffer will be marked as NOACCESS,
535+
* until the buffer is pinned again - which never happens in io
536+
* workers. Therefore the next time there is IO for the same
537+
* buffer, the memory will be considered inaccessible. To avoid
538+
* that, explicitly allow access to the memory before reading data
539+
* into it.
540+
*/
541+
#ifdefUSE_VALGRIND
542+
{
543+
structiovec*iov;
544+
uint16iov_length=pgaio_io_get_iovec_length(ioh,&iov);
545+
546+
for (inti=0;i<iov_length;i++)
547+
VALGRIND_MAKE_MEM_UNDEFINED(iov[i].iov_base,iov[i].iov_len);
548+
}
549+
#endif
550+
532551
/*
533552
* We don't expect this to ever fail with ERROR or FATAL, no need
534553
* to keep error_ioh set to the IO.

‎src/backend/storage/buffer/bufmgr.c

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6881,6 +6881,19 @@ buffer_readv_complete_one(PgAioTargetData *td, uint8 buf_off, Buffer buffer,
68816881
/* Check for garbage data. */
68826882
if (!failed)
68836883
{
6884+
/*
6885+
* If the buffer is not currently pinned by this backend, e.g. because
6886+
* we're completing this IO after an error, the buffer data will have
6887+
* been marked as inaccessible when the buffer was unpinned. The AIO
6888+
* subsystem holds a pin, but that doesn't prevent the buffer from
6889+
* having been marked as inaccessible. The completion might also be
6890+
* executed in a different process.
6891+
*/
6892+
#ifdefUSE_VALGRIND
6893+
if (!BufferIsPinned(buffer))
6894+
VALGRIND_MAKE_MEM_DEFINED(bufdata,BLCKSZ);
6895+
#endif
6896+
68846897
if (!PageIsVerified((Page)bufdata,tag.blockNum,piv_flags,
68856898
failed_checksum))
68866899
{
@@ -6899,6 +6912,12 @@ buffer_readv_complete_one(PgAioTargetData *td, uint8 buf_off, Buffer buffer,
68996912
elseif (*failed_checksum)
69006913
*ignored_checksum= true;
69016914

6915+
/* undo what we did above */
6916+
#ifdefUSE_VALGRIND
6917+
if (!BufferIsPinned(buffer))
6918+
VALGRIND_MAKE_MEM_NOACCESS(bufdata,BLCKSZ);
6919+
#endif
6920+
69026921
/*
69036922
* Immediately log a message about the invalid page, but only to the
69046923
* server log. The reason to do so immediately is that this may be

‎src/backend/storage/smgr/smgr.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -746,6 +746,8 @@ smgrreadv(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
746746
* responsible for pgaio_result_report() to mirror that news to the user (if
747747
* the IO results in PGAIO_RS_WARNING) or abort the (sub)transaction (if
748748
* PGAIO_RS_ERROR).
749+
* - Under Valgrind, the "buffers" memory may or may not change status to
750+
* DEFINED, depending on io_method and concurrent activity.
749751
*/
750752
void
751753
smgrstartreadv(PgAioHandle*ioh,

‎src/include/storage/aio_internal.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,7 @@ extern PgAioResult pgaio_io_call_complete_local(PgAioHandle *ioh);
344344
externvoidpgaio_io_perform_synchronously(PgAioHandle*ioh);
345345
externconstchar*pgaio_io_get_op_name(PgAioHandle*ioh);
346346
externboolpgaio_io_uses_fd(PgAioHandle*ioh,intfd);
347+
externintpgaio_io_get_iovec_length(PgAioHandle*ioh,structiovec**iov);
347348

348349
/* aio_target.c */
349350
externboolpgaio_io_can_reopen(PgAioHandle*ioh);

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp