Re: Silent data corruption in blkdev_direct_IO()
From: Martin Wilck <hidden>
Date: 2018-07-12 16:11:56
On Thu, 2018-07-12 at 09:08 -0600, Jens Axboe wrote:
On 7/12/18 8:36 AM, Hannes Reinecke wrote:quoted
Hi Jens, Christoph, we're currently hunting down a silent data corruption occurring due to commit 72ecad22d9f1 ("block: support a full bio worth of IO for simplified bdev direct-io"). While the whole thing is still hazy on the details, the one thing we've found is that reverting that patch fixes the data corruption. And looking closer, I've found this: static ssize_t blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter) { int nr_pages; nr_pages = iov_iter_npages(iter, BIO_MAX_PAGES + 1); if (!nr_pages) return 0; if (is_sync_kiocb(iocb) && nr_pages <= BIO_MAX_PAGES) return __blkdev_direct_IO_simple(iocb, iter, nr_pages); return __blkdev_direct_IO(iocb, iter, min(nr_pages, BIO_MAX_PAGES)); } When checking the call path __blkdev_direct_IO()->bio_alloc_bioset()->bvec_alloc() I found that bvec_alloc() will fail if nr_pages > BIO_MAX_PAGES. So why is there the check for 'nr_pages <= BIO_MAX_PAGES' ? It's not that we can handle it in __blkdev_direct_IO() ...The logic could be cleaned up like below, the sync part is really all we care about. What is the test case for this? async or sync?
It's sync, and the corruption is in the __blkdev_direct_IO_simple() path. It starts to occur with your "block: support a full bio worth of IO for simplified bdev direct-io" patch, which causes the "simple" path to be taken for larger IO sizes. Martin
quoted hunk ↗ jump to hunk
I also don't remember why it's BIO_MAX_PAGES + 1...diff --git a/fs/block_dev.c b/fs/block_dev.c index 0dd87aaeb39a..14ef3d71b55f 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c@@ -424,13 +424,13 @@ blkdev_direct_IO(struct kiocb *iocb, structiov_iter *iter) { int nr_pages; - nr_pages = iov_iter_npages(iter, BIO_MAX_PAGES + 1); + nr_pages = iov_iter_npages(iter, BIO_MAX_PAGES); if (!nr_pages) return 0; - if (is_sync_kiocb(iocb) && nr_pages <= BIO_MAX_PAGES) + if (is_sync_kiocb(iocb)) return __blkdev_direct_IO_simple(iocb, iter, nr_pages); - return __blkdev_direct_IO(iocb, iter, min(nr_pages, BIO_MAX_PAGES)); + return __blkdev_direct_IO(iocb, iter, nr_pages); } static __init int blkdev_init(void)
-- Dr. Martin Wilck [off-list ref], Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)