On Wed 19-07-17 10:48:44, Lukas Czerner wrote:
> Currently when mixing buffered reads and asynchronous direct writes it
> is possible to end up with the situation where we have stale data in the
> page cache while the new data is already written to disk. This is
> permanent until the affected pages are flushed away. Despite the fact
> that mixing buffered and direct IO is ill-advised it does pose a thread
> for a data integrity, is unexpected and should be fixed.
>
> Fix this by deferring completion of asynchronous direct writes to a
> process context in the case that there are mapped pages to be found in
> the inode. Later before the completion in dio_complete() invalidate
> the pages in question. This ensures that after the completion the pages
> in the written area are either unmapped, or populated with up-to-date
> data. Also do the same for the iomap case which uses
> iomap_dio_complete() instead.
>
> This has a side effect of deferring the completion to a process context
> for every AIO DIO that happens on inode that has pages mapped. However
> since the consensus is that this is ill-advised practice the performance
> implication should not be a problem.
>
> This was based on proposal from Jeff Moyer, thanks!
>
> Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
> Cc: Jeff Moyer <jmoyer@xxxxxxxxxx>
> ---
> v2: Remove leftover ret variable from invalidate call in iomap_dio_complete
> v3: Do not invalidate in case of error. Add some coments
> v4: Remove unnecessary variable, remove unnecessary inner braces
Looks good to me now, just two style nits below. You can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 08cf278..efd3246 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -258,6 +258,12 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async)
> if (ret == 0)
> ret = transferred;
>
> + if ((ret > 0) &&
> + (dio->op == REQ_OP_WRITE && dio->inode->i_mapping->nrpages))
Heh, you seem to love braces. The general rule is that braces should be
around bit-ops (as there people find the priority unclear and also it is
too easy to forget to add those braces when negating the condition) but not
around comparison or such. I.e. the above would be:
if (ret > 0 && dio->op == REQ_OP_WRITE &&
dio->inode->i_mapping->nrpages)
...
> + if (dio->is_async && iov_iter_rw(iter) == WRITE) {
> + retval = 0;
> + if ((iocb->ki_filp->f_flags & O_DSYNC) ||
> + IS_SYNC(iocb->ki_filp->f_mapping->host))
> + retval = dio_set_defer_completion(dio);
> + else if (!dio->inode->i_sb->s_dio_done_wq)
> + /*
> + * In case of AIO write racing with buffered read we
> + * need to defer completion. We can't decide this now,
> + * however the workqueue needs to be initialized here.
> + */
> + retval = sb_init_dio_done_wq(dio->inode->i_sb);
Curly braces here please. When command block is multi-line we enforce those
despite it is only a single statement and thus they are not necessary
strictly speaking. Thanks!
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
