-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always validate checksums for Direct I/O reads #16598
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1008,7 +1008,9 @@ abd_borrow_buf_copy(abd_t *abd, size_t n) | |
* borrowed. We can not ASSERT that the contents of the buffer have not changed | ||
* if it is composed of user pages because the pages can not be placed under | ||
* write protection and the user could have possibly changed the contents in | ||
* the pages at any time. | ||
* the pages at any time. This is also an issue for Direct I/O reads. Checksum | ||
* verifications in the ZIO pipeline check fo rthis issue and handle it by | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "check for this issue" |
||
* returning an error on checksum verification failure. | ||
*/ | ||
void | ||
abd_return_buf(abd_t *abd, void *buf, size_t n) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -34,6 +34,7 @@ | |||||
#include <sys/zap.h> | ||||||
#include <sys/abd.h> | ||||||
#include <sys/zthr.h> | ||||||
#include <sys/fm/fs/zfs.h> | ||||||
|
||||||
/* | ||||||
* An indirect vdev corresponds to a vdev that has been removed. Since | ||||||
|
@@ -1832,6 +1833,19 @@ vdev_indirect_io_done(zio_t *zio) | |||||
|
||||||
zio_bad_cksum_t zbc; | ||||||
int ret = zio_checksum_error(zio, &zbc); | ||||||
/* | ||||||
* Any Direct I/O read with that has a checksum error must be treated | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* as suspicious as the contents of the buffer could be getting | ||||||
* manipulated while the I/O is taking place. The checksum verify error | ||||||
* will be reported to the top-level VDEV. | ||||||
*/ | ||||||
if (zio->io_flags & ZIO_FLAG_DIO_READ && ret == ECKSUM) { | ||||||
zio->io_error = ret; | ||||||
zio->io_flags |= ZIO_FLAG_DIO_CHKSUM_ERR; | ||||||
zio_dio_chksum_verify_error_report(zio); | ||||||
ret = 0; | ||||||
} | ||||||
|
||||||
if (ret == 0) { | ||||||
zio_checksum_verified(zio); | ||||||
return; | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -303,6 +303,8 @@ zfs_read(struct znode *zp, zfs_uio_t *uio, int ioflag, cred_t *cr) | |||||
(void) cr; | ||||||
int error = 0; | ||||||
boolean_t frsync = B_FALSE; | ||||||
boolean_t dio_checksum_failure = B_FALSE; | ||||||
ssize_t nbytes; | ||||||
|
||||||
zfsvfs_t *zfsvfs = ZTOZSB(zp); | ||||||
if ((error = zfs_enter_verify_zp(zfsvfs, zp, FTAG)) != 0) | ||||||
|
@@ -407,7 +409,8 @@ zfs_read(struct znode *zp, zfs_uio_t *uio, int ioflag, cred_t *cr) | |||||
} | ||||||
|
||||||
while (n > 0) { | ||||||
ssize_t nbytes = MIN(n, chunk_size - | ||||||
top: | ||||||
nbytes = MIN(n, chunk_size - | ||||||
P2PHASE(zfs_uio_offset(uio), chunk_size)); | ||||||
#ifdef UIO_NOCOPY | ||||||
if (zfs_uio_segflg(uio) == UIO_NOCOPY) | ||||||
|
@@ -424,8 +427,26 @@ zfs_read(struct znode *zp, zfs_uio_t *uio, int ioflag, cred_t *cr) | |||||
|
||||||
if (error) { | ||||||
/* convert checksum errors into IO errors */ | ||||||
if (error == ECKSUM) | ||||||
error = SET_ERROR(EIO); | ||||||
if (error == ECKSUM) { | ||||||
/* | ||||||
* If a Direct I/O read returned a checksum | ||||||
* verify error, then it must be treated as | ||||||
* suspicious. The contents of the buffer could | ||||||
* have beeen manipulated while the I/O was in | ||||||
* flight. In this case, the remainder of I/O | ||||||
* request will just be just be reissued through | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* the ARC. | ||||||
*/ | ||||||
if (uio->uio_extflg & UIO_DIRECT) { | ||||||
dio_checksum_failure = B_TRUE; | ||||||
uio->uio_extflg &= ~UIO_DIRECT; | ||||||
n += dio_remaining_resid; | ||||||
dio_remaining_resid = 0; | ||||||
goto top; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||||||
} else { | ||||||
error = SET_ERROR(EIO); | ||||||
} | ||||||
} | ||||||
|
||||||
#if defined(__linux__) | ||||||
/* | ||||||
|
@@ -472,6 +493,10 @@ zfs_read(struct znode *zp, zfs_uio_t *uio, int ioflag, cred_t *cr) | |||||
out: | ||||||
zfs_rangelock_exit(lr); | ||||||
|
||||||
if (dio_checksum_failure == B_TRUE) { | ||||||
uio->uio_extflg |= UIO_DIRECT; | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: parenthesis can be removed. |
||||||
|
||||||
/* | ||||||
* Cleanup for Direct I/O if requested. | ||||||
*/ | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"for any ABDs"
"we just iterate"