On Wed, Jul 19, 2017 at 11:26:37AM +0200, Jan Kara wrote:
> 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)
:D
sure, I'll resend.
>
> ...
>
> > + 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!
ok.
Thanks!
-Lukas
>
> Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
