On Fri, Nov 17, 2017 at 11:39:25AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <[email protected]> > > If two programs simultaneously try to write to the same part of a file > via direct IO and buffered IO, there's a chance that the post-diowrite > pagecache invalidation will fail on the dirty page. When this happens, > the dio write succeeded, which means that the page cache is no longer > coherent with the disk! > > Programs are not supposed to mix IO types and this is a clear case of > data corruption, so store an EIO which will be reflected to userspace > during the next fsync. Replace the WARN_ON with a ratelimited pr_crit > so that the developers have /some/ kind of breadcrumb to track down the > offending program(s) and file(s) involved. >
Looks good to me, thanks for addressing the warning. Reviewed-by: Liu Bo <[email protected]> Thanks, -liubo > Signed-off-by: Darrick J. Wong <[email protected]> > --- > fs/direct-io.c | 24 +++++++++++++++++++++++- > fs/iomap.c | 12 ++++++++++-- > include/linux/fs.h | 1 + > 3 files changed, 34 insertions(+), 3 deletions(-) > > diff --git a/fs/direct-io.c b/fs/direct-io.c > index 98fe132..ef5d12a 100644 > --- a/fs/direct-io.c > +++ b/fs/direct-io.c > @@ -219,6 +219,27 @@ static inline struct page *dio_get_page(struct dio *dio, > return dio->pages[sdio->head]; > } > > +/* > + * Warn about a page cache invalidation failure during a direct io write. > + */ > +void dio_warn_stale_pagecache(struct file *filp) > +{ > + static DEFINE_RATELIMIT_STATE(_rs, 30 * HZ, DEFAULT_RATELIMIT_BURST); > + char pathname[128]; > + struct inode *inode = file_inode(filp); > + char *path; > + > + errseq_set(&inode->i_mapping->wb_err, -EIO); > + if (__ratelimit(&_rs)) { > + path = file_path(filp, pathname, sizeof(pathname)); > + if (IS_ERR(path)) > + path = "(unknown)"; > + pr_crit("Page cache invalidation failure on direct I/O. > Possible data corruption due to collision with buffered I/O!\n"); > + pr_crit("File: %s PID: %d Comm: %.20s\n", path, current->pid, > + current->comm); > + } > +} > + > /** > * dio_complete() - called when all DIO BIO I/O has been completed > * @offset: the byte offset in the file of the completed operation > @@ -290,7 +311,8 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, > unsigned int flags) > err = invalidate_inode_pages2_range(dio->inode->i_mapping, > offset >> PAGE_SHIFT, > (offset + ret - 1) >> PAGE_SHIFT); > - WARN_ON_ONCE(err); > + if (err) > + dio_warn_stale_pagecache(dio->iocb->ki_filp); > } > > if (!(dio->flags & DIO_SKIP_DIO_COUNT)) > diff --git a/fs/iomap.c b/fs/iomap.c > index 5011a96..028f329 100644 > --- a/fs/iomap.c > +++ b/fs/iomap.c > @@ -753,7 +753,8 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio) > err = invalidate_inode_pages2_range(inode->i_mapping, > offset >> PAGE_SHIFT, > (offset + dio->size - 1) >> PAGE_SHIFT); > - WARN_ON_ONCE(err); > + if (err) > + dio_warn_stale_pagecache(iocb->ki_filp); > } > > inode_dio_end(file_inode(iocb->ki_filp)); > @@ -1012,9 +1013,16 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > if (ret) > goto out_free_dio; > > + /* > + * Try to invalidate cache pages for the range we're direct > + * writing. If this invalidation fails, tough, the write will > + * still work, but racing two incompatible write paths is a > + * pretty crazy thing to do, so we don't support it 100%. > + */ > ret = invalidate_inode_pages2_range(mapping, > start >> PAGE_SHIFT, end >> PAGE_SHIFT); > - WARN_ON_ONCE(ret); > + if (ret) > + dio_warn_stale_pagecache(iocb->ki_filp); > ret = 0; > > if (iov_iter_rw(iter) == WRITE && !is_sync_kiocb(iocb) && > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 2690864..0e5f060 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2976,6 +2976,7 @@ enum { > }; > > void dio_end_io(struct bio *bio); > +void dio_warn_stale_pagecache(struct file *filp); > > ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode, > struct block_device *bdev, struct iov_iter *iter, > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
